Re: [PATCH] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Jul 9, 2025, at 20:27, Darrick J. Wong wrote:
> On Wed, Jul 09, 2025 at 08:10:14PM +0200, Arnd Bergmann wrote:

> though we probably want a helper or something to encapsulate those three
> comparisons to avoid the SOMETHING_SOMETHING part:
>
> #define IOC_DISPATCH(c) \
> 	((c) & ~(_IOC(0, 0, 0, _IOC_SIZE(_IOC_SIZEMASK))))
>
> 	switch (IOC_DISPATCH(cmd)) {
> 	case IOC_DISPATCH(FS_IOC_FSGETXATTR):
> 		return ioctl_fsgetxattr(filp, cmd, argp);
>
> Assuming that ioctl_fsgetxattr derives size from @cmd and rejects values
> that it doesn't like.  Hrm?

This may work in specific cases, but it adds a lot of complexity
and room for error if we try to do this in more places:

Ignoring the 'size' argument as above would mean that
each case now has to add an extra size check in each 'case',
which then defeats the entire purpose.

I should maybe dig out my notes for table-driver ioctl
handlers, if we want to improve the way that drivers define
their ioctl implementations, I'm sure there is some
infrastructure we can come up with that can help here,
but I don't think 'same as before but more macros' is the
answer.

joydev_ioctl_common() is an existing example doing something
like it and gets it right, while snd_compr_ioctl() is an
example that looks completely broken to me.

>> +	    _IOC_SIZE(cmd) >= LBMD_SIZE_VER0 &&
>> +	    _IOC_SIZE(cmd) <= _IOC_SIZE(FS_IOC_GETLBMD_CAP))
>
> blk_get_meta_cap already checks this.

I had thought about removing it there, but decided against that.
Maybe a better way would be to have the full check inside of 
blk_get_meta_cap() and use the -ENOIOCTLCMD return code
to keep the caller simple:

   switch(cmd) {
   ...
   default:
         break;
   }
   ret = blk_get_meta_cap(bdev, cmd, argp);
   if (ret != -ENOIOCTLCMD)
        return ret;
   ...
   return -ENOIOCTLCMD;

       Arnd