[PATCH v2] null_blk: fix validation of block size

[PATCH v2] null_blk: fix validation of block size

[Andreas Hindborg]

From: Andreas Hindborg <a.hindborg@samsung.com>

Block size should be between 512 and PAGE_SIZE and be a power of 2. The current
check does not validate this, so update the check.

Without this patch, null_blk would Oops due to a null pointer deref when
loaded with bs=1536 [1].

Link: https://lore.kernel.org/all/87wmn8mocd.fsf@metaspace.dk/

Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
---

Changes from v2:

 - Use blk_validate_block_size instead of open coding the check.
 - Change upper bound of chec from 4096 to PAGE_SIZE.

V1: https://lore.kernel.org/all/20240601202351.691952-1-nmi@metaspace.dk/

 drivers/block/null_blk/main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index eb023d267369..967d39d191ca 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1823,8 +1823,9 @@ static int null_validate_conf(struct nullb_device *dev)
 		dev->queue_mode = NULL_Q_MQ;
 	}
 
-	dev->blocksize = round_down(dev->blocksize, 512);
-	dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);
+	if (blk_validate_block_size(dev->blocksize) != 0) {
+		return -EINVAL;
+	}
 
 	if (dev->use_per_node_hctx) {
 		if (dev->submit_queues != nr_online_nodes)

base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
-- 
2.45.1

Re: [PATCH v2] null_blk: fix validation of block size

[Keith Busch]

On Mon, Jun 03, 2024 at 09:26:45PM +0200, Andreas Hindborg wrote:
> -	dev->blocksize = round_down(dev->blocksize, 512);
> -	dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);
> +	if (blk_validate_block_size(dev->blocksize) != 0) {
> +		return -EINVAL;
> +	}

No need for the { } brackets for a one-line if.

It also looks like a good idea if this check was just done in
blk_validate_limits() so that each driver doesn't have to do their own
checks. That block function is kind of recent though. Your patch here
looks fine if you want stable back-ports, but I haven't heard any
complaints till recently :)

Re: [PATCH v2] null_blk: fix validation of block size

[Christoph Hellwig]

On Mon, Jun 03, 2024 at 01:47:17PM -0600, Keith Busch wrote:
> On Mon, Jun 03, 2024 at 09:26:45PM +0200, Andreas Hindborg wrote:
> > -	dev->blocksize = round_down(dev->blocksize, 512);
> > -	dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);
> > +	if (blk_validate_block_size(dev->blocksize) != 0) {
> > +		return -EINVAL;
> > +	}
> 
> No need for the { } brackets for a one-line if.

or the != 0.

> 
> It also looks like a good idea if this check was just done in
> blk_validate_limits() so that each driver doesn't have to do their own
> checks. That block function is kind of recent though.

Yes.  We already discussed this in another thread a few days ago.

Re: [PATCH v2] null_blk: fix validation of block size

[John Garry]

On 04/06/2024 05:46, Christoph Hellwig wrote:
>> It also looks like a good idea if this check was just done in
>> blk_validate_limits() so that each driver doesn't have to do their own
>> checks. That block function is kind of recent though.
> Yes.  We already discussed this in another thread a few days ago.
Has anyone taken this work? I was going to unless someone else wants to. 
4 or 5 drivers directly reference blk_validate_block_size() now.

Thanks

Re: [PATCH v2] null_blk: fix validation of block size

[Christoph Hellwig]

On Fri, Jun 28, 2024 at 03:30:00PM +0100, John Garry wrote:
> On 04/06/2024 05:46, Christoph Hellwig wrote:
> > > It also looks like a good idea if this check was just done in
> > > blk_validate_limits() so that each driver doesn't have to do their own
> > > checks. That block function is kind of recent though.
> > Yes.  We already discussed this in another thread a few days ago.
> Has anyone taken this work? I was going to unless someone else wants to. 4
> or 5 drivers directly reference blk_validate_block_size() now.

I haven't look at it yet, so from my point of view feel free to tackle
it.

blk_validate_limits validation of block size (was Re: [PATCH v2] null_blk: fix validation of block size)

[John Garry]

(trim list)

On 29/06/2024 06:07, Christoph Hellwig wrote:
> On Fri, Jun 28, 2024 at 03:30:00PM +0100, John Garry wrote:
>> On 04/06/2024 05:46, Christoph Hellwig wrote:
>>>> It also looks like a good idea if this check was just done in
>>>> blk_validate_limits() so that each driver doesn't have to do their own
>>>> checks. That block function is kind of recent though.
>>> Yes.  We already discussed this in another thread a few days ago.
>> Has anyone taken this work? I was going to unless someone else wants to. 4
>> or 5 drivers directly reference blk_validate_block_size() now.
> 
> I haven't look at it yet, so from my point of view feel free to tackle
> it.

I spent a bit of time on this, and the driver changes are pretty 
straightforward, apart from nbd.

For nbd, we cannot only change to just stop calling 
blk_validate_limits(). This is because the LBS is possibly updated in a
2-stage process:
a. update block size in the driver and validate
b. update queue limits

like:

static int __nbd_set_size(struct nbd_device *nbd, loff_t bytesize,
		loff_t blksize)
{
	...

	if (blk_validate_block_size(blksize))
		return -EINVAL;

	nbd->config->bytesize = bytesize;
	nbd->config->blksize_bits = __ffs(blksize);

	if (!nbd->pid)
		return 0;

	lim = queue_limits_start_update(nbd->disk->queue);
	...
	error = queue_limits_commit_update(nbd->disk->queue, &lim);

So if we stop validating the limits in a., there is a user-visible 
change in behaviour (as we stop rejecting invalid limits from the 
NBD_SET_BLKSIZE ioctl).

We could add a "dryrun" option to queue_limits_commit_update() (and call 
that instead of blk_validate_block_size(), which is effectively the same 
as calling blk_validate_block_size()). Or we can keep
nbd as the only blk_validate_limits() user (outside the block layer).

Any better ideas?

Re: blk_validate_limits validation of block size (was Re: [PATCH v2] null_blk: fix validation of block size)

[Christoph Hellwig]

On Wed, Jul 03, 2024 at 01:20:26PM +0100, John Garry wrote:
> So if we stop validating the limits in a., there is a user-visible change in
> behaviour (as we stop rejecting invalid limits from the NBD_SET_BLKSIZE
> ioctl).
> 
> We could add a "dryrun" option to queue_limits_commit_update() (and call
> that instead of blk_validate_block_size(), which is effectively the same as
> calling blk_validate_block_size()). Or we can keep
> nbd as the only blk_validate_limits() user (outside the block layer).

I'd just keep the extra external blk_validate_block_size call in nbd.c.

Maybe add a comment to the blk_validate_block_size declaration that
drivers should not bother with it as it's already done by
blk_validate_limits.

Re: blk_validate_limits validation of block size (was Re: [PATCH v2] null_blk: fix validation of block size)

[John Garry]

On 03/07/2024 14:19, Christoph Hellwig wrote:
> On Wed, Jul 03, 2024 at 01:20:26PM +0100, John Garry wrote:
>> So if we stop validating the limits in a., there is a user-visible change in
>> behaviour (as we stop rejecting invalid limits from the NBD_SET_BLKSIZE
>> ioctl).
>>
>> We could add a "dryrun" option to queue_limits_commit_update() (and call
>> that instead of blk_validate_block_size(), which is effectively the same as
>> calling blk_validate_block_size()). Or we can keep
>> nbd as the only blk_validate_limits() user (outside the block layer).
> I'd just keep the extra external blk_validate_block_size call in nbd.c.
> 
> Maybe add a comment to the blk_validate_block_size declaration that
> drivers should not bother with it as it's already done by
> blk_validate_limits.

ok, fine. It's a bit unfortunate that blk_validate_block_size() won't be 
internal to the block layer.

Re: blk_validate_limits validation of block size (was Re: [PATCH v2] null_blk: fix validation of block size)

[Christoph Hellwig]

On Wed, Jul 03, 2024 at 06:28:48PM +0100, John Garry wrote:
> ok, fine. It's a bit unfortunate that blk_validate_block_size() won't be
> internal to the block layer.

It is a completely trivial helper not really exposing any internals.
In theory we could just open code, but with the PAGE_SIZE limit that
people are trying to remove with large block size support that might
actually create more problems than it solves.

Re: [PATCH v2] null_blk: fix validation of block size

[Ming Lei]

On Mon, Jun 03, 2024 at 09:26:45PM +0200, Andreas Hindborg wrote:
> From: Andreas Hindborg <a.hindborg@samsung.com>
> 
> Block size should be between 512 and PAGE_SIZE and be a power of 2. The current
> check does not validate this, so update the check.
> 
> Without this patch, null_blk would Oops due to a null pointer deref when
> loaded with bs=1536 [1].
> 
> Link: https://lore.kernel.org/all/87wmn8mocd.fsf@metaspace.dk/
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
> ---
> 
> Changes from v2:
> 
>  - Use blk_validate_block_size instead of open coding the check.
>  - Change upper bound of chec from 4096 to PAGE_SIZE.
> 
> V1: https://lore.kernel.org/all/20240601202351.691952-1-nmi@metaspace.dk/
> 
>  drivers/block/null_blk/main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index eb023d267369..967d39d191ca 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1823,8 +1823,9 @@ static int null_validate_conf(struct nullb_device *dev)
>  		dev->queue_mode = NULL_Q_MQ;
>  	}
>  
> -	dev->blocksize = round_down(dev->blocksize, 512);
> -	dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);
> +	if (blk_validate_block_size(dev->blocksize) != 0) {
> +		return -EINVAL;
> +	}

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

Re: [PATCH v2] null_blk: fix validation of block size

[Jens Axboe]

On Mon, 03 Jun 2024 21:26:45 +0200, Andreas Hindborg wrote:
> Block size should be between 512 and PAGE_SIZE and be a power of 2. The current
> check does not validate this, so update the check.
> 
> Without this patch, null_blk would Oops due to a null pointer deref when
> loaded with bs=1536 [1].
> 
> Link: https://lore.kernel.org/all/87wmn8mocd.fsf@metaspace.dk/
> 
> [...]

Applied, thanks!

[1/1] null_blk: fix validation of block size
      commit: 237e061865aa5a24c2cd960c4feb2904c8736a75

Best regards,
-- 
Jens Axboe