diff mbox series

block: make sure that bvec length can't be overflowed

Message ID 20190416153847.8173-1-ming.lei@redhat.com (mailing list archive)
State New, archived
Headers show
Series block: make sure that bvec length can't be overflowed | expand

Commit Message

Ming Lei April 16, 2019, 3:38 p.m. UTC
bvec->bv_offset may be bigger than PAGE_SIZE sometimes, such as,
when one bio is splitted in the middle of one bvec via bio_split(),
and bi_iter.bi_bvec_done is used to build offset of the 1st bvec of
remained bio.

So we have to make sure that every bvec's offset is less than
PAGE_SIZE from bio_for_each_segment().

This patch fixes this issue reported by Zhang Yi When running nvme/011.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Yi Zhang <yi.zhang@redhat.com>
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Fixes: 6dc4f100c175 ("block: allow bio_for_each_segment_all() to iterate over multi-page bvec")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/bvec.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig April 16, 2019, 4:46 p.m. UTC | #1
s/overflowed/overflow/ in the subject.

Otherwise this looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Although this will create yet another conflict between Linus' tree
and the 5.2 block tree.

Although maybe Jens still reset the tree and move the merge past this..
Jens Axboe April 16, 2019, 5:03 p.m. UTC | #2
On 4/16/19 10:46 AM, Christoph Hellwig wrote:
> s/overflowed/overflow/ in the subject.
> 
> Otherwise this looks good to me:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Although this will create yet another conflict between Linus' tree
> and the 5.2 block tree.
> 
> Although maybe Jens still reset the tree and move the merge past this..

That was my main worry too... But I'll probably just deal with it and
redo the merge, I don't want to do two, and I don't want to have to
deal with answering for why we get repeated conflicts in this area.

For the patch, I do generally prefer doing an AND with PAGE_MASK
rather than a modulo with PAGE_SIZE. I know the compiler will take
care of it, but still, it's more prudent imho.
Ming Lei April 17, 2019, 12:48 a.m. UTC | #3
On Tue, Apr 16, 2019 at 11:03:00AM -0600, Jens Axboe wrote:
> On 4/16/19 10:46 AM, Christoph Hellwig wrote:
> > s/overflowed/overflow/ in the subject.
> > 
> > Otherwise this looks good to me:
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > Although this will create yet another conflict between Linus' tree
> > and the 5.2 block tree.
> > 
> > Although maybe Jens still reset the tree and move the merge past this..
> 
> That was my main worry too... But I'll probably just deal with it and
> redo the merge, I don't want to do two, and I don't want to have to
> deal with answering for why we get repeated conflicts in this area.

This one is a fix which should affect on loop and nvme-loop only in theory.

That is typical conflict between fix and feature(improvement).

Anyway, sorry for the a bit late fix.

> 
> For the patch, I do generally prefer doing an AND with PAGE_MASK
> rather than a modulo with PAGE_SIZE. I know the compiler will take
> care of it, but still, it's more prudent imho.

OK, do it in V2.


Thanks,
Ming
Hannes Reinecke April 17, 2019, 11:52 a.m. UTC | #4
On 4/16/19 5:38 PM, Ming Lei wrote:
> bvec->bv_offset may be bigger than PAGE_SIZE sometimes, such as,
> when one bio is splitted in the middle of one bvec via bio_split(),
> and bi_iter.bi_bvec_done is used to build offset of the 1st bvec of
> remained bio.
> 
> So we have to make sure that every bvec's offset is less than
> PAGE_SIZE from bio_for_each_segment().
> 
> This patch fixes this issue reported by Zhang Yi When running nvme/011.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Yi Zhang <yi.zhang@redhat.com>
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Fixes: 6dc4f100c175 ("block: allow bio_for_each_segment_all() to iterate over multi-page bvec")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   include/linux/bvec.h | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index 3bc91879e1e2..f179b370066f 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -160,8 +160,9 @@ static inline void bvec_advance(const struct bio_vec *bvec,
>   		bv->bv_page = nth_page(bv->bv_page, 1);
>   		bv->bv_offset = 0;
>   	} else {
> -		bv->bv_page = bvec->bv_page;
> -		bv->bv_offset = bvec->bv_offset;
> +		bv->bv_page = bvec_nth_page(bvec->bv_page, bvec->bv_offset /
> +					    PAGE_SIZE);
> +		bv->bv_offset = bvec->bv_offset % PAGE_SIZE;
>   	}
>   	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
>   			   bvec->bv_len - iter_all->done);
> 
Looks okay.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
diff mbox series

Patch

diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index 3bc91879e1e2..f179b370066f 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -160,8 +160,9 @@  static inline void bvec_advance(const struct bio_vec *bvec,
 		bv->bv_page = nth_page(bv->bv_page, 1);
 		bv->bv_offset = 0;
 	} else {
-		bv->bv_page = bvec->bv_page;
-		bv->bv_offset = bvec->bv_offset;
+		bv->bv_page = bvec_nth_page(bvec->bv_page, bvec->bv_offset /
+					    PAGE_SIZE);
+		bv->bv_offset = bvec->bv_offset % PAGE_SIZE;
 	}
 	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
 			   bvec->bv_len - iter_all->done);