Skip to contentSkip to conversation

block: check for max_hw_sectors underflow

PATCH In review Hannes Reinecke 3 participants 5 messages May 24, 2024 – May 28, 2024

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 block: check for max_hw_sectors underflow

The logical block size need to be smaller than the max_hw_sector setting, otherwise we can't even transfer a single LBA.

---
 block/blk-settings.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
block/blk-settings.c +6 -2
index d2731843f2fc..030afb597183 100644
@@ -104,6 +104,7 @@ static int blk_validate_zoned_limits(struct queue_limits *lim)
 static int blk_validate_limits(struct queue_limits *lim)
 {
 	unsigned int max_hw_sectors;
+	unsigned int logical_block_sectors;
 
 	/*
 	 * Unless otherwise specified, default to 512 byte logical blocks and a
@@ -134,8 +135,11 @@ static int blk_validate_limits(struct queue_limits *lim)
 		lim->max_hw_sectors = BLK_SAFE_MAX_SECTORS;
 	if (WARN_ON_ONCE(lim->max_hw_sectors < PAGE_SECTORS))
 		return -EINVAL;
+	logical_block_sectors = lim->logical_block_size >> SECTOR_SHIFT;
+	if (WARN_ON_ONCE(logical_block_sectors > lim->max_hw_sectors))
+		return -EINVAL;
 	lim->max_hw_sectors = round_down(lim->max_hw_sectors,
-			lim->logical_block_size >> SECTOR_SHIFT);
+			logical_block_sectors);
 
 	/*
 	 * The actual max_sectors value is a complex beast and also takes the
@@ -153,7 +157,7 @@ static int blk_validate_limits(struct queue_limits *lim)
 		lim->max_sectors = min(max_hw_sectors, BLK_DEF_MAX_SECTORS_CAP);
 	}
 	lim->max_sectors = round_down(lim->max_sectors,
-			lim->logical_block_size >> SECTOR_SHIFT);
+			logical_block_sectors);
 
 	/*
 	 * Random default for the maximum number of segments.  Driver should not
Signature · 1 line
2.35.3
Reviewed-by on v2 1/1
#2 lore raw
In reply to Hannes Reinecke
Quoted 4 lines · Hannes Reinecke

The logical block size need to be smaller than the max_hw_sector setting, otherwise we can't even transfer a single LBA.

Regardless of comment, below:

Quoted 17 lines
---
  block/blk-settings.c | 8 ++++++--
  1 file changed, 6 insertions(+), 2 deletions(-)
block/blk-settings.c +1 -0
index d2731843f2fc..030afb597183 100644
@@ -104,6 +104,7 @@ static int blk_validate_zoned_limits(struct queue_limits *lim)
  static int blk_validate_limits(struct queue_limits *lim)
  {
  	unsigned int max_hw_sectors;
+	unsigned int logical_block_sectors;
 
  	/*
  	 * Unless otherwise specified, default to 512 byte logical blocks and a

block/blk-settings.c static int blk_validate_limits(struct queue_limits *lim) · line 141

Quoted 7 lines
block/blk-settings.c +3 -0
@@ -134,8 +135,11 @@ static int blk_validate_limits(struct queue_limits *lim)
  		lim->max_hw_sectors = BLK_SAFE_MAX_SECTORS;
  	if (WARN_ON_ONCE(lim->max_hw_sectors < PAGE_SECTORS))
  		return -EINVAL;
+	logical_block_sectors = lim->logical_block_size >> SECTOR_SHIFT;
+	if (WARN_ON_ONCE(logical_block_sectors > lim->max_hw_sectors))
+		return -EINVAL;
  	lim->max_hw_sectors = round_down(lim->max_hw_sectors,

I don't think that we ever check if lim->logical_block_size is a power-of-2 - but that's a given, right?

Quoted 14 lines
block/blk-settings.c +2 -2
-			lim->logical_block_size >> SECTOR_SHIFT);
+			logical_block_sectors);
 
  	/*
  	 * The actual max_sectors value is a complex beast and also takes the
@@ -153,7 +157,7 @@ static int blk_validate_limits(struct queue_limits *lim)
  		lim->max_sectors = min(max_hw_sectors, BLK_DEF_MAX_SECTORS_CAP);
  	}
  	lim->max_sectors = round_down(lim->max_sectors,
-			lim->logical_block_size >> SECTOR_SHIFT);
+			logical_block_sectors);
 
  	/*
  	 * Random default for the maximum number of segments.  Driver should not
#3 lore raw
Quoted 2 lines · John Garry

I don't think that we ever check if lim->logical_block_size is a power-of-2
- but that's a given, right?

It has to be for the block stack to work. That being said now that we do have a single good place for sanity checks it's probably worth to add this check explicitly.

#4 lore raw
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · John Garry

I don't think that we ever check if lim->logical_block_size is a power-of-2
- but that's a given, right?

It has to be for the block stack to work. That being said now that we do have a single good place for sanity checks it's probably worth to add this check explicitly.

I think that we might be able to get rid of some of the driver blk_validate_block_size() calls (if we do that), like __nbd_set_size() -> blk_validate_block_size()