Re: [PATCH] block: fix discard alignment for non-power-of-two granularities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] block: divide-by-zero in bio_discard_limit() when discard_granularity is 0
--

commit c2a5682afd9058576d36c11b303c1f6073d6bfa9
Author: Hungyu Lin <dennylin0707@gmail.com>

block: fix discard alignment for non-power-of-two granularities

This commit replaces round_up() and round_down() in bio_discard_limit()
with sector_div() and rounddown() to properly align discard bios when
discard_granularity is not a power of two.

Link: https://lore.kernel.org/r/CALjSBEvjbiGDVewuapWKhGDtaxZwcn4=22raUiBEOyj22rY7-g@mail.gmail.com

> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 688bc67cbf733..e0de5bb4d969e 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -13,26 +13,25 @@
>  static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
>  {
>  	unsigned int discard_granularity = bdev_discard_granularity(bdev);
> -	sector_t granularity_aligned_sector;
> +	unsigned int remainder;
>  
>  	if (bdev_is_partition(bdev))
>  		sector += bdev->bd_start_sect;
>  
> -	granularity_aligned_sector =
> -		round_up(sector, discard_granularity >> SECTOR_SHIFT);
> +	remainder = sector_div(sector, discard_granularity >> SECTOR_SHIFT);
>  
>  	/*
>  	 * Make sure subsequent bios start aligned to the discard granularity if
>  	 * it needs to be split.
>  	 */
> -	if (granularity_aligned_sector != sector)
> -		return granularity_aligned_sector - sector;
> +	if (remainder)
> +		return (discard_granularity >> SECTOR_SHIFT) - remainder;
>  
>  	/*
>  	 * Align the bio size to the discard granularity to make splitting the bio
>  	 * at discard granularity boundaries easier in the driver if needed.
>  	 */
> -	return round_down(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT;
> +	return rounddown(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT;
>  }

[Severity: Critical]
Can this trigger a divide-by-zero when discard_granularity is 0?

When a block device does not support discard or has discard disabled via
sysfs, blk_validate_limits() in block/blk-settings.c explicitly sets
discard_granularity to 0:

	if (lim->max_discard_sectors)
		lim->discard_granularity =
			max(lim->discard_granularity, lim->physical_block_size);
	else
		lim->discard_granularity = 0;

Since commit 094259204578 ("block: remove the discard_granularity check
in __blkdev_issue_discard") and commit 30f1e7241422 ("block: move discard
checks into the ioctl handler"), neither blkdev_issue_discard(),
__blkdev_issue_discard(), nor blk_alloc_discard_bio() checks
bdev_max_discard_sectors(bdev) or bdev_discard_granularity(bdev) before
calling bio_discard_limit(bdev, *sector):

block/blk-lib.c:blk_alloc_discard_bio() {
	sector_t bio_sects = min(*nr_sects, bio_discard_limit(bdev, *sector));
	...
}

Instead, they rely on submit_bio_noacct() -> blk_check_bio() in
block/blk-core.c to reject unsupported REQ_OP_DISCARD bios with
BLK_STS_NOTSUPP.

Several callers can reach blk_alloc_discard_bio() on a device with
discard_granularity == 0, such as:

- Callers that do not pre-check bdev_max_discard_sectors(), for example
  hpfs_ioctl() -> hpfs_trim_fs() -> do_trim() -> sb_issue_discard(),
  nvmet_bdev_execute_discard() -> __blkdev_issue_discard(),
  fat_free_clusters() / fat_trim_fs() when mounted with -o discard,
  nilfs_discard_segments(), jfs_issue_discard(), __ext4_remount() with
  -o remount,discard, f2fs_secure_erase(), Xen dispatch_discard_io(),
  r5l_write_super_and_discard_space(), and fd_execute_unmap().

- Any discard caller (such as blk_ioctl_discard(), blkdev_cmd_discard(),
  or active filesystem/swap discard) racing with runtime discard
  disablement when 0 is written to discard_max_bytes via
  queue_max_discard_sectors_store().

Previously, round_up(sector, 0) and round_down(BIO_MAX_SIZE, 0) used
bitwise masks and did not trap. With sector_div() and rounddown(), a zero
discard_granularity divides by zero.

Should discard_granularity be guarded here (similar to
max(lim->discard_granularity >> 9, 1U) in __bio_split_discard()), or
should bio_discard_limit() / blk_alloc_discard_bio() handle
discard_granularity == 0 before dividing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026000511.73371-1-dennylin0707@gmail.com?part=1