ACK/cmnt: [trusty][PATCH] UBUNTU: SAUCE: storvsc: do not assume SG list is continuous when doing bounce buffers

[Khaled Elmously]

On 2018-01-30 13:39:02 , Marcelo Henrique Cerri wrote:
> From: Long Li <longli at microsoft.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1742480
> 
> 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 at microsoft.com>
> Signed-off-by: Marcelo Henrique Cerri <marcelo.cerri at canonical.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 fa846f9c06b3..272e08fccf2a 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -561,17 +561,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;
>  }

Since the code was changed from treating sgl as a continuous array to a treating it like a linked list, I guess I was expecting the loop iteration to change from:

for (i = 0; i < sg_count; i++) {
  // body of loop
}

to something like:

sgl = sgl[0];
while (sgl) {
  // body of loop
  sgl = sg_next(sgl);
}


Though I can't say it's wrong the way it is (I don't know where sg_count is derived from).

Anyhow, since this is the patch that Microsoft provided, I'll assume they know what they're doing :)

Acked-by: Khalid Elmously <khalid.elmously at canonical.com>