drivers/block/null_blk/main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
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
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
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
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 :)
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.
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
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.
(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?
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.
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.
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.
© 2016 - 2026 Red Hat, Inc.