[PATCH] Btrfs: fix btrfs_decompress_buf2page()

[PATCH] Btrfs: fix btrfs_decompress_buf2page()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

If btrfs_decompress_buf2page() is handed a bio with its page in the
middle of the working buffer, then we adjust the offset into the working
buffer. After we copy into the bio, we advance the iterator by the
number of bytes we copied. Then, we have some logic to handle the case
of discontiguous pages and adjust the offset into the working buffer
again. However, if we didn't advance the bio to a new page, we may enter
this case in error, essentially repeating the adjustment that we already
made when we entered the function. The end result is bogus data in the
bio.

Previously, we only checked for this case when we advanced to a new
page, but the conversion to bio iterators changed that. This restores
the old, correct behavior.

Fixes: 974b1adc3b10 ("btrfs: use bio iterators for the decompression handlers")
Reported-by: Pat Erley <pat-lkml@erley.org>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
A case I saw when testing with zlib was:

    buf_start = 42769
    total_out = 46865
    working_bytes = total_out - buf_start = 4096
    start_byte = 45056


The condition (total_out > start_byte && buf_start < start_byte) is
true, so we adjust the offset:

    buf_offset = start_byte - buf_start = 2287
    working_bytes -= buf_offset = 1809
    current_buf_start = buf_start = 42769

Then, we copy

    bytes = min(bvec.bv_len, PAGE_SIZE - buf_offset, working_bytes) = 1809
    buf_offset += bytes = 4096
    working_bytes -= bytes = 0
    current_buf_start += bytes = 44578

After bio_advance(), we are still in the same page, so start_byte is the
same. Then, we check (total_out > start_byte && current_buf_start < start_byte),
which is true! So, we adjust the values again:

    buf_offset = start_byte - buf_start = 2287
    working_bytes = total_out - start_byte = 1809
    current_buf_start = buf_start + buf_offset = 45056

But note that working_bytes was already zero before this, so we should
have stopped copying.

 fs/btrfs/compression.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 7f390849343b..f9f22976d77d 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1072,25 +1072,27 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
 			return 0;
 		bvec = bio_iter_iovec(bio, bio->bi_iter);
 
-		start_byte = page_offset(bvec.bv_page) - disk_start;
+		if (bvec.bv_offset == 0) {
+			start_byte = page_offset(bvec.bv_page) - disk_start;
 
-		/*
-		 * make sure our new page is covered by this
-		 * working buffer
-		 */
-		if (total_out <= start_byte)
-			return 1;
+			/*
+			 * make sure our new page is covered by this
+			 * working buffer
+			 */
+			if (total_out <= start_byte)
+				return 1;
 
-		/*
-		 * the next page in the biovec might not be adjacent
-		 * to the last page, but it might still be found
-		 * inside this working buffer. bump our offset pointer
-		 */
-		if (total_out > start_byte &&
-		    current_buf_start < start_byte) {
-			buf_offset = start_byte - buf_start;
-			working_bytes = total_out - start_byte;
-			current_buf_start = buf_start + buf_offset;
+			/*
+			 * the next page in the biovec might not be adjacent
+			 * to the last page, but it might still be found
+			 * inside this working buffer. bump our offset pointer
+			 */
+			if (total_out > start_byte &&
+			    current_buf_start < start_byte) {
+				buf_offset = start_byte - buf_start;
+				working_bytes = total_out - start_byte;
+				current_buf_start = buf_start + buf_offset;
+			}
 		}
 	}
 
-- 
2.11.1

Re: [PATCH] Btrfs: fix btrfs_decompress_buf2page()

[Liu Bo]

On Fri, Feb 10, 2017 at 12:15:11PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> If btrfs_decompress_buf2page() is handed a bio with its page in the
> middle of the working buffer, then we adjust the offset into the working
> buffer. After we copy into the bio, we advance the iterator by the
> number of bytes we copied. Then, we have some logic to handle the case
> of discontiguous pages and adjust the offset into the working buffer
> again. However, if we didn't advance the bio to a new page, we may enter
> this case in error, essentially repeating the adjustment that we already
> made when we entered the function. The end result is bogus data in the
> bio.
> 
> Previously, we only checked for this case when we advanced to a new
> page, but the conversion to bio iterators changed that. This restores
> the old, correct behavior.

The fix looks good to me, just one comment below.

