On Wed, Feb 22, 2023 at 02:58:27PM -0700, Jonathan Derrick wrote:
> From: Jon Derrick <jonathan.derrick@xxxxxxxxx>
>
> Page->index is a pgoff_t and multiplying could cause overflows on a
> 32-bit architecture. In the sb writer, this is used to calculate and
> verify the sector being used, and is multiplied by a sector value. Using
> sector_t will cast it to a u64 type and is the more appropriate type for
> the unit. Additionally, the integer size unit is converted to a sector
> unit in later calculations, and is now corrected to be an unsigned type.
>
> Finally, clean up the calculations using variable aliases to improve
> readabiliy.
>
> Signed-off-by: Jon Derrick <jonathan.derrick@xxxxxxxxx>
> ---
> drivers/md/md-bitmap.c | 36 +++++++++++++++---------------------
> 1 file changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 5c65268a2d97..11f4453775ee 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -215,55 +215,49 @@ static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap,
> struct block_device *bdev;
> struct mddev *mddev = bitmap->mddev;
> struct bitmap_storage *store = &bitmap->storage;
> - loff_t offset = mddev->bitmap_info.offset;
> - int size = PAGE_SIZE;
> + sector_t offset = mddev->bitmap_info.offset;
> + sector_t ps, sboff, doff;
> + unsigned int size = PAGE_SIZE;
>
> bdev = (rdev->meta_bdev) ? rdev->meta_bdev : rdev->bdev;
> if (page->index == store->file_pages - 1) {
> - int last_page_size = store->bytes & (PAGE_SIZE - 1);
> + unsigned int last_page_size = store->bytes & (PAGE_SIZE - 1);
> if (last_page_size == 0)
This should probably grow an empty line after the variable
declaration (here or in the previous patch).
> + sboff < (doff + mddev->dev_sectors + (PAGE_SIZE / SECTOR_SIZE)))
No need for either pair of braces here.
> - rdev->sb_start + offset
> - + page->index * (PAGE_SIZE / SECTOR_SIZE),
> - size, page);
> + md_super_write(mddev, rdev, sboff + ps,
> + (int) size, page);
This easily fits into a single line now.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
