linux-kernel - Re: PROBLEM: BLKPG_DEL_PARTITION with GENHD_FL_NO_PART used to return ENXIO, now returns EINVAL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <592625f7-36d7-02e0-2ee6-8211334aa0f9@huaweicloud.com>
Date: Tue, 16 Jan 2024 21:46:21 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Yu Kuai <yukuai1@...weicloud.com>, Christoph Hellwig <hch@...radead.org>,
 Allison Karlitskaya <allison.karlitskaya@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
 Jens Axboe <axboe@...nel.dk>, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: PROBLEM: BLKPG_DEL_PARTITION with GENHD_FL_NO_PART used to return
 ENXIO, now returns EINVAL

Hi,

在 2024/01/16 21:23, Yu Kuai 写道:
> Hi, Christoph
> 
> 在 2024/01/16 18:47, Christoph Hellwig 写道:
>> Hi Allison,
>>
>> please try this minimal fix.  I need to double check if we historically
>> returned ENXIO or EINVAL for adding / resizing partitions, which would
>> make things more complicated.  Or maybe you already have data for that
>> at hand?
>>
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index 9c73a763ef8838..f2028e39767821 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -21,7 +21,7 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>>       sector_t start, length;
>>       if (disk->flags & GENHD_FL_NO_PART)
>> -        return -EINVAL;
>> +        return -ENXIO;
> 
> I think this might not be a proper fix, the reason if that before this
> condition is added, -ENXIO is returned from bdev_del_partition(). And
> there are also some other error number like -EACCES,-EFAULT following,
> so this change will still make changes for user in other cases.

Please ignore the patch from last email. Sorry for the noise...
bdev_resize_partition() will also return -ENXIO if partition does't
exist. So the right patch should be following:

diff --git a/block/ioctl.c b/block/ioctl.c
index 4160f4e6bd5b..ba8d44fa7e02 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -20,8 +20,6 @@ static int blkpg_do_ioctl(struct block_device *bdev,
         struct blkpg_partition p;
         long long start, length;

-       if (disk->flags & GENHD_FL_NO_PART)
-               return -EINVAL;
         if (!capable(CAP_SYS_ADMIN))
                 return -EACCES;
         if (copy_from_user(&p, upart, sizeof(struct blkpg_partition)))
diff --git a/block/partitions/core.c b/block/partitions/core.c
index f47ffcfdfcec..f14602022c5e 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -447,6 +447,11 @@ int bdev_add_partition(struct gendisk *disk, int 
partno, sector_t start,
                 goto out;
         }

+       if (disk->flags & GENHD_FL_NO_PART) {
+               ret = -EINVAL;
+               goto out;
+       }
+
         if (partition_overlaps(disk, start, length, -1)) {
                 ret = -EBUSY;
                 goto out;

Thanks,
Kuai
> 
> How about following patch?
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 4160f4e6bd5b..ec012cf910dc 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -20,8 +20,6 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>          struct blkpg_partition p;
>          long long start, length;
> 
> -       if (disk->flags & GENHD_FL_NO_PART)
> -               return -EINVAL;
>          if (!capable(CAP_SYS_ADMIN))
>                  return -EACCES;
>          if (copy_from_user(&p, upart, sizeof(struct blkpg_partition)))
> @@ -38,6 +36,9 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>          start = p.start >> SECTOR_SHIFT;
>          length = p.length >> SECTOR_SHIFT;
> 
> +       if (disk->flags & GENHD_FL_NO_PART)
> +               return -EINVAL;
> +
>          switch (op) {
>          case BLKPG_ADD_PARTITION:
>                  /* check if partition is aligned to blocksize */
> 
> Thanks,
> Kuai
> 
> 
>>       if (!capable(CAP_SYS_ADMIN))
>>           return -EACCES;
>>       if (copy_from_user(&p, upart, sizeof(struct blkpg_partition)))
>>
>> .
>>
> 
> .
>