Re: [PATCH 4.19 13/35] null_blk: Fix zone size initialization
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 >> +#include >> #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 > -- Damien Le Moal Western Digital Research
Re: [PATCH 4.19 13/35] null_blk: Fix zone size initialization
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 > +#include > #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 signature.asc Description: PGP signature
[PATCH 4.19 13/35] null_blk: Fix zone size initialization
From: Damien Le Moal
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.
Reported-by: Naohiro Aota
Fixes: ca4b2a011948 ("null_blk: add zone support")
Cc: sta...@vger.kernel.org
Signed-off-by: Damien Le Moal
Reviewed-by: Christoph Hellwig
Reviewed-by: Johannes Thumshirn
Signed-off-by: Jens Axboe
Signed-off-by: Greg Kroah-Hartman
---
drivers/block/null_blk_zoned.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
--- 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
+#include
#include "null_blk.h"
-/* zone_size in MBs to sectors. */
-#define ZONE_SIZE_SHIFT11
+#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 = >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;
