[PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Long Li]

From: Long Li <longli@microsoft.com>

This patch is for linux-stable 4.1 branch only.

storvsc checks the SG list for gaps before passing them to Hyper-v device.
If there are gaps, data is copied to a bounce buffer and a continuous data
buffer is passed to Hyper-V.

The check on gaps assumes SG list is continuous, and not chained. This is
 not always true. Failing the check may result in incorrect I/O data
passed to the Hyper-v device.

This code path is not used post Linux 4.1.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/scsi/storvsc_drv.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 6c52d14..14dc5c6 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -584,17 +584,18 @@ static int do_bounce_buffer(struct scatterlist *sgl, unsigned int sg_count)
 	for (i = 0; i < sg_count; i++) {
 		if (i == 0) {
 			/* make sure 1st one does not have hole */
-			if (sgl[i].offset + sgl[i].length != PAGE_SIZE)
+			if (sgl->offset + sgl->length != PAGE_SIZE)
 				return i;
 		} else if (i == sg_count - 1) {
 			/* make sure last one does not have hole */
-			if (sgl[i].offset != 0)
+			if (sgl->offset != 0)
 				return i;
 		} else {
 			/* make sure no hole in the middle */
-			if (sgl[i].length != PAGE_SIZE || sgl[i].offset != 0)
+			if (sgl->length != PAGE_SIZE || sgl->offset != 0)
 				return i;
 		}
+		sgl = sg_next(sgl);
 	}
 	return -1;
 }
-- 
2.7.4

Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Christoph Hellwig]

Wouldn't it make sense to backport the changes to set the virt_boundary
(which probably still is the SG_GAPS flag in such an old kernel)?

RE: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Long Li]

 
> Wouldn't it make sense to backport the changes to set the virt_boundary
> (which probably still is the SG_GAPS flag in such an old kernel)?

We can make storvsc use SG_GAPS. But the following patch is missing in 4.1 stable block layer to make this work on some I/O situations. Backporting is more difficult and affect other code.

commit 5e7c4274a70aa2d6f485996d0ca1dad52d0039ca
Author: Jens Axboe <axboe@fb.com>
Date:   Thu Sep 3 19:28:20 2015 +0300

    block: Check for gaps on front and back merges
    
    We are checking for gaps to previous bio_vec, which can
    only detect back merges gaps. Moreover, at the point where
    we check for a gap, we don't know if we will attempt a back
    or a front merge. Thus, check for gap to prev in a back merge
    attempt and check for a gap to next in a front merge attempt.
    
    Signed-off-by: Jens Axboe <axboe@fb.com>
    [sagig: Minor rename change]
    Signed-off-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Christoph Hellwig]

Ok.  If the stable maintainers are ok with your small fix
I'm not going to complain too loudly.  But I'm always worried about
stable trees divering too much from mainline.

Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Greg KH]

On Tue, Aug 22, 2017 at 11:43:19PM -0700, Christoph Hellwig wrote:
> Ok.  If the stable maintainers are ok with your small fix
> I'm not going to complain too loudly.  But I'm always worried about
> stable trees divering too much from mainline.

Given that 90% of the time we do this, something breaks, you have a
right to be worried...

Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Martin K. Petersen]

Christoph,

> Ok.  If the stable maintainers are ok with your small fix
> I'm not going to complain too loudly.  But I'm always worried about
> stable trees divering too much from mainline.

The seemingly innocuous transition from SG_GAPS to virt boundary has
caused several data corruption regressions in the distro kernels. So has
the corresponding conversion of storvsc.

As a result, getting the current upstream code into 4.1 would mean
backporting and testing a significant amount of both block layer and
driver code. I don't think it's worth the risk. This patch is simple and
the path of least resistance.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

[Long Li]

> Christoph,
> 
> > Ok.  If the stable maintainers are ok with your small fix I'm not
> > going to complain too loudly.  But I'm always worried about stable
> > trees divering too much from mainline.
> 
> The seemingly innocuous transition from SG_GAPS to virt boundary has
> caused several data corruption regressions in the distro kernels. So has the
> corresponding conversion of storvsc.
> 
> As a result, getting the current upstream code into 4.1 would mean
> backporting and testing a significant amount of both block layer and driver
> code. I don't think it's worth the risk. This patch is simple and the path of least
> resistance.
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

Sorry to bring up this patch again. It seems it hasn't made it to stable branches.

Please take a look.

> 
> --
> Martin K. Petersen	Oracle Linux Engineering