2017-08-22 05:32:58

by Long Li

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

From: Long Li <[email protected]>

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 <[email protected]>
---
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


2017-08-22 06:28:47

by Christoph Hellwig

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

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

2017-08-22 19:12:08

by Long Li

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


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

2017-08-23 06:43:24

by Christoph Hellwig

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

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.

2017-08-23 13:41:51

by Greg Kroah-Hartman

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

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

2017-08-23 15:22:43

by Martin K. Petersen

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


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 <[email protected]>

--
Martin K. Petersen Oracle Linux Engineering

2018-01-09 23:05:01

by Long Li

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

> 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 <[email protected]>

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