> 
> Fixes: 974b1adc3b10 ("btrfs: use bio iterators for the decompression handlers")
> Reported-by: Pat Erley <pat-lkml@erley.org>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> A case I saw when testing with zlib was:
> 
>     buf_start = 42769
>     total_out = 46865
>     working_bytes = total_out - buf_start = 4096
>     start_byte = 45056
> 
> 
> The condition (total_out > start_byte && buf_start < start_byte) is
> true, so we adjust the offset:
> 
>     buf_offset = start_byte - buf_start = 2287
>     working_bytes -= buf_offset = 1809
>     current_buf_start = buf_start = 42769
> 
> Then, we copy
> 
>     bytes = min(bvec.bv_len, PAGE_SIZE - buf_offset, working_bytes) = 1809
>     buf_offset += bytes = 4096
>     working_bytes -= bytes = 0
>     current_buf_start += bytes = 44578
> 
> After bio_advance(), we are still in the same page, so start_byte is the
> same. Then, we check (total_out > start_byte && current_buf_start < start_byte),
> which is true! So, we adjust the values again:
> 
>     buf_offset = start_byte - buf_start = 2287
>     working_bytes = total_out - start_byte = 1809
>     current_buf_start = buf_start + buf_offset = 45056
> 
> But note that working_bytes was already zero before this, so we should
> have stopped copying.
> 
>  fs/btrfs/compression.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 7f390849343b..f9f22976d77d 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -1072,25 +1072,27 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
>  			return 0;
>  		bvec = bio_iter_iovec(bio, bio->bi_iter);
>  
> -		start_byte = page_offset(bvec.bv_page) - disk_start;
> +		if (bvec.bv_offset == 0) {
> +			start_byte = page_offset(bvec.bv_page) - disk_start;

I'm not fully convinced that the next bvec's bv_offset is always
zero, since the pages are all locked, can we keep a orig_page and
check if (orig_page == bvec.bv_page)?

Thanks,

-liubo
>  
> -		/*
> -		 * make sure our new page is covered by this
> -		 * working buffer
> -		 */
> -		if (total_out <= start_byte)
> -			return 1;
> +			/*
> +			 * make sure our new page is covered by this
> +			 * working buffer
> +			 */
> +			if (total_out <= start_byte)
> +				return 1;
>  
> -		/*
> -		 * the next page in the biovec might not be adjacent
> -		 * to the last page, but it might still be found
> -		 * inside this working buffer. bump our offset pointer
> -		 */
> -		if (total_out > start_byte &&
> -		    current_buf_start < start_byte) {
> -			buf_offset = start_byte - buf_start;
> -			working_bytes = total_out - start_byte;
> -			current_buf_start = buf_start + buf_offset;
> +			/*
> +			 * the next page in the biovec might not be adjacent
> +			 * to the last page, but it might still be found
> +			 * inside this working buffer. bump our offset pointer
> +			 */
> +			if (total_out > start_byte &&
> +			    current_buf_start < start_byte) {
> +				buf_offset = start_byte - buf_start;
> +				working_bytes = total_out - start_byte;
> +				current_buf_start = buf_start + buf_offset;
> +			}
>  		}
>  	}
>  
> -- 
> 2.11.1
> 

Re: [PATCH] Btrfs: fix btrfs_decompress_buf2page()

[Omar Sandoval]

