-Wswitch Clang warnings in drivers/scsi

-Wswitch Clang warnings in drivers/scsi

[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.

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

Re: -Wswitch Clang warnings in drivers/scsi

[Bart Van Assche]

On Thu, 2018-10-04 at 11:30 -0700, Nathan Chancellor wrote:
> 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.

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.

Re: -Wswitch Clang warnings in drivers/scsi

[Nathan Chancellor]

On Thu, Oct 04, 2018 at 11:34:29AM -0700, Bart Van Assche wrote:
> On Thu, 2018-10-04 at 11:30 -0700, Nathan Chancellor wrote:
> > 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.
> 
> 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

Re: -Wswitch Clang warnings in drivers/scsi

[Nick Desaulniers]

On Thu, Oct 4, 2018 at 11:45 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Oct 04, 2018 at 11:34:29AM -0700, Bart Van Assche wrote:
> > On Thu, 2018-10-04 at 11:30 -0700, Nathan Chancellor wrote:
> > > 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.
> >
> > 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.

> >
> > 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

Re: -Wswitch Clang warnings in drivers/scsi

[Nathan Chancellor]

On Thu, Oct 04, 2018 at 02:16:49PM -0700, Nick Desaulniers wrote:
> On Thu, Oct 4, 2018 at 11:45 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Thu, Oct 04, 2018 at 11:34:29AM -0700, Bart Van Assche wrote:
> > > On Thu, 2018-10-04 at 11:30 -0700, Nathan Chancellor wrote:
> > > > 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.
> > >
> > > 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.
> 

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

> > >
> > > 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

Re: -Wswitch Clang warnings in drivers/scsi

[Nick Desaulniers]

On Thu, Oct 4, 2018 at 11:57 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Oct 04, 2018 at 02:16:49PM -0700, Nick Desaulniers wrote:
> > On Thu, Oct 4, 2018 at 11:45 AM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > On Thu, Oct 04, 2018 at 11:34:29AM -0700, Bart Van Assche wrote:
> > > > On Thu, 2018-10-04 at 11:30 -0700, Nathan Chancellor wrote:
> > > > > 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.
> > > >
> > > > 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.
> >
>
> 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.

>
> Nathan
>
> > > >
> > > > 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



-- 
Thanks,
~Nick Desaulniers

Re: -Wswitch Clang warnings in drivers/scsi

[Bart Van Assche]

On Thu, 2018-10-04 at 23:57 -0700, Nathan Chancellor wrote:
> 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>:

	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.

Re: -Wswitch Clang warnings in drivers/scsi

[Nathan Chancellor]

On Mon, Oct 08, 2018 at 11:47:09AM -0700, Bart Van Assche wrote:
> On Thu, 2018-10-04 at 23:57 -0700, Nathan Chancellor wrote:
> > 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>:
> 
> 	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

Re: -Wswitch Clang warnings in drivers/scsi

[Bart Van Assche]

On 10/18/18 11:51 PM, Nathan Chancellor wrote:
> On Mon, Oct 08, 2018 at 11:47:09AM -0700, Bart Van Assche wrote:
>>  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>:
>>
>> 	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.

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.