| Message ID | 20240603192645.977968-1-nmi@metaspace.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] null_blk: fix validation of block size | expand |
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 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 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 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,
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.
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)