hi,
[1.] One line summary of the problem:
BLKPG_DEL_PARTITION on an empty loopback device used to return ENXIO
but now returns EINVAL, breaking partprobe
[2.] Full description of the problem/report:
We recently caught this problem in our CI for Cockpit:
https://github.com/cockpit-project/bots/pull/5793
The summary is that if you do something like this:
$ dd if=/dev/zero of=/tmp/foo bs=1M count=50
$ partprobe $(losetup --find --show /tmp/foo)
Then this will fail with the following error message:
Error: Partition(s) 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32,
33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64 on
/dev/loop2 have been written, but we have been unable to inform the
kernel of the change, probably because it/they are in use. As a
result, the old partition(s) will remain in use. You should reboot
now before making further changes.
.. when it used to be successful. That's down to this syscall
(called by partprobe) changing its behaviour between kernel versions:
-ioctl(3, BLKPG, {op=BLKPG_DEL_PARTITION, flags=0, datalen=152,
data={start=0, length=0, pno=1, devname="", volname=""}}) = -1 ENXIO
(No such device or address)
+ioctl(3, BLKPG, {op=BLKPG_DEL_PARTITION, flags=0, datalen=152,
data={start=0, length=0, pno=1, devname="", volname=""}}) = -1 EINVAL
(Invalid argument)
This is observed on Ubuntu jammy with partprobe from parted
3.4-2build1. I've confirmed that the original parted-3.4 download
from https://ftp.gnu.org/gnu/parted/ is also impacted in the same way.
[3.] Keywords:
block, partition, BLKPG_DEL_PARTITION, loop device, EINVAL, ENXIO
[4.] Kernel information:
Linux ubuntu 5.15.0-94-generic #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC
2024 x86_64 x86_64 x86_64 GNU/Linux
This is the version currently in jammy-proposed. The likely culprit
is this commit:
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/jammy/commit/?id=49a502554e8aa853a0357f287121d4cdf4442a58
which is also upstream as 1a721de8489fa559ff4471f73c58bb74ac5580d3.
There has been discussion on linux-kernel before about this:
https://marc.info/?l=linux-kernel&m=169753467305218&w=2
but now we have a pretty clear case of "breaks userspace in the wild".
[4.1.] Kernel version (from /proc/version):
Linux version 5.15.0-94-generic (buildd@lcy02-amd64-096) (gcc (Ubuntu
11.4.0-1ubuntu1~22.04) 11.4.0, GNU ld (GNU Binutils for Ubuntu) 2.38)
#104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024
[4.2.] Kernel .config file:
I pasted a copy here:
https://paste.centos.org/view/8d6506bc
but it won't be around for more than 24 hours. It's just the config
file present in /boot on the affected install.
[5.] Most recent kernel version which did not have the bug:
We last tested 5.15.0-91-generic and found it to be working with the
previous behaviour (ie: returning ENXIO).
[7.] A small shell script or example program which triggers the
problem (if possible)
as above:
$ dd if=/dev/zero of=/tmp/foo bs=1M count=50
$ partprobe $(losetup --find --show /tmp/foo)
[8.] Environment
[8.1.] Software (add the output of the ver_linux script here)
[8.2.] Processor information (from /proc/cpuinfo):
[8.3.] Module information (from /proc/modules):
[8.4.] Loaded driver and hardware information (/proc/ioports, /proc/iomem)
[8.5.] PCI information ('lspci -vvv' as root)
[8.6.] SCSI information (from /proc/scsi/scsi)
[8.7.] Other information that might be relevant to the problem
(please look in /proc and include all information that you
think to be relevant):
[X.] Other notes, patches, fixes, workarounds:
I don't expect there would be anything relevant here, but feel free to
ask. It's a qemu x86_64 VM image running on my Intel laptop. If you
want to test this, check out
https://github.com/cockpit-project/bots/tree/image-refresh-ubuntu-2204-20240114-225118
and run
./vm-run -q ubuntu-2204
at which point you should be presented with instructions about how to
ssh to the machine.
Thanks for the attention!
Allison Karlitskaya
On Mon, Jan 15, 2024 at 01:13:49PM +0100, Allison Karlitskaya wrote:
> hi,
>
> [1.] One line summary of the problem:
> BLKPG_DEL_PARTITION on an empty loopback device used to return ENXIO
> but now returns EINVAL, breaking partprobe
Note that partx from util-linux also assumes ENXIO, and this errno is
interpreted as non-error ("ignore this partition").
Karel
--
Karel Zak <[email protected]>
http://karelzak.blogspot.com
On 1/15/24 5:57 AM, Karel Zak wrote:
> On Mon, Jan 15, 2024 at 01:13:49PM +0100, Allison Karlitskaya wrote:
>> hi,
>>
>> [1.] One line summary of the problem:
>> BLKPG_DEL_PARTITION on an empty loopback device used to return ENXIO
>> but now returns EINVAL, breaking partprobe
>
> Note that partx from util-linux also assumes ENXIO, and this errno is
> interpreted as non-error ("ignore this partition").
Can someone send a patch for this?
--
Jens Axboe
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;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
if (copy_from_user(&p, upart, sizeof(struct blkpg_partition)))
hi Christoph,
I'm not really setup for compiling and testing new kernel images which
is why I didn't offer to develop a patch for myself (which would have
looked a lot like this one which you just sent). I also generally
have a lot of other things I'm working on at the moment.
The thing that worries me about this approach is that it was already
proposed some months ago, and shot down at the time with the (somewhat
reasonable) justification that if you do any partition table
operations on a device that doesn't contain a partition table, then
"EINVAL" is perhaps somewhat more reasonable as an error. See the
email thread I linked from my earlier message, and particular this
message from Li Lingfeng (who wrote the patch that introduced this
regression in the first place):
> I don't think so.
>
> GENHD_FL_NO_PART means "partition support is disabled". If users try
> to add or resize partition on the disk with this flag, kernel should
> remind them that the parameter of device is wrong.
> So I think it's appropriate to return -EINVAL.
https://marc.info/?l=linux-kernel&m=169753292503830&w=2
So: I suspect the offered patch would solve the issue, but I'm not
sure if it's correct. Another approach might involve returning ENXIO
for "delete" and keeping EINVAL for create (and also picking one of
those for modify), which could also look like moving the check down to
below the point in the function where bdev_del_partition() is called.
I've cc:'d Li Lingfeng on this email, who can maybe provide some
additional context.
Thanks (and sorry...)
Allison
On Tue, 16 Jan 2024 at 12:16, Christoph Hellwig <[email protected]> wrote:
>
> 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;
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> if (copy_from_user(&p, upart, sizeof(struct blkpg_partition)))
>
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.
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)))
>
> .
>
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)))
>>
>> .
>>
>
> .
>
On 1/16/24 6:46 AM, Yu Kuai wrote:
> 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:
Can you send this out as a proper patch?
--
Jens Axboe
On Tue, Jan 16, 2024 at 09:46:21PM +0800, Yu Kuai wrote:
> 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:
Yes, this looks good. I've also verified that historically
adding a partition always returned -EINVAL when beyond the maximum
number of partitions (which would have been 0 in this case).