Received-SPF: pass (google.com: domain of linux-kernel+bounces-27401-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <CAOYeF9VsmqKMcQjo1k6YkGNujwN-nzfxY17N3F-CMikE1tYp+w@mail.gmail.com>
 <ZaZesiKpbMEiiTrf@infradead.org>
In-Reply-To: <ZaZesiKpbMEiiTrf@infradead.org>
From: Allison Karlitskaya <allison.karlitskaya@redhat.com>
Date: Tue, 16 Jan 2024 14:03:01 +0100
Message-ID: <CAOYeF9VoN3LpA+CV=fkBR62vqZ8VEvoWD_Hb5Ay5tK9M-Xw1Xw@mail.gmail.com>
Subject: Re: PROBLEM: BLKPG_DEL_PARTITION with GENHD_FL_NO_PART used to return
 ENXIO, now returns EINVAL
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, 
	Jens Axboe <axboe@kernel.dk>, Li Lingfeng <lilingfeng3@huawei.com>
Content-Type: text/plain; charset="UTF-8"

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 <hch@infradead.org> 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)))
>