-Wswitch Clang warnings in drivers/scsi

DISCUSSION Nathan Chancellor 3 participants 9 messages Oct 4, 2018 – Oct 19, 2018 SCSI

Share & tools

Nathan Chancellor

author

2018-10-04 18:30 UTC #0 lore raw

Hi SCSI folks,

In an effort to get the kernel building warning free with Clang, we've come across an interesting occurrence in a few scsi drivers:

drivers/scsi/hpsa.c:6533:7: warning: overflow converting case value to switch condition type (2148024833 to 18446744071562609153) [-Wswitch]

        case CCISS_GETPCIINFO:
             ^
./include/uapi/linux/cciss_ioctl.h:65:26: note: expanded from macro 'CCISS_GETPCIINFO'
#define CCISS_GETPCIINFO _IOR(CCISS_IOC_MAGIC, 1, cciss_pci_info_struct)
                         ^
./include/uapi/asm-generic/ioctl.h:86:28: note: expanded from macro '_IOR'
#define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
                                ^
./include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
        (((dir)  << _IOC_DIRSHIFT) | \
        ^

I see this warning in drivers/scsi/hpsa.c and drivers/scsi/smartpqi/smartpqi_init.c on an arm64 allyesconfig build and it has also been reported in a couple of files in drivers/scsi/cxlflash.

As the warning states, there is an overflow because the switch statement's value is of type int but the switch value is greater than INT_MAX. I did a brief sweep of the tree and it seems that all uses of _IOC in switch statement values either are small enough to fit into size int or the value is of size unsigned int.

I am unsure of the implications of using a smaller _IOC value or converting all ioctls to expect a cmd of type unsigned int (especially since that has userspace implications) but I didn't see any negative ioctl commands. Some clarity and insight would be appreciated.

Thank you for your time,
Nathan

Bart Van Assche

2018-10-04 18:34 UTC #1 lore raw

Quoted 21 lines · Nathan Chancellor

Hi SCSI folks,
In an effort to get the kernel building warning free with Clang, we've come across an interesting occurrence in a few scsi drivers:
drivers/scsi/hpsa.c:6533:7: warning: overflow converting case value to switch condition type (2148024833 to 18446744071562609153) [-Wswitch]> case CCISS_GETPCIINFO:
^>./include/uapi/linux/cciss_ioctl.h:65:26: note: expanded from macro 'CCISS_GETPCIINFO'
#define CCISS_GETPCIINFO _IOR(CCISS_IOC_MAGIC, 1, cciss_pci_info_struct)
                        ^>./include/uapi/asm-generic/ioctl.h:86:28: note: expanded from macro '_IOR'
#define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
                               ^>./include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
       (((dir)  <<_IOC_DIRSHIFT) |\>        ^>
I see this warning in drivers/scsi/hpsa.c and drivers/scsi/smartpqi/smartpqi_init.c on an arm64 allyesconfig build and it has also been reported in a couple of files in drivers/scsi/cxlflash.
As the warning states, there is an overflow because the switch statement's value is of type int but the switch value is greater than INT_MAX. I did a brief sweep of the tree and it seems that all uses of _IOC in switch statement values either are small enough to fit into size int or the value is of size unsigned int.

Quoted 4 lines

I am unsure of the implications of using a smaller _IOC value or converting all ioctls to expect a cmd of type unsigned int (especially since that has userspace implications) but I didn't see any negative ioctl commands. Some clarity and insight would be appreciated.

Have you verified how gcc compiles these switch statements? Maybe gcc supports switch / case statements on integral types that are larger than an int?

Bart.

Nathan Chancellor

author

2018-10-04 18:45 UTC #2 lore raw

Quoted 33 lines · Bart Van Assche

Quoted 27 lines · Nathan Chancellor
Hi SCSI folks,
In an effort to get the kernel building warning free with Clang, we've come across an interesting occurrence in a few scsi drivers:
drivers/scsi/hpsa.c:6533:7: warning: overflow converting case value to switch condition type (2148024833 to 18446744071562609153) [-Wswitch]
        case CCISS_GETPCIINFO:
             ^
./include/uapi/linux/cciss_ioctl.h:65:26: note: expanded from macro 'CCISS_GETPCIINFO'
#define CCISS_GETPCIINFO _IOR(CCISS_IOC_MAGIC, 1, cciss_pci_info_struct)
                         ^
./include/uapi/asm-generic/ioctl.h:86:28: note: expanded from macro '_IOR'
#define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
                                ^
./include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
        (((dir)  << _IOC_DIRSHIFT) | \
        ^
I see this warning in drivers/scsi/hpsa.c and drivers/scsi/smartpqi/smartpqi_init.c on an arm64 allyesconfig build and it has also been reported in a couple of files in drivers/scsi/cxlflash.
As the warning states, there is an overflow because the switch statement's value is of type int but the switch value is greater than INT_MAX. I did a brief sweep of the tree and it seems that all uses of _IOC in switch statement values either are small enough to fit into size int or the value is of size unsigned int.
Quoted 4 lines
I am unsure of the implications of using a smaller _IOC value or converting all ioctls to expect a cmd of type unsigned int (especially since that has userspace implications) but I didn't see any negative ioctl commands. Some clarity and insight would be appreciated.

Quoted 4 lines

Have you verified how gcc compiles these switch statements? Maybe gcc supports switch / case statements on integral types that are larger than an int?
Bart.

Hi Bart,

That is entirely possible, I will do some research. I did build with GCC to see if there was any warning and there isn't so I'll be curious to see what is happening at a lower level.

Thanks for the comment!
Nathan

Nick Desaulniers

2018-10-04 21:16 UTC #3 lore raw

Quoted 30 lines · Nathan Chancellor

Quoted 28 lines · Bart Van Assche
Quoted 27 lines · Nathan Chancellor
Hi SCSI folks,
In an effort to get the kernel building warning free with Clang, we've come across an interesting occurrence in a few scsi drivers:
drivers/scsi/hpsa.c:6533:7: warning: overflow converting case value to switch condition type (2148024833 to 18446744071562609153) [-Wswitch]
        case CCISS_GETPCIINFO:
             ^
./include/uapi/linux/cciss_ioctl.h:65:26: note: expanded from macro 'CCISS_GETPCIINFO'
#define CCISS_GETPCIINFO _IOR(CCISS_IOC_MAGIC, 1, cciss_pci_info_struct)
                         ^
./include/uapi/asm-generic/ioctl.h:86:28: note: expanded from macro '_IOR'
#define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
                                ^
./include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
        (((dir)  << _IOC_DIRSHIFT) | \
        ^
I see this warning in drivers/scsi/hpsa.c and drivers/scsi/smartpqi/smartpqi_init.c on an arm64 allyesconfig build and it has also been reported in a couple of files in drivers/scsi/cxlflash.
As the warning states, there is an overflow because the switch statement's value is of type int but the switch value is greater than INT_MAX. I did a brief sweep of the tree and it seems that all uses of _IOC in switch statement values either are small enough to fit into size int or the value is of size unsigned int.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
I am unsure of the implications of using a smaller _IOC value or converting all ioctls to expect a cmd of type unsigned int (especially since that has userspace implications) but I didn't see any negative ioctl commands. Some clarity and insight would be appreciated.
Have you verified how gcc compiles these switch statements? Maybe gcc supports switch / case statements on integral types that are larger than an int?

GCC just doesn't warn when the case expression is greater than the maximal representable value and thus would wrap (or appears to, this is probably undefined behavior). Using an unsigned int here is no functional change:
https://godbolt.org/z/1IyYV4

GCC and Clang do effectively the same thing as each other, and in the cases of switching on an unsigned int vs signed int.

Quoted 11 lines

Quoted 2 lines
Bart.
Hi Bart,
That is entirely possible, I will do some research. I did build with GCC to see if there was any warning and there isn't so I'll be curious to see what is happening at a lower level.
Thanks for the comment!
Nathan

Signature · 2 lines

Thanks,
~Nick Desaulniers

Nathan Chancellor

author

2018-10-05 06:57 UTC #4 lore raw

Quoted 40 lines · Nick Desaulniers

Quoted 30 lines · Nathan Chancellor
Quoted 28 lines · Bart Van Assche
Quoted 27 lines · Nathan Chancellor
Hi SCSI folks,
In an effort to get the kernel building warning free with Clang, we've come across an interesting occurrence in a few scsi drivers:
drivers/scsi/hpsa.c:6533:7: warning: overflow converting case value to switch condition type (2148024833 to 18446744071562609153) [-Wswitch]
        case CCISS_GETPCIINFO:
             ^
./include/uapi/linux/cciss_ioctl.h:65:26: note: expanded from macro 'CCISS_GETPCIINFO'
#define CCISS_GETPCIINFO _IOR(CCISS_IOC_MAGIC, 1, cciss_pci_info_struct)
                         ^
./include/uapi/asm-generic/ioctl.h:86:28: note: expanded from macro '_IOR'
#define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
                                ^
./include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
        (((dir)  << _IOC_DIRSHIFT) | \
        ^
I see this warning in drivers/scsi/hpsa.c and drivers/scsi/smartpqi/smartpqi_init.c on an arm64 allyesconfig build and it has also been reported in a couple of files in drivers/scsi/cxlflash.
As the warning states, there is an overflow because the switch statement's value is of type int but the switch value is greater than INT_MAX. I did a brief sweep of the tree and it seems that all uses of _IOC in switch statement values either are small enough to fit into size int or the value is of size unsigned int.
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
I am unsure of the implications of using a smaller _IOC value or converting all ioctls to expect a cmd of type unsigned int (especially since that has userspace implications) but I didn't see any negative ioctl commands. Some clarity and insight would be appreciated.
Have you verified how gcc compiles these switch statements? Maybe gcc supports switch / case statements on integral types that are larger than an int?

Quoted 9 lines

GCC just doesn't warn when the case expression is greater than the maximal representable value and thus would wrap (or appears to, this is probably undefined behavior). Using an unsigned int here is no functional change:
https://godbolt.org/z/1IyYV4
GCC and Clang do effectively the same thing as each other, and in the cases of switching on an unsigned int vs signed int.

Regardless of how the overflow is handled within the switch statement, the overflow is also happening when passing in these values to the ioctl, right? I mean these case values are defined in the uapi files so that userspace can easily pass them in to the ioctl, meaning those values are being passed in as a signed integer and I would assume subsequently overflowing unless I'm just missing something here.

Nathan

Quoted 17 lines

Quoted 11 lines
Quoted 2 lines
Bart.
Hi Bart,
That is entirely possible, I will do some research. I did build with GCC to see if there was any warning and there isn't so I'll be curious to see what is happening at a lower level.
Thanks for the comment!
Nathan
Signature · 2 lines
Thanks,
~Nick Desaulniers

Nick Desaulniers

2018-10-08 18:12 UTC #5 lore raw

Quoted 52 lines · Nathan Chancellor

Quoted 40 lines · Nick Desaulniers
Quoted 30 lines · Nathan Chancellor
Quoted 28 lines · Bart Van Assche
Quoted 27 lines · Nathan Chancellor
Hi SCSI folks,
In an effort to get the kernel building warning free with Clang, we've come across an interesting occurrence in a few scsi drivers:
drivers/scsi/hpsa.c:6533:7: warning: overflow converting case value to switch condition type (2148024833 to 18446744071562609153) [-Wswitch]
        case CCISS_GETPCIINFO:
             ^
./include/uapi/linux/cciss_ioctl.h:65:26: note: expanded from macro 'CCISS_GETPCIINFO'
#define CCISS_GETPCIINFO _IOR(CCISS_IOC_MAGIC, 1, cciss_pci_info_struct)
                         ^
./include/uapi/asm-generic/ioctl.h:86:28: note: expanded from macro '_IOR'
#define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
                                ^
./include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
        (((dir)  << _IOC_DIRSHIFT) | \
        ^
I see this warning in drivers/scsi/hpsa.c and drivers/scsi/smartpqi/smartpqi_init.c on an arm64 allyesconfig build and it has also been reported in a couple of files in drivers/scsi/cxlflash.
As the warning states, there is an overflow because the switch statement's value is of type int but the switch value is greater than INT_MAX. I did a brief sweep of the tree and it seems that all uses of _IOC in switch statement values either are small enough to fit into size int or the value is of size unsigned int.
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
I am unsure of the implications of using a smaller _IOC value or converting all ioctls to expect a cmd of type unsigned int (especially since that has userspace implications) but I didn't see any negative ioctl commands. Some clarity and insight would be appreciated.
Have you verified how gcc compiles these switch statements? Maybe gcc supports switch / case statements on integral types that are larger than an int?
Quoted 9 lines
GCC just doesn't warn when the case expression is greater than the maximal representable value and thus would wrap (or appears to, this is probably undefined behavior). Using an unsigned int here is no functional change:
https://godbolt.org/z/1IyYV4
GCC and Clang do effectively the same thing as each other, and in the cases of switching on an unsigned int vs signed int.

Quoted 6 lines

Regardless of how the overflow is handled within the switch statement, the overflow is also happening when passing in these values to the ioctl, right? I mean these case values are defined in the uapi files so that userspace can easily pass them in to the ioctl, meaning those values are being passed in as a signed integer and I would assume subsequently overflowing unless I'm just missing something here.

The kernel compiles its code with -fno-strict-overflow, which sets -fwrapv, which makes signed integer overflow defined to wrap (the default by the C standard is that signed integer overflow is undefined behavior). So while the kernel might be safe from undefined behavior, the wrapped value being provided to userspace might be a problem. If userspace tries to use these definitions, I suspect they will likely trigger the overflow at runtime.

Quoted 20 lines

Nathan
Quoted 17 lines
Quoted 11 lines
Quoted 2 lines
Bart.
Hi Bart,
That is entirely possible, I will do some research. I did build with GCC to see if there was any warning and there isn't so I'll be curious to see what is happening at a lower level.
Thanks for the comment!
Nathan
--
Thanks,
~Nick Desaulniers

Signature · 2 lines

Thanks,
~Nick Desaulniers

Bart Van Assche

2018-10-08 18:47 UTC #6 lore raw

In reply to Nathan Chancellor

Quoted 8 lines · Nathan Chancellor

Regardless of how the overflow is handled within the switch statement, the overflow is also happening when passing in these values to the ioctl, right? I mean these case values are defined in the uapi files so that userspace can easily pass them in to the ioctl, meaning those values are being passed in as a signed integer and I would assume subsequently overflowing unless I'm just missing something here.
From the user space header <sys/ioctl.h>

extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;

Quoted 1 line

From the kernel header <linux/fs.h>

	long (*unlocked_ioctl) (struct file * unsigned int, unsigned long);	long (*compat_ioctl) (struct file * unsigned int, unsigned long);

Why has the second argument been declared as "unsigned long"in the glibc headers and as "unsigned int"in the kernel headers? That's not clear to me.

Bart.

Nathan Chancellor

author

2018-10-19 06:51 UTC #7 lore raw

Quoted 14 lines · Bart Van Assche

Quoted 6 lines · Nathan Chancellor
Regardless of how the overflow is handled within the switch statement, the overflow is also happening when passing in these values to the ioctl, right? I mean these case values are defined in the uapi files so that userspace can easily pass them in to the ioctl, meaning those values are being passed in as a signed integer and I would assume subsequently overflowing unless I'm just missing something here.
From the user space header <sys/ioctl.h>:
extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;
From the kernel header <linux/fs.h>:

Quoted 8 lines

	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
Why has the second argument been declared as "unsigned long" in the glibc headers and as "unsigned int" in the kernel headers? That's not clear to me.
Bart.

Hi Bart,

Sorry it took me so long to reply, somehow this email got lost in my inbox...

Unfortuntely, I am unsure why there is that discrepency between the headers. I tried to do some research but I didn't come up with much.

However, I did test changing the type of ioctl/compat_ioctl's cmd parameter to 'unsigned int' and came up with the following diff (rather large so sharing via a gist instead of pasting here):

https://gist.github.com/nathanchance/8febc92735f4228574cb0464520f0f6f

I'll obviously draft up a proper commit message before formally sending but I can address any major concerns before that happens. I checked every single ioctl for a negative value and there aren't any so I think this change makes sense to fix this warning.

Cheers,
Nathan

Bart Van Assche

2018-10-19 13:55 UTC #8 lore raw

Quoted 28 lines · Nathan Chancellor

Quoted 6 lines · Bart Van Assche
From the user space header <sys/ioctl.h>:
extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;
From the kernel header <linux/fs.h>:
Quoted 8 lines
	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
Why has the second argument been declared as "unsigned long" in the glibc headers and as "unsigned int" in the kernel headers? That's not clear to me.
Bart.
Hi Bart,
Sorry it took me so long to reply, somehow this email got lost in my inbox...
Unfortuntely, I am unsure why there is that discrepency between the headers. I tried to do some research but I didn't come up with much.
However, I did test changing the type of ioctl/compat_ioctl's cmd parameter to 'unsigned int' and came up with the following diff (rather large so sharing via a gist instead of pasting here):

Quoted 6 lines

https://gist.github.com/nathanchance/8febc92735f4228574cb0464520f0f6f
I'll obviously draft up a proper commit message before formally sending but I can address any major concerns before that happens. I checked every single ioctl for a negative value and there aren't any so I think this change makes sense to fix this warning.

Hi Nathan,

Since that change makes the SCSI code more consistent with the rest of the Linux kernel, I'm in favor of making that change.

Bart.