[PATCH] block: disallow changing max_sectors_kb on a request stacking device

Otherwise users can easily shoot themselves in the foot by creating the
situation where the top-level stacked device (e.g. DM multipath) has a
larger max_sectors_kb than the underlying device(s).  Which will
certainly lead to IO errors due to the "over max size limit" check in
blk_cloned_rq_check_limits().

Use of WARN_ON_ONCE() gives users visibility into which application
attempted to perform this unsupported max_sectors_kb change.

This is a crude, yet effective, solution that forces the use of system
software (e.g. udev rules or multipathd) to tweak max_sectors_kb of the
underlying devices _before_ a layer like DM multipath is layered ontop.

Signed-off-by: Mike Snitzer <redacted>
---
 block/blk-sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 9cc8d7c..cce43d7 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c

@@ -199,8 +199,12 @@ queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
 	unsigned long max_sectors_kb,
 		max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1,
 			page_kb = 1 << (PAGE_SHIFT - 10);
-	ssize_t ret = queue_var_store(&max_sectors_kb, page, count);
+	ssize_t ret;
+
+	if (WARN_ON_ONCE(blk_queue_stackable(q)))
+		return -EINVAL;
 
+	ret = queue_var_store(&max_sectors_kb, page, count);
 	if (ret < 0)
 		return ret;
 

-- 
2.8.4 (Apple Git-73)

On Fri, Oct 28 2016 at  3:45pm -0400,
Mike Snitzer [off-list ref] wrote:

Jens,

Never heard from you on this patch.  Would you like me to resend without
the WARN_ON_ONCE?  (Failing is enough, the userspace code that attempts
to set max_sectors_kb on DM multipath will get the user's attention)

Otherwise users can easily shoot themselves in the foot by creating the
situation where the top-level stacked device (e.g. DM multipath) has a
larger max_sectors_kb than the underlying device(s).  Which will
certainly lead to IO errors due to the "over max size limit" check in
blk_cloned_rq_check_limits().

This is a crude, yet effective, solution that forces the use of system
software (e.g. udev rules or multipathd) to tweak max_sectors_kb of the
underlying devices _before_ a layer like DM multipath is layered ontop.

Signed-off-by: Mike Snitzer <redacted>
---
 block/blk-sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 9cc8d7c..934f326 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c

@@ -199,8 +199,12 @@ queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
 	unsigned long max_sectors_kb,
 		max_hw_sectors_kb = queue_max_hw_sectors(q) >> 1,
 			page_kb = 1 << (PAGE_SHIFT - 10);
-	ssize_t ret = queue_var_store(&max_sectors_kb, page, count);
+	ssize_t ret;
+
+	if (blk_queue_stackable(q))
+		return -EINVAL;
 
+	ret = queue_var_store(&max_sectors_kb, page, count);
 	if (ret < 0)
 		return ret;
 

-- 
2.9.3 (Apple Git-75)

On 11/07/2016 12:26 PM, Mike Snitzer wrote:

Otherwise users can easily shoot themselves in the foot by creating the
situation where the top-level stacked device (e.g. DM multipath) has a
larger max_sectors_kb than the underlying device(s).  Which will
certainly lead to IO errors due to the "over max size limit" check in
blk_cloned_rq_check_limits().

This is a crude, yet effective, solution that forces the use of system
software (e.g. udev rules or multipathd) to tweak max_sectors_kb of the
underlying devices _before_ a layer like DM multipath is layered ontop.

Maybe I'm missing something, but the code we have in place splits it
into max sectors for software and hardware. Shouldn't the stacked
devices have max_hw_sectors capped to what the lower levels support? If
that was done, we would not have to worry about a user fiddling with
max_sectors_kb, since it could only be smaller (or equal to) the max
size of the lower level.

-- 
Jens Axboe

On Mon, Nov 07 2016 at  2:32pm -0500,
Jens Axboe [off-list ref] wrote:

On 11/07/2016 12:26 PM, Mike Snitzer wrote:

quoted

Otherwise users can easily shoot themselves in the foot by creating the
situation where the top-level stacked device (e.g. DM multipath) has a
larger max_sectors_kb than the underlying device(s).  Which will
certainly lead to IO errors due to the "over max size limit" check in
blk_cloned_rq_check_limits().

This is a crude, yet effective, solution that forces the use of system
software (e.g. udev rules or multipathd) to tweak max_sectors_kb of the
underlying devices _before_ a layer like DM multipath is layered ontop.

Maybe I'm missing something, but the code we have in place splits it
into max sectors for software and hardware. Shouldn't the stacked
devices have max_hw_sectors capped to what the lower levels support? If
that was done, we would not have to worry about a user fiddling with
max_sectors_kb, since it could only be smaller (or equal to) the max
size of the lower level.

DM multipath just uses blk_stack_limits() to stack limits, which has:

        t->max_sectors = min_not_zero(t->max_sectors, b->max_sectors);
        t->max_hw_sectors = min_not_zero(t->max_hw_sectors, b->max_hw_sectors);
        t->max_dev_sectors = min_not_zero(t->max_dev_sectors, b->max_dev_sectors);

But I assume you realize that.. I'm just missing the relation you're
saying exists, or should exist, between max_hw_sectors and max_sectors
(other than the obvious: max_sectors cannot be greater than
max_hw_sectors) as they relate to stacking.

You're suggesting that when the DM multipath device's limits are stacked
up from the underlying devices: cap the mpath's max_hw_sectors to the
underlying devices' max_sectors?