On Fri, Feb 10, 2017 at 02:46:09PM -0800, Liu Bo wrote:
> On Fri, Feb 10, 2017 at 12:15:11PM -0800, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > If btrfs_decompress_buf2page() is handed a bio with its page in the
> > middle of the working buffer, then we adjust the offset into the working
> > buffer. After we copy into the bio, we advance the iterator by the
> > number of bytes we copied. Then, we have some logic to handle the case
> > of discontiguous pages and adjust the offset into the working buffer
> > again. However, if we didn't advance the bio to a new page, we may enter
> > this case in error, essentially repeating the adjustment that we already
> > made when we entered the function. The end result is bogus data in the
> > bio.
> > 
> > Previously, we only checked for this case when we advanced to a new
> > page, but the conversion to bio iterators changed that. This restores
> > the old, correct behavior.
> 
> The fix looks good to me, just one comment below.
> 
> > 
> > Fixes: 974b1adc3b10 ("btrfs: use bio iterators for the decompression handlers")
> > Reported-by: Pat Erley <pat-lkml@erley.org>
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> > A case I saw when testing with zlib was:
> > 
> >     buf_start = 42769
> >     total_out = 46865
> >     working_bytes = total_out - buf_start = 4096
> >     start_byte = 45056
> > 
> > 
> > The condition (total_out > start_byte && buf_start < start_byte) is
> > true, so we adjust the offset:
> > 
> >     buf_offset = start_byte - buf_start = 2287
> >     working_bytes -= buf_offset = 1809
> >     current_buf_start = buf_start = 42769
> > 
> > Then, we copy
> > 
> >     bytes = min(bvec.bv_len, PAGE_SIZE - buf_offset, working_bytes) = 1809
> >     buf_offset += bytes = 4096
> >     working_bytes -= bytes = 0
> >     current_buf_start += bytes = 44578
> > 
> > After bio_advance(), we are still in the same page, so start_byte is the
> > same. Then, we check (total_out > start_byte && current_buf_start < start_byte),
> > which is true! So, we adjust the values again:
> > 
> >     buf_offset = start_byte - buf_start = 2287
> >     working_bytes = total_out - start_byte = 1809
> >     current_buf_start = buf_start + buf_offset = 45056
> > 
> > But note that working_bytes was already zero before this, so we should
> > have stopped copying.
> > 
> >  fs/btrfs/compression.c | 36 +++++++++++++++++++-----------------
> >  1 file changed, 19 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> > index 7f390849343b..f9f22976d77d 100644
> > --- a/fs/btrfs/compression.c
> > +++ b/fs/btrfs/compression.c
> > @@ -1072,25 +1072,27 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
> >  			return 0;
> >  		bvec = bio_iter_iovec(bio, bio->bi_iter);
> >  
> > -		start_byte = page_offset(bvec.bv_page) - disk_start;
> > +		if (bvec.bv_offset == 0) {
> > +			start_byte = page_offset(bvec.bv_page) - disk_start;
> 
> I'm not fully convinced that the next bvec's bv_offset is always
> zero, since the pages are all locked, can we keep a orig_page and
> check if (orig_page == bvec.bv_page)?

That's a good point, that's more foolproof. I'll send a v2.

Re: [PATCH] Btrfs: fix btrfs_decompress_buf2page()

[Liu Bo]

On Fri, Feb 10, 2017 at 02:52:11PM -0800, Omar Sandoval wrote:
> On Fri, Feb 10, 2017 at 02:46:09PM -0800, Liu Bo wrote:
> > On Fri, Feb 10, 2017 at 12:15:11PM -0800, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > > 
> > > If btrfs_decompress_buf2page() is handed a bio with its page in the
> > > middle of the working buffer, then we adjust the offset into the working
> > > buffer. After we copy into the bio, we advance the iterator by the
> > > number of bytes we copied. Then, we have some logic to handle the case
> > > of discontiguous pages and adjust the offset into the working buffer
> > > again. However, if we didn't advance the bio to a new page, we may enter
> > > this case in error, essentially repeating the adjustment that we already
> > > made when we entered the function. The end result is bogus data in the
> > > bio.
> > > 
> > > Previously, we only checked for this case when we advanced to a new
> > > page, but the conversion to bio iterators changed that. This restores
> > > the old, correct behavior.
> > 
> > The fix looks good to me, just one comment below.
> > 
> > > 
> > > Fixes: 974b1adc3b10 ("btrfs: use bio iterators for the decompression handlers")
> > > Reported-by: Pat Erley <pat-lkml@erley.org>
> > > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > > ---
> > > A case I saw when testing with zlib was:
> > > 
> > >     buf_start = 42769
> > >     total_out = 46865
> > >     working_bytes = total_out - buf_start = 4096
> > >     start_byte = 45056
> > > 
> > > 
> > > The condition (total_out > start_byte && buf_start < start_byte) is
> > > true, so we adjust the offset:
> > > 
> > >     buf_offset = start_byte - buf_start = 2287
> > >     working_bytes -= buf_offset = 1809
> > >     current_buf_start = buf_start = 42769
> > > 
> > > Then, we copy
> > > 
> > >     bytes = min(bvec.bv_len, PAGE_SIZE - buf_offset, working_bytes) = 1809
> > >     buf_offset += bytes = 4096
> > >     working_bytes -= bytes = 0
> > >     current_buf_start += bytes = 44578
> > > 
> > > After bio_advance(), we are still in the same page, so start_byte is the
> > > same. Then, we check (total_out > start_byte && current_buf_start < start_byte),
> > > which is true! So, we adjust the values again:
> > > 
> > >     buf_offset = start_byte - buf_start = 2287
> > >     working_bytes = total_out - start_byte = 1809
> > >     current_buf_start = buf_start + buf_offset = 45056
> > > 
> > > But note that working_bytes was already zero before this, so we should
> > > have stopped copying.
> > > 
> > >  fs/btrfs/compression.c | 36 +++++++++++++++++++-----------------
> > >  1 file changed, 19 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> > > index 7f390849343b..f9f22976d77d 100644
> > > --- a/fs/btrfs/compression.c
> > > +++ b/fs/btrfs/compression.c
> > > @@ -1072,25 +1072,27 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
> > >  			return 0;
> > >  		bvec = bio_iter_iovec(bio, bio->bi_iter);
> > >  
> > > -		start_byte = page_offset(bvec.bv_page) - disk_start;
> > > +		if (bvec.bv_offset == 0) {
> > > +			start_byte = page_offset(bvec.bv_page) - disk_start;
> > 
> > I'm not fully convinced that the next bvec's bv_offset is always
> > zero, since the pages are all locked, can we keep a orig_page and
> > check if (orig_page == bvec.bv_page)?
> 
> That's a good point, that's more foolproof. I'll send a v2.

With that, you can have

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Tested-by: Liu Bo <bo.li.liu@oracle.com>

We may add ASSERT(bvec.bv_offset == 0) in the if statement of
(orig_page == bvec.bv_page), then we could know whether it's true :)

Thanks,

-liubo