[PATCH 0/2] block: make some queue limits checks more robust

[PATCH 0/2] block: make some queue limits checks more robust

[John Garry]

This series contains a couple of changes to make request queue limits
checks more robust.

I made the change to enforce a power-of-2 pbs as RFC as I am not sure
what setups this can break.

John Garry (2):
  block: avoid possible overflow for chunk_sectors check in
    blk_stack_limits()
  block: Enforce power-of-2 physical block size

 block/blk-settings.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.43.5

[PATCH 1/2] block: avoid possible overflow for chunk_sectors check in blk_stack_limits()

[John Garry]

In blk_stack_limits(), we check that the t->chunk_sectors value is a
multiple of the t->physical_block_size value.

However, by finding the chunk_sectors value in bytes, we may overflow
the unsigned int which holds chunk_sectors, so change the check to be
based on sectors.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-settings.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index a6ac293f47e3..fa53a330f9b9 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -795,7 +795,7 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 	}
 
 	/* chunk_sectors a multiple of the physical block size? */
-	if ((t->chunk_sectors << 9) & (t->physical_block_size - 1)) {
+	if (t->chunk_sectors % (t->physical_block_size >> SECTOR_SHIFT)) {
 		t->chunk_sectors = 0;
 		t->flags |= BLK_FLAG_MISALIGNED;
 		ret = -1;
-- 
2.43.5

[PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[John Garry]

The merging/splitting code and other queue limits checking depends on the
physical block size being a power-of-2, so enforce it.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-settings.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index fa53a330f9b9..5ae0a253e43f 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -274,6 +274,10 @@ int blk_validate_limits(struct queue_limits *lim)
 	}
 	if (lim->physical_block_size < lim->logical_block_size)
 		lim->physical_block_size = lim->logical_block_size;
+	else if (!is_power_of_2(lim->physical_block_size)) {
+		pr_warn("Invalid physical block size (%d)\n", lim->physical_block_size);
+		return -EINVAL;
+	}
 
 	/*
 	 * The minimum I/O size defaults to the physical block size unless
-- 
2.43.5

Re: [PATCH 1/2] block: avoid possible overflow for chunk_sectors check in blk_stack_limits()

[Hannes Reinecke]

On 7/22/25 12:26, John Garry wrote:
> In blk_stack_limits(), we check that the t->chunk_sectors value is a
> multiple of the t->physical_block_size value.
> 
> However, by finding the chunk_sectors value in bytes, we may overflow
> the unsigned int which holds chunk_sectors, so change the check to be
> based on sectors.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>   block/blk-settings.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index a6ac293f47e3..fa53a330f9b9 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -795,7 +795,7 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
>   	}
>   
>   	/* chunk_sectors a multiple of the physical block size? */
> -	if ((t->chunk_sectors << 9) & (t->physical_block_size - 1)) {
> +	if (t->chunk_sectors % (t->physical_block_size >> SECTOR_SHIFT)) {
>   		t->chunk_sectors = 0;
>   		t->flags |= BLK_FLAG_MISALIGNED;
>   		ret = -1;

<Silent round of applause; someone paid attention to the
numerical mathematics course>

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[Hannes Reinecke]

On 7/22/25 12:26, John Garry wrote:
> The merging/splitting code and other queue limits checking depends on the
> physical block size being a power-of-2, so enforce it.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>   block/blk-settings.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index fa53a330f9b9..5ae0a253e43f 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -274,6 +274,10 @@ int blk_validate_limits(struct queue_limits *lim)
>   	}
>   	if (lim->physical_block_size < lim->logical_block_size)
>   		lim->physical_block_size = lim->logical_block_size;
> +	else if (!is_power_of_2(lim->physical_block_size)) {
> +		pr_warn("Invalid physical block size (%d)\n", lim->physical_block_size);
> +		return -EINVAL;
> +	}
>   
>   	/*
>   	 * The minimum I/O size defaults to the physical block size unless

Why not calling 'blk_validate_block_size()' here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[John Garry]

On 22/07/2025 12:28, Hannes Reinecke wrote:
>> The merging/splitting code and other queue limits checking depends on the
>> physical block size being a power-of-2, so enforce it.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   block/blk-settings.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index fa53a330f9b9..5ae0a253e43f 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -274,6 +274,10 @@ int blk_validate_limits(struct queue_limits *lim)
>>       }
>>       if (lim->physical_block_size < lim->logical_block_size)
>>           lim->physical_block_size = lim->logical_block_size;
>> +    else if (!is_power_of_2(lim->physical_block_size)) {
>> +        pr_warn("Invalid physical block size (%d)\n", lim- 
>> >physical_block_size);
>> +        return -EINVAL;
>> +    }
>>       /*
>>        * The minimum I/O size defaults to the physical block size unless
> 
> Why not calling 'blk_validate_block_size()' here?

blk_validate_block_size() enforces that that size cannot be greater than 
64K - does such a limit exist for the physical block size?

Incidentally blk_validate_block_size() also checks that the size is not 
less then SECTOR_SIZE, which would never be true in 
blk_validate_limits() for the physical block size. But duplicating such 
a check is not a huge deal.

Thanks,
John

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[John Garry]

On 22/07/2025 11:26, John Garry wrote:
> The merging/splitting code and other queue limits checking depends on the
> physical block size being a power-of-2, so enforce it.

JFYI, I have done an audit of all drivers setting physical_block_size 
queue limit. I have doubts on a couple, but it seems that NVMe may be 
the only driver which does not guarantee a power-of-2 physical block 
size - maybe I even am wrong about that.

drivers/block/brd.c - uses PAGE_SIZE, so ok

drivers/block/drbd/drbd_main.c - uses bdev_physical_block_size(), so ok

drivers/block/loop.c - uses same size as LBS (which must be a 
power-of-2), so ok

drivers/block/mtip32xx/mtip32xx.c - uses 4096, so ok

drivers/block/n64cart.c - uses 4096, so ok

drivers/block/nbd.c - uses same size as LBS (which must be a 
power-of-2), so ok

drivers/block/null_blk/main.c - uses same size as LBS (which must be a
power-of-2), so ok

drivers/block/rnbd/rnbd-clt.c - drivers/block/rnbd/rnbd-srv.c - uses
bdev_physical_block_size() to fill in
rnbd_msg_open_rsp.physical_block_size, and rnbd-clt.c fills in
queue_limits.physical_block_size from
rnbd_msg_open_rsp.physical_block_size, so looks ok

drivers/block/sunvdc.c not sure on this one. For v1.2 spec we have 
vio_disk_attr_info.phys_block_size, but I cannot
find a spec detailing it.
https://oss.oracle.com/sparcdocs/hypervisor-api-3.0draft7.pdf has
earlier specs. FWIW, no rules on power-of-2 not mentioned for
vdisk_block_size in that spec, so unlikely to have rules for physical
block size. Default pbs is VDC_DEFAULT_BLK_SIZE = 512, and other sizes
in driver are all power-of-2, so likely phys_block_size
will be a power-of-2 always

drivers/block/ublk_drv.c - uses 1 << p->physical_bs_shift, so ok

drivers/block/virtio_blk.c - not sure, as we use
zone_write_granularity and I don't know if that must be a power-of-2,
but from sd_zbc_read_zones() we use physical_block_size, so prob ok

drivers/block/zloop.c - uses same size as LBS (which must be a
power-of-2), so ok

drivers/block/zram/zram_drv.c - uses PAGE_SIZE, so ok

drivers/md/bcache/super.c - uses same size as LBS (which must be a
power-of-2), so ok

drivers/md/dm-crypt.c - uses same size as LBS (which must be a
power-of-2), so ok

drivers/md/dm-ebs-target.c - ebs_ctr() -> __ebs_check_bs() ensures a
power-of-2, so ok

drivers/md/dm-integrity.c - uses same size as LBS (which must be a
power-of-2), so ok

drivers/md/dm-log-writes.c - uses bdev_physical_block_size(), so ok

drivers/md/dm-vdo/dm-vdo-target.c - VDO_BLOCK_SIZE = 4096, so ok

drivers/md/dm-verity-target.c - uses 1 << v->data_dev_block_bits, so ok

drivers/md/dm-writecache.c - writecache_ctr() line 2376 ensure
blocksize is a power-of-2, so ok

drivers/md/dm-zoned-target.c - uses same size as LBS (which must be a
power-of-2), so ok

drivers/nvdimm/pmem.c - uses PAGE_SIZE, so ok

drivers/nvme/host/core.c - may not be ok, so pbs comes from npwg and
spec does not mandate this is a power-of-2

drivers/scsi/sd.c - uses (1 << (buffer[13] & 0xf)) * sector_size in
sdkp->physical_block_size, so ok

drivers/scsi/sd_zbc.c - uses sdkp->physical_block_size, so ok

drivers/target/target_core_iblock.c - uses bdev_physical_block_size(), so ok


> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>   block/blk-settings.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index fa53a330f9b9..5ae0a253e43f 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -274,6 +274,10 @@ int blk_validate_limits(struct queue_limits *lim)
>   	}
>   	if (lim->physical_block_size < lim->logical_block_size)
>   		lim->physical_block_size = lim->logical_block_size;
> +	else if (!is_power_of_2(lim->physical_block_size)) {
> +		pr_warn("Invalid physical block size (%d)\n", lim->physical_block_size);
> +		return -EINVAL;
> +	}
>   
>   	/*
>   	 * The minimum I/O size defaults to the physical block size unless

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[Damien Le Moal]

On 7/24/25 19:06, John Garry wrote:
> On 22/07/2025 11:26, John Garry wrote:

> drivers/block/virtio_blk.c - not sure, as we use
> zone_write_granularity and I don't know if that must be a power-of-2,
> but from sd_zbc_read_zones() we use physical_block_size, so prob ok

Yes, zone_write_granularity must be a power of 2. Its default should be the LBA
size. There is only one case were it can be different than LBA size: on 512e
HDDs (512 B logical, 4K physical) in which case it must be equal to the physical
block size.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[Hannes Reinecke]

On 7/24/25 12:06, John Garry wrote:
> On 22/07/2025 11:26, John Garry wrote:
>> The merging/splitting code and other queue limits checking depends on the
>> physical block size being a power-of-2, so enforce it.
> 
> JFYI, I have done an audit of all drivers setting physical_block_size 
> queue limit. I have doubts on a couple, but it seems that NVMe may be 
> the only driver which does not guarantee a power-of-2 physical block 
> size - maybe I even am wrong about that.
> 
While there are no real checks on the physical blocksize (all what
matters to the block layer is the logical blocksize) I can't really
see how a device can have a physical blocksize which is _not_ a
multiple of the logical blocksize.

Wouldn't that be a more meaningful check?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.com                               +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[John Garry]

On 24/07/2025 12:04, Hannes Reinecke wrote:
> On 7/24/25 12:06, John Garry wrote:
>> On 22/07/2025 11:26, John Garry wrote:
>>> The merging/splitting code and other queue limits checking depends on 
>>> the
>>> physical block size being a power-of-2, so enforce it.
>>
>> JFYI, I have done an audit of all drivers setting physical_block_size 
>> queue limit. I have doubts on a couple, but it seems that NVMe may be 
>> the only driver which does not guarantee a power-of-2 physical block 
>> size - maybe I even am wrong about that.
>>
> While there are no real checks on the physical blocksize (all what
> matters to the block layer is the logical blocksize) I can't really
> see how a device can have a physical blocksize which is _not_ a
> multiple of the logical blocksize.

The logical block size must be a power-of-2 and we enforce that already. 
So checking the physical block size is a power-of-2 is a stronger check 
that just is it a multiple of the LBS.

> 
> Wouldn't that be a more meaningful check?

There is code in the queue limits checking and also merging/splitting 
code which relies on physical block size being a power-of-2, so that is 
why I wanted to enforce it.

Thanks,
John

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[Bart Van Assche]

On 7/24/25 4:04 AM, Hannes Reinecke wrote:
> While there are no real checks on the physical blocksize (all what
> matters to the block layer is the logical blocksize) I can't really
> see how a device can have a physical blocksize which is _not_ a
> multiple of the logical blocksize.

 From SBC-5:

------------------------------------------------------------------------
Table 91 — LOGICAL BLOCKS PER PHYSICAL BLOCK EXPONENT field

Code   Description
0      One or more physical blocks per logical block (a)
n > 0  2n logical blocks per physical block
a  The number of physical blocks per logical block is not reported.
------------------------------------------------------------------------

I think this means that the SCSI standards support physical blocks that
are smaller than the logical block size. However, as far as I know the
Linux kernel does not try to determine the physical block size in this
case. As one can see in drivers/scsi/sd.c LBPPBE == 0 is treated as one
physical block per logical block:

	/* Logical blocks per physical block exponent */
	sdkp->physical_block_size = (1 << (buffer[13] & 0xf)) * sector_size;

Bart.

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[Bart Van Assche]

On 7/24/25 4:09 AM, John Garry wrote:
> There is code in the queue limits checking and also merging/splitting 
> code which relies on physical block size being a power-of-2, so that is 
> why I wanted to enforce it.

I'm OK with enforcing this.

Thanks,

Bart.

Re: [PATCH 0/2] block: make some queue limits checks more robust

[Martin K. Petersen]

John,

> This series contains a couple of changes to make request queue limits
> checks more robust.

This is OK with me.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen

Re: [PATCH RFC 2/2] block: Enforce power-of-2 physical block size

[Martin K. Petersen]

Bart,

> I think this means that the SCSI standards support physical blocks
> that are smaller than the logical block size.

It's just boilerplate weasel wording to avoid breaking the seal of
compliance for any devices that may have done something like this in the
past.

> However, as far as I know the Linux kernel does not try to determine
> the physical block size in this case. As one can see in
> drivers/scsi/sd.c LBPPBE == 0 is treated as one physical block per
> logical block:
>
> 	/* Logical blocks per physical block exponent */
> 	sdkp->physical_block_size = (1 << (buffer[13] & 0xf)) * sector_size;

Yep. It's the only sensible choice. There is no way for the device to
report a physical block size smaller than the logical block size. Also,
there is no way for us to send a read or a write operation to a block
smaller than the reported logical block size.

-- 
Martin K. Petersen