Commit 6e78b3f7 authored Feb 10, 2017 by Omar Sandoval Committed by Chris Mason Feb 10, 2017

Btrfs: fix btrfs_decompress_buf2page()



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.

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.

Fixes: 974b1adc ("btrfs: use bio iterators for the decompression handlers")
Reported-by: Pat Erley <pat-lkml@erley.org>
Reviewed-by: Chris Mason <clm@fb.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
Signed-off-by: Chris Mason <clm@fb.com>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Tested-by: Liu Bo <bo.li.liu@oracle.com>

parent f3c7bfbd

fs/btrfs/compression.c

+24 −15

Original line number	Original line	Diff line number	Diff line
	@@ -1024,6 +1024,7 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
	unsigned long buf_offset;		unsigned long buf_offset;
	unsigned long current_buf_start;		unsigned long current_buf_start;
	unsigned long start_byte;		unsigned long start_byte;
			unsigned long prev_start_byte;
	unsigned long working_bytes = total_out - buf_start;		unsigned long working_bytes = total_out - buf_start;
	unsigned long bytes;		unsigned long bytes;
	char *kaddr;		char *kaddr;
	@@ -1071,9 +1072,16 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
	if (!bio->bi_iter.bi_size)		if (!bio->bi_iter.bi_size)
	return 0;		return 0;
	bvec = bio_iter_iovec(bio, bio->bi_iter);		bvec = bio_iter_iovec(bio, bio->bi_iter);
			prev_start_byte = start_byte;
	start_byte = page_offset(bvec.bv_page) - disk_start;		start_byte = page_offset(bvec.bv_page) - disk_start;

			/*
			* We need to make sure we're only adjusting
			* our offset into compression working buffer when
			* we're switching pages. Otherwise we can incorrectly
			* keep copying when we were actually done.
			*/
			if (start_byte != prev_start_byte) {
	/*		/*
	* make sure our new page is covered by this		* make sure our new page is covered by this
	* working buffer		* working buffer
	@@ -1093,6 +1101,7 @@ int btrfs_decompress_buf2page(char *buf, unsigned long buf_start,
	current_buf_start = buf_start + buf_offset;		current_buf_start = buf_start + buf_offset;
	}		}
	}		}
			}

	return 1;		return 1;
	}		}

Admin message