[dm-devel] [PATCH] dm: check max_sectors in dm_merge_bvec (was: Re: dm: max_segments=1 if merge_bvec_fn is not supported)

[Mike Snitzer]

I'm late to this old thread but I stumbled across it while auditing the
various dm-devel patchwork patches, e.g.:
https://patchwork.kernel.org/patch/83666/
https://patchwork.kernel.org/patch/83932/

On Mon, Mar 08 2010 at  8:14am -0500,
Lars Ellenberg <lars.ellenberg at linbit.com> wrote:

> On Mon, Mar 08, 2010 at 03:35:37AM -0500, Mikulas Patocka wrote:
> > Hi
> > 
> > That patch with limits->max_segments = 1; is wrong. It fixes this bug 
> > sometimes and sometimes not.
> > 
> > The problem is, if someone attempts to create a bio with two vector 
> > entries, the first maps the last sector contained in some page and the 
> > second maps the first sector of the next physical page: it has one 
> > segment, it has size <= PAGE_SIZE, but it still may cross raid stripe and 
> > the raid driver will reject it.
> 
> Now that you put it that way ;)
> You are right.
> 
> My asumption that "single segment" was  
> equalvalent in practice with "single bvec"
> does not hold true in that case.
> 
> Then, what about adding seg_boundary_mask restrictions as well?
> 	max_sectors = PAGE_SIZE >> 9;
> 	max_segments = 1;
> 	seg_boundary_mask = PAGE_SIZE -1;
> or some such.
> 
> > > > This is not the first time this has been patched, btw.
> > > > See https://bugzilla.redhat.com/show_bug.cgi?id=440093
> > > > and the patch by Mikulas:
> > > > https://bugzilla.redhat.com/attachment.cgi?id=342638&action=diff
> > 
> > Look at this patch, it is the proper way how to fix it: create a 
> > merge_bvec_fn that reject more than one biovec entry.
> 
> If adding seg_boundary_mask is still not sufficient,
> lets merge that patch instead?
> Why has it been dropped, respectively never been merged?
> It became obsolete for dm-linear by 7bc3447b,
> but in general the bug is still there, or am I missing something?

No it _should_ be fixed in general given DM's dm_merge_bvec() _but_ I
did uncover what I think is a subtle oversight in its implementation.

Given dm_set_device_limits() sets q->limits->max_sectors,
shouldn't dm_merge_bvec() be using queue_max_sectors rather than
queue_max_hw_sectors?

blk_queue_max_hw_sectors() establishes that max_hw_sectors is the hard
limit and max_sectors the soft.  But AFAICT no relation is maintained
between the two over time (even though max_sectors <= max_hw_sectors
_should_ be enforced; in practice there is no blk_queue_max_sectors
setter that uniformly enforces as much).

Anyway, I think we need the following patch:
--

From: Mike Snitzer <snitzer at redhat.com>
Subject: dm: check max_sectors in dm_merge_bvec

dm_set_device_limits() will set q->limits->max_sectors to <= PAGE_SIZE
if an underlying device has a merge_bvec_fn.  Therefore, dm_merge_bvec()
must use queue_max_sectors() rather than queue_max_hw_sectors() to check
the appropriate limit.

Signed-off-by: Mike Snitzer <snitzer at redhat.com>
---
 drivers/md/dm.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7cb1352..e83dcc8 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1358,12 +1358,11 @@ static int dm_merge_bvec(struct request_queue *q,
 	/*
 	 * If the target doesn't support merge method and some of the devices
 	 * provided their merge_bvec method (we know this by looking at
-	 * queue_max_hw_sectors), then we can't allow bios with multiple vector
+	 * queue_max_sectors), then we can't allow bios with multiple vector
 	 * entries.  So always set max_size to 0, and the code below allows
 	 * just one page.
 	 */
-	else if (queue_max_hw_sectors(q) <= PAGE_SIZE >> 9)
-
+	else if (queue_max_sectors(q) <= PAGE_SIZE >> 9)
 		max_size = 0;
 
 out_table: