| Message ID | 20210104155704.049016882@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi! > commit 0ebcdd702f49aeb0ad2e2d894f8c124a0acc6e23 upstream. > > For a null_blk device with zoned mode enabled is currently initialized > with a number of zones equal to the device capacity divided by the zone > size, without considering if the device capacity is a multiple of the > zone size. If the zone size is not a divisor of the capacity, the zones > end up not covering the entire capacity, potentially resulting is out > of bounds accesses to the zone array. > > Fix this by adding one last smaller zone with a size equal to the > remainder of the disk capacity divided by the zone size if the capacity > is not a multiple of the zone size. For such smaller last zone, the zone > capacity is also checked so that it does not exceed the smaller zone > size. > --- a/drivers/block/null_blk_zoned.c > +++ b/drivers/block/null_blk_zoned.c > @@ -1,9 +1,9 @@ > // SPDX-License-Identifier: GPL-2.0 > #include <linux/vmalloc.h> > +#include <linux/sizes.h> > #include "null_blk.h" > > -/* zone_size in MBs to sectors. */ > -#define ZONE_SIZE_SHIFT 11 > +#define MB_TO_SECTS(mb) (((sector_t)mb * SZ_1M) >> SECTOR_SHIFT) This macro is quite dangerous. (mb) would help, but inline function would be better. > + dev->nr_zones = dev_capacity_sects >> ilog2(dev->zone_size_sects); > + if (dev_capacity_sects & (dev->zone_size_sects - 1)) > + dev->nr_zones++; Is this same as nr_zones = DIV_ROUND_UP(dev_capacity_sects, dev->zone_size_sects)? Would that be faster, more readable and robust against weird dev->zone_size_sects sizes? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
On 2021/01/06 21:55, Pavel Machek wrote: > Hi! > >> commit 0ebcdd702f49aeb0ad2e2d894f8c124a0acc6e23 upstream. >> >> For a null_blk device with zoned mode enabled is currently initialized >> with a number of zones equal to the device capacity divided by the zone >> size, without considering if the device capacity is a multiple of the >> zone size. If the zone size is not a divisor of the capacity, the zones >> end up not covering the entire capacity, potentially resulting is out >> of bounds accesses to the zone array. >> >> Fix this by adding one last smaller zone with a size equal to the >> remainder of the disk capacity divided by the zone size if the capacity >> is not a multiple of the zone size. For such smaller last zone, the zone >> capacity is also checked so that it does not exceed the smaller zone >> size. > >> --- a/drivers/block/null_blk_zoned.c >> +++ b/drivers/block/null_blk_zoned.c >> @@ -1,9 +1,9 @@ >> // SPDX-License-Identifier: GPL-2.0 >> #include <linux/vmalloc.h> >> +#include <linux/sizes.h> >> #include "null_blk.h" >> >> -/* zone_size in MBs to sectors. */ >> -#define ZONE_SIZE_SHIFT 11 >> +#define MB_TO_SECTS(mb) (((sector_t)mb * SZ_1M) >> SECTOR_SHIFT) > > This macro is quite dangerous. (mb) would help, but inline function > would be better. Indeed. > > >> + dev->nr_zones = dev_capacity_sects >> ilog2(dev->zone_size_sects); >> + if (dev_capacity_sects & (dev->zone_size_sects - 1)) >> + dev->nr_zones++; > > Is this same as nr_zones = DIV_ROUND_UP(dev_capacity_sects, > dev->zone_size_sects)? Would that be faster, more readable and robust > against weird dev->zone_size_sects sizes? Yes, we can change to this to be more readable. Will send a cleanup patch. Thanks ! > > Best regards, > Pavel >
--- a/drivers/block/null_blk_zoned.c +++ b/drivers/block/null_blk_zoned.c @@ -1,9 +1,9 @@ // SPDX-License-Identifier: GPL-2.0 #include <linux/vmalloc.h> +#include <linux/sizes.h> #include "null_blk.h" -/* zone_size in MBs to sectors. */ -#define ZONE_SIZE_SHIFT 11 +#define MB_TO_SECTS(mb) (((sector_t)mb * SZ_1M) >> SECTOR_SHIFT) static inline unsigned int null_zone_no(struct nullb_device *dev, sector_t sect) { @@ -12,7 +12,7 @@ static inline unsigned int null_zone_no( int null_zone_init(struct nullb_device *dev) { - sector_t dev_size = (sector_t)dev->size * 1024 * 1024; + sector_t dev_capacity_sects; sector_t sector = 0; unsigned int i; @@ -25,9 +25,12 @@ int null_zone_init(struct nullb_device * return -EINVAL; } - dev->zone_size_sects = dev->zone_size << ZONE_SIZE_SHIFT; - dev->nr_zones = dev_size >> - (SECTOR_SHIFT + ilog2(dev->zone_size_sects)); + dev_capacity_sects = MB_TO_SECTS(dev->size); + dev->zone_size_sects = MB_TO_SECTS(dev->zone_size); + dev->nr_zones = dev_capacity_sects >> ilog2(dev->zone_size_sects); + if (dev_capacity_sects & (dev->zone_size_sects - 1)) + dev->nr_zones++; + dev->zones = kvmalloc_array(dev->nr_zones, sizeof(struct blk_zone), GFP_KERNEL | __GFP_ZERO); if (!dev->zones) @@ -37,7 +40,10 @@ int null_zone_init(struct nullb_device * struct blk_zone *zone = &dev->zones[i]; zone->start = zone->wp = sector; - zone->len = dev->zone_size_sects; + if (zone->start + dev->zone_size_sects > dev_capacity_sects) + zone->len = dev_capacity_sects - zone->start; + else + zone->len = dev->zone_size_sects; zone->type = BLK_ZONE_TYPE_SEQWRITE_REQ; zone->cond = BLK_ZONE_COND_EMPTY;