|
From: Manomugdha B. <man...@ke...> - 2020-08-15 13:36:50
|
Hi,
I have a 32 bit application (user space) which communicate with a kernel module through compat_ioctl(). My system is
# uname -a
Linux chassis1-board1-port5 3.10 #1 SMP Fri Apr 24 02:31:48 PDT 2020 mips64 GNU/Linux
Ioctl function is following:
long mgr_compat_ioctl(struct file *pFile, unsigned int cmd, unsigned long arg)
inside this function definition, I am getting correct cmd value e.g. 0xc018786f. the variable to which this cmd is to be matched has exactly same value of 0xc018786f (printk shows same value). But still if check fails.
Sample code is like this:
int mgr_ioctl_common (struct inode *pInode, struct file *pFile,
unsigned int cmd, unsigned long arg)
{
int err = -EFAULT;
int minor;
void *pUserStruct = (void *)arg;
SkixpppMgrDevDesc *pDev = NULL;
tKHandle retHandle;
unsigned int openLink = 3222829167;
if (openLink == cmd) { //printk shows value of cmd is 3222829167
printk("matches\n");
} else {
printk("no matche\n");
}
}
Why this if check fails here?
Regards,
Mano
|
|
From: Manomugdha B. <man...@ke...> - 2020-08-15 13:36:00
Attachments:
image001.png
|
(openLink - cmd) shows zero still if check fails. Note: this is happening only under valgrind! Regards, Mano, IxNetwork [Keysight-signature-block-5]<http://www.keysight.com/> From: Manomugdha Biswas Sent: Saturday, August 15, 2020 6:38 PM To: val...@li... Subject: compat_ioctl cmd does not match even if it shows same value Hi, I have a 32 bit application (user space) which communicate with a kernel module through compat_ioctl(). My system is # uname -a Linux chassis1-board1-port5 3.10 #1 SMP Fri Apr 24 02:31:48 PDT 2020 mips64 GNU/Linux Ioctl function is following: long mgr_compat_ioctl(struct file *pFile, unsigned int cmd, unsigned long arg) inside this function definition, I am getting correct cmd value e.g. 0xc018786f. the variable to which this cmd is to be matched has exactly same value of 0xc018786f (printk shows same value). But still if check fails. Sample code is like this: int mgr_ioctl_common (struct inode *pInode, struct file *pFile, unsigned int cmd, unsigned long arg) { int err = -EFAULT; int minor; void *pUserStruct = (void *)arg; SkixpppMgrDevDesc *pDev = NULL; tKHandle retHandle; unsigned int openLink = 3222829167; if (openLink == cmd) { //printk shows value of cmd is 3222829167 printk("matches\n"); } else { printk("no matche\n"); } } Why this if check fails here? Regards, Mano |
|
From: John R. <jr...@bi...> - 2020-08-15 17:58:00
|
> I have a 32 bit application (user space) which communicate with a kernel module through compat_ioctl(). My system is
>
> # uname -a
>
> Linux chassis1-board1-port5 3.10 #1 SMP Fri Apr 24 02:31:48 PDT 2020 mips64 GNU/Linux
Which version of valgrind?
The source code of valgrind:
commit 24b247aec5397cad5f6cfcf885f118e90fea8735 (HEAD -> master, origin/master, origin/HEAD)
Date: Sat Aug 15 16:54:14 2020 +0200
does not contain the string 'compat_ioctl' anywhere, so you must show us the prototype
and the environment (32-bit or 64-bit) for compat_ioctl.
> Ioctl function is following:
>
> long mgr_compat_ioctl(struct file *pFile, unsigned int cmd, unsigned long arg)
>
> inside this function definition, I am getting correct cmd value e.g. 0xc018786f.
The fact that you write '0xc018786f' here, but '3222829167' in the code,
shows that you are not sufficiently paranoid. First, if the bit pattern is important
then you should write hex. If you insist on decimal, then you should write '3222829167u'
to remind yourself and the compiler that the value is unsigned; otherwise because
3222829167 exceeds INT_MAX then a compiler is allowed to interpret it as -1072138129
(or ANY VALUE WHATSOEVER!!!)
Then, it is very likely that your problem lies in a mismatch of width (64-bit vs 32-bit)
and/or signedness at one or more interfaces (subroutine call or system call).
You say "printk shows same value" but you did not say what format conversion
you specified to printk, therefore no one can determine what the actual value is.
This is likely to be important, because printk is implemented using "..." varargs,
and on most 64-bit machines that means that most scalar actual arguments are promoted
to 64-bit width upon the transition into the varargs mechanism, and it is UNSPECIFIED
whether the conversion from 32-bit to 64-bit is sign-extended or zero-extended.
That makes it extremely important that each varargs access uses the desired width
and signedness.
Due to the possibility of bugs, the only way to be sure of what is happening
is to look at the assembly-language code, and examine registers and values
while single-stepping actual execution.
--
|
|
From: Manomugdha B. <man...@ke...> - 2020-08-15 19:35:11
|
Hi John,
Valgrind version: valgrind-3.14.0
compat_ioctl prototype:
/* File operations core */
struct file_operations mgr_fops =
{
owner:
THIS_MODULE,
open:
mgr_open_common,
FILE_OPS_IOCTL(mgr_ioctl_common),
#ifdef CONFIG_COMPAT
compat_ioctl:
mgr_compat_ioctl,
#endif
poll:
mgr_poll_common,
release:
mgr_close_common
};
long mgr_compat_ioctl(struct file *pFile, unsigned int cmd, unsigned long arg)
{
int err = -EFAULT;
//unsigned int openLink = kIoctlOpenEthernetLink;
unsigned int openLink = 3222829167;
printk("cmd 0x%x/%u, openLink 0x%x/%u\n", cmd, cmd, openLink, openLink);
if (openLink == cmd) {
printk("match\n");
} else {
printk("no match: cmd 0x%x/%u, openLink 0x%x/%u\n", cmd, cmd, openLink, openLink);
}
}
Output is:
[409204.415924/1] cmd 0xc018786f/3222829167, openLink 0xc018786f/3222829167
[409204.415933/1] no match: cmd 0xc018786f/3222829167, openLink 0xc018786f/3222829167
Note1: it fails at MSB. If i do memcmp instead of "if" then it works.
Note2: this is happening only under valgrind. When I run it without valgrind then it works fine.
Note3: this is a kernel module and ioctl is called from user space application.
Regards,
Mano
-----Original Message-----
From: John Reiser <jr...@bi...>
Sent: Saturday, August 15, 2020 11:28 PM
To: val...@li...
Subject: Re: [Valgrind-users] compat_ioctl cmd does not match even if it shows same value
[EXTERNAL]
> I have a 32 bit application (user space) which communicate with a
> kernel module through compat_ioctl(). My system is
>
> # uname -a
>
> Linux chassis1-board1-port5 3.10 #1 SMP Fri Apr 24 02:31:48 PDT 2020
> mips64 GNU/Linux
Which version of valgrind?
The source code of valgrind:
commit 24b247aec5397cad5f6cfcf885f118e90fea8735 (HEAD -> master, origin/master, origin/HEAD)
Date: Sat Aug 15 16:54:14 2020 +0200
does not contain the string 'compat_ioctl' anywhere, so you must show us the prototype and the environment (32-bit or 64-bit) for compat_ioctl.
> Ioctl function is following:
>
> long mgr_compat_ioctl(struct file *pFile, unsigned int cmd, unsigned
> long arg)
>
> inside this function definition, I am getting correct cmd value e.g. 0xc018786f.
The fact that you write '0xc018786f' here, but '3222829167' in the code, shows that you are not sufficiently paranoid. First, if the bit pattern is important then you should write hex. If you insist on decimal, then you should write '3222829167u'
to remind yourself and the compiler that the value is unsigned; otherwise because
3222829167 exceeds INT_MAX then a compiler is allowed to interpret it as -1072138129 (or ANY VALUE WHATSOEVER!!!) Then, it is very likely that your problem lies in a mismatch of width (64-bit vs 32-bit) and/or signedness at one or more interfaces (subroutine call or system call).
You say "printk shows same value" but you did not say what format conversion you specified to printk, therefore no one can determine what the actual value is.
This is likely to be important, because printk is implemented using "..." varargs, and on most 64-bit machines that means that most scalar actual arguments are promoted to 64-bit width upon the transition into the varargs mechanism, and it is UNSPECIFIED whether the conversion from 32-bit to 64-bit is sign-extended or zero-extended.
That makes it extremely important that each varargs access uses the desired width and signedness.
Due to the possibility of bugs, the only way to be sure of what is happening is to look at the assembly-language code, and examine registers and values while single-stepping actual execution.
--
_______________________________________________
Valgrind-users mailing list
Val...@li...
https://urldefense.com/v3/__https://lists.sourceforge.net/lists/listinfo/valgrind-users__;!!I5pVk4LIGAfnvw!w9ZQebKExIzQOBnXRp0AgAnSuApBAKyRQwK8-da-j4Y9NjzgGTYvJNpMt7LSIX7caCZHaZDi$
|
|
From: John R. <jr...@bi...> - 2020-08-16 14:16:07
|
> long mgr_compat_ioctl(struct file *pFile, unsigned int cmd, unsigned long arg)
> {
> int err = -EFAULT;
> //unsigned int openLink = kIoctlOpenEthernetLink;
> unsigned int openLink = 3222829167;
"3222829167" is not a legal 32-bit value in source code. You MUST use "3222829167u"
with a trailing 'u'. Do not post or reply to this mailing list until you have changed
the source, re-compiled, re-linked, and re-run the test.
>
> printk("cmd 0x%x/%u, openLink 0x%x/%u\n", cmd, cmd, openLink, openLink);
// [409204.415924/1] cmd 0xc018786f/3222829167, openLink 0xc018786f/3222829167
The documentation for ioctl(), as shown by running the command "man 2 ioctl",
says that the prototype is:
int ioctl(int fd, unsigned long request, ...);
Notice that the second parameter has type "unsigned long", which is 64 bits.
In contrast, the definition of this function 'mgr_compat_ioctl' has a second
parameter type of "unsigned int", which is 32 bits. Beware of this difference.
[Also note that the width of the return value differs: 'int' in the documentation,
'long' in the implementation; but this should not matter here.]
Try an experiment: change the implementation to:
long mgr_compat_ioctl(struct file *pFile, unsigned long cmd, unsigned long arg)
where the second parameter has 'long' width instead of 'int'. If necessary,
(for instance, in the "struct file_operations mgr_fops" table of function pointers)
then cast the type of the function pointer &mgr_compat_ioctl to hide the difference
in the type of the second parameter. At the same time, change the printk format
conversions corresponding to 'cmd' to become "0x%lx/%lu" which has two 'l' modifiers.
The experiment might reveal that the second parameter to mgr_compat_ioctl is being passed
a 64-bit value whose upper 32 bits are not all zero.
>
> if (openLink == cmd) {
> printk("match\n");
> } else {
> printk("no match: cmd 0x%x/%u, openLink 0x%x/%u\n", cmd, cmd, openLink, openLink);
> }
Post the assembly-language code for the function mgr_compat_ioctl. The safest way
is to use "(gdb) disassemble mgr_compat_ioctl" or "objdump --disaassemble",
but "gcc -S" usually is OK. We need to see EXACTLY what instructions are generated.
--
|
|
From: Patrick J. L. <lop...@gm...> - 2020-08-17 02:13:32
|
On Sat, Aug 15, 2020 at 10:59 AM John Reiser <jr...@bi...> wrote: > > The fact that you write '0xc018786f' here, but '3222829167' in the code, > shows that you are not sufficiently paranoid. First, if the bit pattern is important > then you should write hex. If you insist on decimal, then you should write '3222829167u' > to remind yourself and the compiler that the value is unsigned; otherwise because > 3222829167 exceeds INT_MAX then a compiler is allowed to interpret it as -1072138129 > (or ANY VALUE WHATSOEVER!!!) This is incorrect for C99 and indeed any compiler that supports "long long". On such systems, the integer constant 3222829167 has type "long long", and it is absolutely guaranteed to preserve that value when cast to unsigned. Assuming 32-bit int, of course. I agree it is poor style, but there is essentially zero chance it is causing a problem here. Although 32-bit vs. 64-bit confusion elsewhere certainly might. - Pat |
|
From: Paul F. <pj...@wa...> - 2020-08-17 08:53:53
|
> This is incorrect for C99 and indeed any compiler that supports "long long". On such systems, the integer constant 3222829167 > has type "long long", and it is absolutely guaranteed to preserve that value when cast to unsigned. Assuming 32-bit int, of course. It is also incorrect for C90. See here https://en.cppreference.com/w/c/language/integer_constant. For C90 an integer constant without suffix can be int, long int or unsigned long int. In this case is it would be unsigned long int. A+ Paul |