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 

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

Please take a look.

> 
> --
> Martin K. PetersenOracle Linux Engineering

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 K. Petersen  Oracle Linux Engineering

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)

[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)

[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 
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 
[sagig: Minor rename change]
Signed-off-by: Sagi Grimberg 

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)?

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

[Long Li]

From: Long Li 

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 
---
 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