* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-11 8:46 ` [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl() Arnd Bergmann
@ 2025-07-11 9:40 ` Anders Roxell
2025-07-11 10:04 ` Christian Brauner
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Anders Roxell @ 2025-07-11 9:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Anuj Gupta, Martin K. Petersen, Kanchan Joshi, Christian Brauner,
Christoph Hellwig, Arnd Bergmann, Naresh Kamboju, Jens Axboe,
Keith Busch, Caleb Sander Mateos, Pavel Begunkov, Alexey Dobriyan,
Darrick J. Wong, linux-block, linux-kernel
On Fri, 11 Jul 2025 at 10:47, Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> Anders and Naresh found that the addition of the FS_IOC_GETLBMD_CAP
> handling in the blockdev ioctl handler breaks all ioctls with
> _IOC_NR==2, as the new command is not added to the switch but only
> a few of the command bits are check.
>
> Move the check into the blk_get_meta_cap() function itself and make
> it return -ENOIOCTLCMD for any unsupported command code, including
> those with a smaller size that previously returned -EINVAL.
>
> For consistency this also drops the check for NULL 'arg' that
> is really useless, as any invalid pointer should return -EFAULT.
>
> Fixes: 9eb22f7fedfc ("fs: add ioctl to query metadata and protection info capabilities")
> Link: https://lore.kernel.org/all/CA+G9fYvk9HHE5UJ7cdJHTcY6P5JKnp+_e+sdC5U-ZQFTP9_hqQ@mail.gmail.com/
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Anders Roxell <anders.roxell@linaro.org>
Tested it in qemu-aarch64, it now ran the ltp-smoke test fine.
Cheers,
Anders
> ---
> v2: add the check in blk-integrity.c instead of ioctl.c
>
> I've left out the maximum-size check this time, as there was no
> consensus on whether there should be one, or what value.
>
> We still need to come up with a better way of handling these in
> general, for now the patch just addresses the immediate regression
> that Naresh found.
>
> I have also sent a handful of patches for other drivers that have
> variations of the same bug.
> ---
> block/blk-integrity.c | 10 ++++++----
> block/ioctl.c | 6 ++++--
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> index 9d9dc9c32083..61a79e19c78f 100644
> --- a/block/blk-integrity.c
> +++ b/block/blk-integrity.c
> @@ -62,10 +62,12 @@ int blk_get_meta_cap(struct block_device *bdev, unsigned int cmd,
> struct logical_block_metadata_cap meta_cap = {};
> size_t usize = _IOC_SIZE(cmd);
>
> - if (!argp)
> - return -EINVAL;
> - if (usize < LBMD_SIZE_VER0)
> - return -EINVAL;
> + if (_IOC_DIR(cmd) != _IOC_DIR(FS_IOC_GETLBMD_CAP) ||
> + _IOC_TYPE(cmd) != _IOC_TYPE(FS_IOC_GETLBMD_CAP) ||
> + _IOC_NR(cmd) != _IOC_NR(FS_IOC_GETLBMD_CAP) ||
> + _IOC_SIZE(cmd) < LBMD_SIZE_VER0)
> + return -ENOIOCTLCMD;
> +
> if (!bi)
> goto out;
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 9ad403733e19..af2e22e5533c 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -566,9 +566,11 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
> void __user *argp)
> {
> unsigned int max_sectors;
> + int ret;
>
> - if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
> - return blk_get_meta_cap(bdev, cmd, argp);
> + ret = blk_get_meta_cap(bdev, cmd, argp);
> + if (ret != -ENOIOCTLCMD)
> + return ret;
>
> switch (cmd) {
> case BLKFLSBUF:
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-11 8:46 ` [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl() Arnd Bergmann
2025-07-11 9:40 ` Anders Roxell
@ 2025-07-11 10:04 ` Christian Brauner
2025-07-11 14:47 ` Jens Axboe
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2025-07-11 10:04 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Anuj Gupta, Martin K. Petersen, Kanchan Joshi, Christoph Hellwig,
Arnd Bergmann, Naresh Kamboju, Anders Roxell, Jens Axboe,
Keith Busch, Caleb Sander Mateos, Pavel Begunkov, Alexey Dobriyan,
Darrick J. Wong, linux-block, linux-kernel
On Fri, Jul 11, 2025 at 10:46:51AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Anders and Naresh found that the addition of the FS_IOC_GETLBMD_CAP
> handling in the blockdev ioctl handler breaks all ioctls with
> _IOC_NR==2, as the new command is not added to the switch but only
> a few of the command bits are check.
>
> Move the check into the blk_get_meta_cap() function itself and make
> it return -ENOIOCTLCMD for any unsupported command code, including
> those with a smaller size that previously returned -EINVAL.
>
> For consistency this also drops the check for NULL 'arg' that
> is really useless, as any invalid pointer should return -EFAULT.
>
> Fixes: 9eb22f7fedfc ("fs: add ioctl to query metadata and protection info capabilities")
> Link: https://lore.kernel.org/all/CA+G9fYvk9HHE5UJ7cdJHTcY6P5JKnp+_e+sdC5U-ZQFTP9_hqQ@mail.gmail.com/
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: add the check in blk-integrity.c instead of ioctl.c
>
> I've left out the maximum-size check this time, as there was no
> consensus on whether there should be one, or what value.
>
> We still need to come up with a better way of handling these in
> general, for now the patch just addresses the immediate regression
> that Naresh found.
>
> I have also sent a handful of patches for other drivers that have
> variations of the same bug.
Arnd, let me know how I can help!
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-11 8:46 ` [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl() Arnd Bergmann
2025-07-11 9:40 ` Anders Roxell
2025-07-11 10:04 ` Christian Brauner
@ 2025-07-11 14:47 ` Jens Axboe
2025-07-17 23:37 ` Klara Modin
2025-07-28 13:39 ` Marek Szyprowski
4 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2025-07-11 14:47 UTC (permalink / raw)
To: Arnd Bergmann, Anuj Gupta, Martin K. Petersen, Kanchan Joshi,
Christian Brauner
Cc: Christoph Hellwig, Arnd Bergmann, Naresh Kamboju, Anders Roxell,
Keith Busch, Caleb Sander Mateos, Pavel Begunkov, Alexey Dobriyan,
Darrick J. Wong, linux-block, linux-kernel
On 7/11/25 2:46 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Anders and Naresh found that the addition of the FS_IOC_GETLBMD_CAP
> handling in the blockdev ioctl handler breaks all ioctls with
> _IOC_NR==2, as the new command is not added to the switch but only
> a few of the command bits are check.
>
> Move the check into the blk_get_meta_cap() function itself and make
> it return -ENOIOCTLCMD for any unsupported command code, including
> those with a smaller size that previously returned -EINVAL.
>
> For consistency this also drops the check for NULL 'arg' that
> is really useless, as any invalid pointer should return -EFAULT.
>
> Fixes: 9eb22f7fedfc ("fs: add ioctl to query metadata and protection info capabilities")
Since this isn't from my tree:
Reviewed-by: Jens Axboe <axboe@kernel.dk>
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-11 8:46 ` [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl() Arnd Bergmann
` (2 preceding siblings ...)
2025-07-11 14:47 ` Jens Axboe
@ 2025-07-17 23:37 ` Klara Modin
2025-07-18 5:56 ` Arnd Bergmann
2025-07-28 13:39 ` Marek Szyprowski
4 siblings, 1 reply; 9+ messages in thread
From: Klara Modin @ 2025-07-17 23:37 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Anuj Gupta, Martin K. Petersen, Kanchan Joshi, Christian Brauner,
Christoph Hellwig, Arnd Bergmann, Naresh Kamboju, Anders Roxell,
Jens Axboe, Keith Busch, Caleb Sander Mateos, Pavel Begunkov,
Alexey Dobriyan, Darrick J. Wong, linux-block, linux-kernel
Hi,
On 2025-07-11 10:46:51 +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Anders and Naresh found that the addition of the FS_IOC_GETLBMD_CAP
> handling in the blockdev ioctl handler breaks all ioctls with
> _IOC_NR==2, as the new command is not added to the switch but only
> a few of the command bits are check.
>
> Move the check into the blk_get_meta_cap() function itself and make
> it return -ENOIOCTLCMD for any unsupported command code, including
> those with a smaller size that previously returned -EINVAL.
>
> For consistency this also drops the check for NULL 'arg' that
> is really useless, as any invalid pointer should return -EFAULT.
>
> Fixes: 9eb22f7fedfc ("fs: add ioctl to query metadata and protection info capabilities")
> Link: https://lore.kernel.org/all/CA+G9fYvk9HHE5UJ7cdJHTcY6P5JKnp+_e+sdC5U-ZQFTP9_hqQ@mail.gmail.com/
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: add the check in blk-integrity.c instead of ioctl.c
>
> I've left out the maximum-size check this time, as there was no
> consensus on whether there should be one, or what value.
>
> We still need to come up with a better way of handling these in
> general, for now the patch just addresses the immediate regression
> that Naresh found.
>
> I have also sent a handful of patches for other drivers that have
> variations of the same bug.
> ---
> block/blk-integrity.c | 10 ++++++----
> block/ioctl.c | 6 ++++--
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> index 9d9dc9c32083..61a79e19c78f 100644
> --- a/block/blk-integrity.c
> +++ b/block/blk-integrity.c
> @@ -62,10 +62,12 @@ int blk_get_meta_cap(struct block_device *bdev, unsigned int cmd,
> struct logical_block_metadata_cap meta_cap = {};
> size_t usize = _IOC_SIZE(cmd);
>
> - if (!argp)
> - return -EINVAL;
> - if (usize < LBMD_SIZE_VER0)
> - return -EINVAL;
> + if (_IOC_DIR(cmd) != _IOC_DIR(FS_IOC_GETLBMD_CAP) ||
> + _IOC_TYPE(cmd) != _IOC_TYPE(FS_IOC_GETLBMD_CAP) ||
> + _IOC_NR(cmd) != _IOC_NR(FS_IOC_GETLBMD_CAP) ||
> + _IOC_SIZE(cmd) < LBMD_SIZE_VER0)
> + return -ENOIOCTLCMD;
> +
> if (!bi)
> goto out;
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 9ad403733e19..af2e22e5533c 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -566,9 +566,11 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
> void __user *argp)
> {
> unsigned int max_sectors;
> + int ret;
>
> - if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
> - return blk_get_meta_cap(bdev, cmd, argp);
> + ret = blk_get_meta_cap(bdev, cmd, argp);
> + if (ret != -ENOIOCTLCMD)
> + return ret;
This check seems to be incomplete. In the case when BLK_DEV_INTEGRITY is
disabled the ioctl can never complete as blk_get_meta_cap will then
always return -EOPNOTSUPP. Or should the !BLK_DEV_INTEGRITY stub be
changed to return -ENOIOCTLCMD instead?
It makes e.g. cryptsetup fail in my initramfs. Adding -EOPNOTSUPP to the
check fixes it for me:
diff --git a/block/ioctl.c b/block/ioctl.c
index af2e22e5533c..7d5361fd1b7d 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -569,7 +569,7 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
int ret;
ret = blk_get_meta_cap(bdev, cmd, argp);
- if (ret != -ENOIOCTLCMD)
+ if (ret != -EOPNOTSUPP && ret != -ENOIOCTLCMD)
return ret;
switch (cmd) {
Regards,
Klara Modin
>
> switch (cmd) {
> case BLKFLSBUF:
> --
> 2.39.5
>
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-17 23:37 ` Klara Modin
@ 2025-07-18 5:56 ` Arnd Bergmann
2025-07-24 10:16 ` Klara Modin
0 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2025-07-18 5:56 UTC (permalink / raw)
To: Klara Modin, Arnd Bergmann
Cc: Anuj Gupta, Martin K. Petersen, Kanchan Joshi, Christian Brauner,
Christoph Hellwig, Naresh Kamboju, Anders Roxell, Jens Axboe,
Keith Busch, Caleb Sander Mateos, Pavel Begunkov, Alexey Dobriyan,
Darrick J. Wong, linux-block, linux-kernel
On Fri, Jul 18, 2025, at 01:37, Klara Modin wrote:
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index 9ad403733e19..af2e22e5533c 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -566,9 +566,11 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
>> void __user *argp)
>> {
>> unsigned int max_sectors;
>> + int ret;
>>
>> - if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
>> - return blk_get_meta_cap(bdev, cmd, argp);
>
>> + ret = blk_get_meta_cap(bdev, cmd, argp);
>> + if (ret != -ENOIOCTLCMD)
>> + return ret;
>
> This check seems to be incomplete. In the case when BLK_DEV_INTEGRITY is
> disabled the ioctl can never complete as blk_get_meta_cap will then
> always return -EOPNOTSUPP. Or should the !BLK_DEV_INTEGRITY stub be
> changed to return -ENOIOCTLCMD instead?
Ah, I did miss the stub.
> It makes e.g. cryptsetup fail in my initramfs. Adding -EOPNOTSUPP to the
> check fixes it for me:
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index af2e22e5533c..7d5361fd1b7d 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -569,7 +569,7 @@ static int blkdev_common_ioctl(struct block_device
> *bdev, blk_mode_t mode,
> int ret;
>
> ret = blk_get_meta_cap(bdev, cmd, argp);
> - if (ret != -ENOIOCTLCMD)
> + if (ret != -EOPNOTSUPP && ret != -ENOIOCTLCMD)
> return ret;
>
> switch (cmd) {
I think returning -ENOIOCTLCMD from the stub makes more sense,
but I don't know what the motivation for the -EOPNOTSUPP was.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-18 5:56 ` Arnd Bergmann
@ 2025-07-24 10:16 ` Klara Modin
2025-07-25 4:36 ` Anuj gupta
0 siblings, 1 reply; 9+ messages in thread
From: Klara Modin @ 2025-07-24 10:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Arnd Bergmann, Anuj Gupta, Martin K. Petersen, Kanchan Joshi,
Christian Brauner, Christoph Hellwig, Naresh Kamboju,
Anders Roxell, Jens Axboe, Keith Busch, Caleb Sander Mateos,
Pavel Begunkov, Alexey Dobriyan, Darrick J. Wong, linux-block,
linux-kernel
On 2025-07-18 07:56:49 +0200, Arnd Bergmann wrote:
> On Fri, Jul 18, 2025, at 01:37, Klara Modin wrote:
>
> >> diff --git a/block/ioctl.c b/block/ioctl.c
> >> index 9ad403733e19..af2e22e5533c 100644
> >> --- a/block/ioctl.c
> >> +++ b/block/ioctl.c
> >> @@ -566,9 +566,11 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
> >> void __user *argp)
> >> {
> >> unsigned int max_sectors;
> >> + int ret;
> >>
> >> - if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
> >> - return blk_get_meta_cap(bdev, cmd, argp);
> >
> >> + ret = blk_get_meta_cap(bdev, cmd, argp);
> >> + if (ret != -ENOIOCTLCMD)
> >> + return ret;
> >
> > This check seems to be incomplete. In the case when BLK_DEV_INTEGRITY is
> > disabled the ioctl can never complete as blk_get_meta_cap will then
> > always return -EOPNOTSUPP. Or should the !BLK_DEV_INTEGRITY stub be
> > changed to return -ENOIOCTLCMD instead?
>
> Ah, I did miss the stub.
>
> > It makes e.g. cryptsetup fail in my initramfs. Adding -EOPNOTSUPP to the
> > check fixes it for me:
> >
> > diff --git a/block/ioctl.c b/block/ioctl.c
> > index af2e22e5533c..7d5361fd1b7d 100644
> > --- a/block/ioctl.c
> > +++ b/block/ioctl.c
> > @@ -569,7 +569,7 @@ static int blkdev_common_ioctl(struct block_device
> > *bdev, blk_mode_t mode,
> > int ret;
> >
> > ret = blk_get_meta_cap(bdev, cmd, argp);
> > - if (ret != -ENOIOCTLCMD)
> > + if (ret != -EOPNOTSUPP && ret != -ENOIOCTLCMD)
> > return ret;
> >
> > switch (cmd) {
>
> I think returning -ENOIOCTLCMD from the stub makes more sense,
> but I don't know what the motivation for the -EOPNOTSUPP was.
>
> Arnd
Should I send a patch changing the stub? At least from reading
Documentation/driver-api/ioctl.rst it seems clear that only -ENOIOCTLCMD
or -ENOTTY is correct when the command number is unknown.
I didn't find any particular reason in 9eb22f7fedfc ("fs: add ioctl to
query metadata and protection info capabilities") for the -EOPNOTSUPP
return.
Regards,
Klara Modin
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-24 10:16 ` Klara Modin
@ 2025-07-25 4:36 ` Anuj gupta
0 siblings, 0 replies; 9+ messages in thread
From: Anuj gupta @ 2025-07-25 4:36 UTC (permalink / raw)
To: Klara Modin
Cc: Arnd Bergmann, Arnd Bergmann, Anuj Gupta, Martin K. Petersen,
Kanchan Joshi, Christian Brauner, Christoph Hellwig,
Naresh Kamboju, Anders Roxell, Jens Axboe, Keith Busch,
Caleb Sander Mateos, Pavel Begunkov, Alexey Dobriyan,
Darrick J. Wong, linux-block, linux-kernel
On Thu, Jul 24, 2025 at 3:46 PM Klara Modin <klarasmodin@gmail.com> wrote:
>
> On 2025-07-18 07:56:49 +0200, Arnd Bergmann wrote:
> > On Fri, Jul 18, 2025, at 01:37, Klara Modin wrote:
> >
> > >> diff --git a/block/ioctl.c b/block/ioctl.c
> > >> index 9ad403733e19..af2e22e5533c 100644
> > >> --- a/block/ioctl.c
> > >> +++ b/block/ioctl.c
> > >> @@ -566,9 +566,11 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
> > >> void __user *argp)
> > >> {
> > >> unsigned int max_sectors;
> > >> + int ret;
> > >>
> > >> - if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
> > >> - return blk_get_meta_cap(bdev, cmd, argp);
> > >
> > >> + ret = blk_get_meta_cap(bdev, cmd, argp);
> > >> + if (ret != -ENOIOCTLCMD)
> > >> + return ret;
> > >
> > > This check seems to be incomplete. In the case when BLK_DEV_INTEGRITY is
> > > disabled the ioctl can never complete as blk_get_meta_cap will then
> > > always return -EOPNOTSUPP. Or should the !BLK_DEV_INTEGRITY stub be
> > > changed to return -ENOIOCTLCMD instead?
> >
> > Ah, I did miss the stub.
> >
> > > It makes e.g. cryptsetup fail in my initramfs. Adding -EOPNOTSUPP to the
> > > check fixes it for me:
> > >
> > > diff --git a/block/ioctl.c b/block/ioctl.c
> > > index af2e22e5533c..7d5361fd1b7d 100644
> > > --- a/block/ioctl.c
> > > +++ b/block/ioctl.c
> > > @@ -569,7 +569,7 @@ static int blkdev_common_ioctl(struct block_device
> > > *bdev, blk_mode_t mode,
> > > int ret;
> > >
> > > ret = blk_get_meta_cap(bdev, cmd, argp);
> > > - if (ret != -ENOIOCTLCMD)
> > > + if (ret != -EOPNOTSUPP && ret != -ENOIOCTLCMD)
> > > return ret;
> > >
> > > switch (cmd) {
> >
> > I think returning -ENOIOCTLCMD from the stub makes more sense,
> > but I don't know what the motivation for the -EOPNOTSUPP was.
> >
> > Arnd
>
> Should I send a patch changing the stub? At least from reading
> Documentation/driver-api/ioctl.rst it seems clear that only -ENOIOCTLCMD
> or -ENOTTY is correct when the command number is unknown.
>
> I didn't find any particular reason in 9eb22f7fedfc ("fs: add ioctl to
> query metadata and protection info capabilities") for the -EOPNOTSUPP
> return.
Hi Klara,
Thanks for pointing this out — I had originally used -EOPNOTSUPP
because the ioctl command is recognized, but the operation isn’t
supported when CONFIG_BLK_DEV_INTEGRITY=n.
That said, I agree that returning -ENOIOCTLCMD from the stub might be
more appropriate in this context.
Thanks,
Anuj
>
> Regards,
> Klara Modin
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()
2025-07-11 8:46 ` [PATCH] [v2] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl() Arnd Bergmann
` (3 preceding siblings ...)
2025-07-17 23:37 ` Klara Modin
@ 2025-07-28 13:39 ` Marek Szyprowski
4 siblings, 0 replies; 9+ messages in thread
From: Marek Szyprowski @ 2025-07-28 13:39 UTC (permalink / raw)
To: Arnd Bergmann, Anuj Gupta, Martin K. Petersen, Kanchan Joshi,
Christian Brauner
Cc: Christoph Hellwig, Arnd Bergmann, Naresh Kamboju, Anders Roxell,
Jens Axboe, Keith Busch, Caleb Sander Mateos, Pavel Begunkov,
Alexey Dobriyan, Darrick J. Wong, linux-block, linux-kernel
On 11.07.2025 10:46, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Anders and Naresh found that the addition of the FS_IOC_GETLBMD_CAP
> handling in the blockdev ioctl handler breaks all ioctls with
> _IOC_NR==2, as the new command is not added to the switch but only
> a few of the command bits are check.
>
> Move the check into the blk_get_meta_cap() function itself and make
> it return -ENOIOCTLCMD for any unsupported command code, including
> those with a smaller size that previously returned -EINVAL.
>
> For consistency this also drops the check for NULL 'arg' that
> is really useless, as any invalid pointer should return -EFAULT.
>
> Fixes: 9eb22f7fedfc ("fs: add ioctl to query metadata and protection info capabilities")
> Link: https://lore.kernel.org/all/CA+G9fYvk9HHE5UJ7cdJHTcY6P5JKnp+_e+sdC5U-ZQFTP9_hqQ@mail.gmail.com/
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: add the check in blk-integrity.c instead of ioctl.c
>
> I've left out the maximum-size check this time, as there was no
> consensus on whether there should be one, or what value.
>
> We still need to come up with a better way of handling these in
> general, for now the patch just addresses the immediate regression
> that Naresh found.
>
> I have also sent a handful of patches for other drivers that have
> variations of the same bug.
> ---
In my tests I've found that this patch, merged as commit 42b0ef01e6b5
("block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common_ioctl()"),
breaks udev operation on some of my test boards - no /dev/disk/* entries
and directories are created. Reverting $subject on top of next-20250728
fixes/hides this problem. I suspect that another corner case is missing
in the checks. I will try to investigate this a bit more later, probably
tomorrow.
> block/blk-integrity.c | 10 ++++++----
> block/ioctl.c | 6 ++++--
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> index 9d9dc9c32083..61a79e19c78f 100644
> --- a/block/blk-integrity.c
> +++ b/block/blk-integrity.c
> @@ -62,10 +62,12 @@ int blk_get_meta_cap(struct block_device *bdev, unsigned int cmd,
> struct logical_block_metadata_cap meta_cap = {};
> size_t usize = _IOC_SIZE(cmd);
>
> - if (!argp)
> - return -EINVAL;
> - if (usize < LBMD_SIZE_VER0)
> - return -EINVAL;
> + if (_IOC_DIR(cmd) != _IOC_DIR(FS_IOC_GETLBMD_CAP) ||
> + _IOC_TYPE(cmd) != _IOC_TYPE(FS_IOC_GETLBMD_CAP) ||
> + _IOC_NR(cmd) != _IOC_NR(FS_IOC_GETLBMD_CAP) ||
> + _IOC_SIZE(cmd) < LBMD_SIZE_VER0)
> + return -ENOIOCTLCMD;
> +
> if (!bi)
> goto out;
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 9ad403733e19..af2e22e5533c 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -566,9 +566,11 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
> void __user *argp)
> {
> unsigned int max_sectors;
> + int ret;
>
> - if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
> - return blk_get_meta_cap(bdev, cmd, argp);
> + ret = blk_get_meta_cap(bdev, cmd, argp);
> + if (ret != -ENOIOCTLCMD)
> + return ret;
>
> switch (cmd) {
> case BLKFLSBUF:
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 9+ messages in thread