[PATCH] block: avoid scatterlist offsets > PAGE_SIZE

[PATCH] block: avoid scatterlist offsets > PAGE_SIZE

[Christoph Hellwig]

While we generally allow scatterlists to have offsets larger than page
size for an entry, and other subsystems like the crypto code make use of
that, the block layer isn't quite ready for that.  Flip the switch back
to avoid them for now, and revisit that decision early in a merge window
once the known offenders are fixed.

Fixes: 8a96a0e40810 ("block: rewrite blk_bvec_map_sg to avoid a nth_page call")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 247b17f2a0f6..21e87a714a73 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -474,9 +474,21 @@ static unsigned blk_bvec_map_sg(struct request_queue *q,
 	while (nbytes > 0) {
 		unsigned offset = bvec->bv_offset + total;
 		unsigned len = min(get_max_segment_size(q, offset), nbytes);
+		struct page *page = bvec->bv_page;
+
+		/*
+		 * Unfortunately a fair number of drivers barf on scatterlists
+		 * that have an offset larger than PAGE_SIZE, despite other
+		 * subsystems dealing with that invariant just fine.  For now
+		 * stick to the legacy format where we never present those from
+		 * the block layer, but the code below should be removed once
+		 * these offenders (mostly MMC/SD drivers) are fixed.
+		 */
+		page += (offset >> PAGE_SHIFT);
+		offset &= ~PAGE_MASK;
 
 		*sg = blk_next_sg(sg, sglist);
-		sg_set_page(*sg, bvec->bv_page, len, offset);
+		sg_set_page(*sg, page, len, offset);
 
 		total += len;
 		nbytes -= len;
-- 
2.20.1

Re: [PATCH] block: avoid scatterlist offsets > PAGE_SIZE

[Guenter Roeck]

On Fri, Apr 19, 2019 at 08:56:24AM +0200, Christoph Hellwig wrote:
> While we generally allow scatterlists to have offsets larger than page
> size for an entry, and other subsystems like the crypto code make use of
> that, the block layer isn't quite ready for that.  Flip the switch back
> to avoid them for now, and revisit that decision early in a merge window
> once the known offenders are fixed.
> 
> Fixes: 8a96a0e40810 ("block: rewrite blk_bvec_map_sg to avoid a nth_page call")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

(cautious)

Tested-by: Guenter Roeck <linux@roeck-us.net>

Cautious because there are currently a number if crashes and runtime
warnings in -next. I don't think any are related to the block code,
but I may be wrong.

Guenter

> ---
>  block/blk-merge.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 247b17f2a0f6..21e87a714a73 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -474,9 +474,21 @@ static unsigned blk_bvec_map_sg(struct request_queue *q,
>  	while (nbytes > 0) {
>  		unsigned offset = bvec->bv_offset + total;
>  		unsigned len = min(get_max_segment_size(q, offset), nbytes);
> +		struct page *page = bvec->bv_page;
> +
> +		/*
> +		 * Unfortunately a fair number of drivers barf on scatterlists
> +		 * that have an offset larger than PAGE_SIZE, despite other
> +		 * subsystems dealing with that invariant just fine.  For now
> +		 * stick to the legacy format where we never present those from
> +		 * the block layer, but the code below should be removed once
> +		 * these offenders (mostly MMC/SD drivers) are fixed.
> +		 */
> +		page += (offset >> PAGE_SHIFT);
> +		offset &= ~PAGE_MASK;
>  
>  		*sg = blk_next_sg(sg, sglist);
> -		sg_set_page(*sg, bvec->bv_page, len, offset);
> +		sg_set_page(*sg, page, len, offset);
>  
>  		total += len;
>  		nbytes -= len;
> -- 
> 2.20.1
> 

Re: [PATCH] block: avoid scatterlist offsets > PAGE_SIZE

[Ming Lei]

On Sat, Apr 20, 2019 at 6:55 AM Christoph Hellwig <hch@lst.de> wrote:
>
> While we generally allow scatterlists to have offsets larger than page
> size for an entry, and other subsystems like the crypto code make use of
> that, the block layer isn't quite ready for that.  Flip the switch back
> to avoid them for now, and revisit that decision early in a merge window
> once the known offenders are fixed.
>
> Fixes: 8a96a0e40810 ("block: rewrite blk_bvec_map_sg to avoid a nth_page call")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-merge.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 247b17f2a0f6..21e87a714a73 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -474,9 +474,21 @@ static unsigned blk_bvec_map_sg(struct request_queue *q,
>         while (nbytes > 0) {
>                 unsigned offset = bvec->bv_offset + total;
>                 unsigned len = min(get_max_segment_size(q, offset), nbytes);
> +               struct page *page = bvec->bv_page;
> +
> +               /*
> +                * Unfortunately a fair number of drivers barf on scatterlists
> +                * that have an offset larger than PAGE_SIZE, despite other
> +                * subsystems dealing with that invariant just fine.  For now
> +                * stick to the legacy format where we never present those from
> +                * the block layer, but the code below should be removed once
> +                * these offenders (mostly MMC/SD drivers) are fixed.
> +                */
> +               page += (offset >> PAGE_SHIFT);
> +               offset &= ~PAGE_MASK;
>
>                 *sg = blk_next_sg(sg, sglist);
> -               sg_set_page(*sg, bvec->bv_page, len, offset);
> +               sg_set_page(*sg, page, len, offset);

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming Lei

Re: [PATCH] block: avoid scatterlist offsets > PAGE_SIZE

[Jens Axboe]

On 4/19/19 12:56 AM, Christoph Hellwig wrote:
> While we generally allow scatterlists to have offsets larger than page
> size for an entry, and other subsystems like the crypto code make use of
> that, the block layer isn't quite ready for that.  Flip the switch back
> to avoid them for now, and revisit that decision early in a merge window
> once the known offenders are fixed.

Applied, thanks.

-- 
Jens Axboe