[PATCH 1/3] btrfs: return errno instead of -1 from compression

[PATCH 1/3] btrfs: return errno instead of -1 from compression

[Zach Brown]

The compression layer seems to have been built to return -1 and have
callers make up errors that make sense.  This isn't great because there
are different classes of errors that originate down in the compression
layer.  Allocation failure and corrupt compressed data to name two.

So let's return real negative errnos from the compression layer so that
callers can pass on the error without having to guess what happened.

This helps a future path return errors from btrfs_decompress().

Signed-off-by: Zach Brown <zab@redhat.com>
---
 fs/btrfs/lzo.c  | 14 +++++++-------
 fs/btrfs/zlib.c | 26 +++++++++++++-------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index b47f669..24f7ede 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -143,7 +143,7 @@ static int lzo_compress_pages(struct list_head *ws,
 		if (ret != LZO_E_OK) {
 			printk(KERN_DEBUG "BTRFS: deflate in loop returned %d\n",
 			       ret);
-			ret = -1;
+			ret = -EIO;
 			goto out;
 		}
 
@@ -189,7 +189,7 @@ static int lzo_compress_pages(struct list_head *ws,
 				kunmap(out_page);
 				if (nr_pages == nr_dest_pages) {
 					out_page = NULL;
-					ret = -1;
+					ret = -EIO;
 					goto out;
 				}
 
@@ -208,7 +208,7 @@ static int lzo_compress_pages(struct list_head *ws,
 
 		/* we're making it bigger, give up */
 		if (tot_in > 8192 && tot_in < tot_out) {
-			ret = -1;
+			ret = -EIO;
 			goto out;
 		}
 
@@ -335,7 +335,7 @@ cont:
 					break;
 
 				if (page_in_index + 1 >= total_pages_in) {
-					ret = -1;
+					ret = -EIO;
 					goto done;
 				}
 
@@ -358,7 +358,7 @@ cont:
 			kunmap(pages_in[page_in_index - 1]);
 		if (ret != LZO_E_OK) {
 			printk(KERN_WARNING "BTRFS: decompress failed\n");
-			ret = -1;
+			ret = -EIO;
 			break;
 		}
 
@@ -402,12 +402,12 @@ static int lzo_decompress(struct list_head *ws, unsigned char *data_in,
 	ret = lzo1x_decompress_safe(data_in, in_len, workspace->buf, &out_len);
 	if (ret != LZO_E_OK) {
 		printk(KERN_WARNING "BTRFS: decompress failed!\n");
-		ret = -1;
+		ret = -EIO;
 		goto out;
 	}
 
 	if (out_len < start_byte) {
-		ret = -1;
+		ret = -EIO;
 		goto out;
 	}
 
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 8e57191..f14c4a0 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -98,7 +98,7 @@ static int zlib_compress_pages(struct list_head *ws,
 
 	if (Z_OK != zlib_deflateInit(&workspace->def_strm, 3)) {
 		printk(KERN_WARNING "BTRFS: deflateInit failed\n");
-		ret = -1;
+		ret = -EIO;
 		goto out;
 	}
 
@@ -110,7 +110,7 @@ static int zlib_compress_pages(struct list_head *ws,
 
 	out_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
 	if (out_page == NULL) {
-		ret = -1;
+		ret = -ENOMEM;
 		goto out;
 	}
 	cpage_out = kmap(out_page);
@@ -128,7 +128,7 @@ static int zlib_compress_pages(struct list_head *ws,
 			printk(KERN_DEBUG "BTRFS: deflate in loop returned %d\n",
 			       ret);
 			zlib_deflateEnd(&workspace->def_strm);
-			ret = -1;
+			ret = -EIO;
 			goto out;
 		}
 
@@ -136,7 +136,7 @@ static int zlib_compress_pages(struct list_head *ws,
 		if (workspace->def_strm.total_in > 8192 &&
 		    workspace->def_strm.total_in <
 		    workspace->def_strm.total_out) {
-			ret = -1;
+			ret = -EIO;
 			goto out;
 		}
 		/* we need another page for writing out.  Test this
@@ -147,12 +147,12 @@ static int zlib_compress_pages(struct list_head *ws,
 			kunmap(out_page);
 			if (nr_pages == nr_dest_pages) {
 				out_page = NULL;
-				ret = -1;
+				ret = -EIO;
 				goto out;
 			}
 			out_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
 			if (out_page == NULL) {
-				ret = -1;
+				ret = -ENOMEM;
 				goto out;
 			}
 			cpage_out = kmap(out_page);
@@ -188,12 +188,12 @@ static int zlib_compress_pages(struct list_head *ws,
 	zlib_deflateEnd(&workspace->def_strm);
 
 	if (ret != Z_STREAM_END) {
-		ret = -1;
+		ret = -EIO;
 		goto out;
 	}
 
 	if (workspace->def_strm.total_out >= workspace->def_strm.total_in) {
-		ret = -1;
+		ret = -EIO;
 		goto out;
 	}
 
@@ -253,7 +253,7 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
 
 	if (Z_OK != zlib_inflateInit2(&workspace->inf_strm, wbits)) {
 		printk(KERN_WARNING "BTRFS: inflateInit failed\n");
-		return -1;
+		return -EIO;
 	}
 	while (workspace->inf_strm.total_in < srclen) {
 		ret = zlib_inflate(&workspace->inf_strm, Z_NO_FLUSH);
@@ -295,7 +295,7 @@ static int zlib_decompress_biovec(struct list_head *ws, struct page **pages_in,
 		}
 	}
 	if (ret != Z_STREAM_END)
-		ret = -1;
+		ret = -EIO;
 	else
 		ret = 0;
 done:
@@ -337,7 +337,7 @@ static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
 
 	if (Z_OK != zlib_inflateInit2(&workspace->inf_strm, wbits)) {
 		printk(KERN_WARNING "BTRFS: inflateInit failed\n");
-		return -1;
+		return -EIO;
 	}
 
 	while (bytes_left > 0) {
@@ -354,7 +354,7 @@ static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
 		total_out = workspace->inf_strm.total_out;
 
 		if (total_out == buf_start) {
-			ret = -1;
+			ret = -EIO;
 			break;
 		}
 
@@ -382,7 +382,7 @@ next:
 	}
 
 	if (ret != Z_STREAM_END && bytes_left != 0)
-		ret = -1;
+		ret = -EIO;
 	else
 		ret = 0;
 
-- 
1.8.1.4

[PATCH 2/3] btrfs: return ptr error from compression workspace

[Zach Brown]

The btrfs compression wrappers translated errors from workspace
allocation to either -ENOMEM or -1.  The compression type workspace
allocators are already returning a ERR_PTR(-ENOMEM).  Just return that
and get rid of the magical -1.

This helps a future patch return errors from the compression wrappers.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 fs/btrfs/compression.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index ed1ff1cb..7912695 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -888,7 +888,7 @@ int btrfs_compress_pages(int type, struct address_space *mapping,
 
 	workspace = find_workspace(type);
 	if (IS_ERR(workspace))
-		return -1;
+		return PTR_ERR(workspace);
 
 	ret = btrfs_compress_op[type-1]->compress_pages(workspace, mapping,
 						      start, len, pages,
@@ -924,7 +924,7 @@ static int btrfs_decompress_biovec(int type, struct page **pages_in,
 
 	workspace = find_workspace(type);
 	if (IS_ERR(workspace))
-		return -ENOMEM;
+		return PTR_ERR(workspace);
 
 	ret = btrfs_compress_op[type-1]->decompress_biovec(workspace, pages_in,
 							 disk_start,
@@ -946,7 +946,7 @@ int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page,
 
 	workspace = find_workspace(type);
 	if (IS_ERR(workspace))
-		return -ENOMEM;
+		return PTR_ERR(workspace);
 
 	ret = btrfs_compress_op[type-1]->decompress(workspace, data_in,
 						  dest_page, start_byte,
-- 
1.8.1.4

Re: [PATCH 2/3] btrfs: return ptr error from compression workspace

[David Sterba]

On Thu, May 08, 2014 at 07:16:18PM -0400, Zach Brown wrote:
> The btrfs compression wrappers translated errors from workspace
> allocation to either -ENOMEM or -1.  The compression type workspace
> allocators are already returning a ERR_PTR(-ENOMEM).  Just return that
> and get rid of the magical -1.
> 
> This helps a future patch return errors from the compression wrappers.
> 
> Signed-off-by: Zach Brown <zab@redhat.com>
Reviewed-by: David Sterba <dsterba@suse.cz>

[PATCH 3/3] btrfs: fix inline compressed read err corruption

[Zach Brown]

uncompress_inline() is silently dropping an error from
btrfs_decompress() after testing it and zeroing the page that was
supposed to hold decompressed data.  This can silently turn compressed
inline data in to zeros if decompression fails due to corrupt compressed
data or memory allocation failure.

I have no idea why uncompress_inline() is zeroing the page for an error
from btrfs_decompress() but not for the earlier ENOMEM from kmalloc().
Can someone explain this?

The fix is to pass the error to its caller.  Which still has a BUG_ON().
So we fix that too.

I verified this by manually forcing the error from btrfs_decompress()
for a silly named copy of od:

	if (!strcmp(current->comm, "failod"))
		ret = -ENOMEM;

  # od -x /mnt/btrfs/dir/80 | head -1
  0000000 3031 3038 310a 2d30 6f70 6e69 0a74 3031
  # echo 3 > /proc/sys/vm/drop_caches
  # cp $(which od) /tmp/failod
  # /tmp/failod -x /mnt/btrfs/dir/80 | head -1
  0000000 0000 0000 0000 0000 0000 0000 0000 0000

Signed-off-by: Zach Brown <zab@redhat.com>
---
 fs/btrfs/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0c0bb45..fc89fa7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6091,7 +6091,7 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 		kunmap_atomic(kaddr);
 	}
 	kfree(tmp);
-	return 0;
+	return ret;
 }
 
 /*
@@ -6292,7 +6292,10 @@ next:
 				ret = uncompress_inline(path, inode, page,
 							pg_offset,
 							extent_offset, item);
-				BUG_ON(ret); /* -ENOMEM */
+				if (ret) {
+					err = ret;
+					goto out;
+				}
 			} else {
 				map = kmap(page);
 				read_extent_buffer(leaf, map + pg_offset, ptr,
-- 
1.8.1.4

Re: [PATCH 3/3] btrfs: fix inline compressed read err corruption

[David Sterba]

On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote:
> uncompress_inline() is silently dropping an error from
> btrfs_decompress() after testing it and zeroing the page that was
> supposed to hold decompressed data.  This can silently turn compressed
> inline data in to zeros if decompression fails due to corrupt compressed
> data or memory allocation failure.
> 
> I have no idea why uncompress_inline() is zeroing the page for an error
> from btrfs_decompress() but not for the earlier ENOMEM from kmalloc().
> Can someone explain this?

I don't see a reason for that in the current code, nor in the code that
introduced it. The changes that could have affected that are in the error
handling, but even with that in mind, the silent error does not make
sense.

> Signed-off-by: Zach Brown <zab@redhat.com>
Reviewed-by: David Sterba <dsterba@suse.cz>

And future ACK if you're going to kill the memset.

Re: [PATCH 3/3] btrfs: fix inline compressed read err corruption

[Zach Brown]

On Fri, May 09, 2014 at 03:58:00PM +0200, David Sterba wrote:
> On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote:
> > uncompress_inline() is silently dropping an error from
> > btrfs_decompress() after testing it and zeroing the page that was
> > supposed to hold decompressed data.  This can silently turn compressed
> > inline data in to zeros if decompression fails due to corrupt compressed
> > data or memory allocation failure.
> > 
> > I have no idea why uncompress_inline() is zeroing the page for an error
> > from btrfs_decompress() but not for the earlier ENOMEM from kmalloc().
> > Can someone explain this?
> 
> I don't see a reason for that in the current code, nor in the code that
> introduced it. The changes that could have affected that are in the error
> handling, but even with that in mind, the silent error does not make
> sense.

Agreed.  I removed it and will send out a v2 of the series.

- z

Re: [PATCH 3/3] btrfs: fix inline compressed read err corruption

[Liu Bo]

On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote:
> uncompress_inline() is silently dropping an error from
> btrfs_decompress() after testing it and zeroing the page that was
> supposed to hold decompressed data.  This can silently turn compressed
> inline data in to zeros if decompression fails due to corrupt compressed
> data or memory allocation failure.
> 
> I have no idea why uncompress_inline() is zeroing the page for an error
> from btrfs_decompress() but not for the earlier ENOMEM from kmalloc().
> Can someone explain this?

I guess that's because decompress() may have put part of real data on the page
and then bail out, and we don't want those data to be exposed to users in this
error case.

And kmalloc() 's ENOMEM runs before that decompress(), page has whatever random
data.

-liubo

> 
> The fix is to pass the error to its caller.  Which still has a BUG_ON().
> So we fix that too.
> 
> I verified this by manually forcing the error from btrfs_decompress()
> for a silly named copy of od:
> 
> 	if (!strcmp(current->comm, "failod"))
> 		ret = -ENOMEM;
> 
>   # od -x /mnt/btrfs/dir/80 | head -1
>   0000000 3031 3038 310a 2d30 6f70 6e69 0a74 3031
>   # echo 3 > /proc/sys/vm/drop_caches
>   # cp $(which od) /tmp/failod
>   # /tmp/failod -x /mnt/btrfs/dir/80 | head -1
>   0000000 0000 0000 0000 0000 0000 0000 0000 0000
> 
> Signed-off-by: Zach Brown <zab@redhat.com>
> ---
>  fs/btrfs/inode.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0c0bb45..fc89fa7 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6091,7 +6091,7 @@ static noinline int uncompress_inline(struct btrfs_path *path,
>  		kunmap_atomic(kaddr);
>  	}
>  	kfree(tmp);
> -	return 0;
> +	return ret;
>  }
>  
>  /*
> @@ -6292,7 +6292,10 @@ next:
>  				ret = uncompress_inline(path, inode, page,
>  							pg_offset,
>  							extent_offset, item);
> -				BUG_ON(ret); /* -ENOMEM */
> +				if (ret) {
> +					err = ret;
> +					goto out;
> +				}
>  			} else {
>  				map = kmap(page);
>  				read_extent_buffer(leaf, map + pg_offset, ptr,
> -- 
> 1.8.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] btrfs: fix inline compressed read err corruption

[David Sterba]

On Mon, May 12, 2014 at 11:00:23PM +0800, Liu Bo wrote:
> On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote:
> > uncompress_inline() is silently dropping an error from
> > btrfs_decompress() after testing it and zeroing the page that was
> > supposed to hold decompressed data.  This can silently turn compressed
> > inline data in to zeros if decompression fails due to corrupt compressed
> > data or memory allocation failure.
> > 
> > I have no idea why uncompress_inline() is zeroing the page for an error
> > from btrfs_decompress() but not for the earlier ENOMEM from kmalloc().
> > Can someone explain this?
> 
> I guess that's because decompress() may have put part of real data on the page
> and then bail out, and we don't want those data to be exposed to users in this
> error case.
> 
> And kmalloc() 's ENOMEM runs before that decompress(), page has whatever random
> data.

But we don't return any data in case of error. In the unpatched code,
there's no error so a zeroed page is returned, but this would not happen
after Zach's fix.

Re: [PATCH 3/3] btrfs: fix inline compressed read err corruption

[Chris Mason]

On 05/12/2014 01:18 PM, David Sterba wrote:
> On Mon, May 12, 2014 at 11:00:23PM +0800, Liu Bo wrote:
>> On Thu, May 08, 2014 at 07:16:19PM -0400, Zach Brown wrote:
>>> uncompress_inline() is silently dropping an error from
>>> btrfs_decompress() after testing it and zeroing the page that was
>>> supposed to hold decompressed data.  This can silently turn compressed
>>> inline data in to zeros if decompression fails due to corrupt compressed
>>> data or memory allocation failure.
>>>
>>> I have no idea why uncompress_inline() is zeroing the page for an error
>>> from btrfs_decompress() but not for the earlier ENOMEM from kmalloc().
>>> Can someone explain this?
>>
>> I guess that's because decompress() may have put part of real data on the page
>> and then bail out, and we don't want those data to be exposed to users in this
>> error case.
>>
>> And kmalloc() 's ENOMEM runs before that decompress(), page has whatever random
>> data.
> 
> But we don't return any data in case of error. In the unpatched code,
> there's no error so a zeroed page is returned, but this would not happen
> after Zach's fix.

I dug a little more, the zeroing goes all the way back to the original
compression code.  It looks like some paranoia of mine.

I'd say its fine to leave off as long as we don't mark the page uptodate.

-chris

Re: [PATCH 1/3] btrfs: return errno instead of -1 from compression

[David Sterba]

On Thu, May 08, 2014 at 07:16:17PM -0400, Zach Brown wrote:
> The compression layer seems to have been built to return -1 and have
> callers make up errors that make sense.  This isn't great because there
> are different classes of errors that originate down in the compression
> layer.  Allocation failure and corrupt compressed data to name two.
> 
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -143,7 +143,7 @@ static int lzo_compress_pages(struct list_head *ws,
>  		if (ret != LZO_E_OK) {
>  			printk(KERN_DEBUG "BTRFS: deflate in loop returned %d\n",
>  			       ret);
> -			ret = -1;
> +			ret = -EIO;
>  			goto out;
>  		}
>  
> @@ -189,7 +189,7 @@ static int lzo_compress_pages(struct list_head *ws,
>  				kunmap(out_page);
>  				if (nr_pages == nr_dest_pages) {
>  					out_page = NULL;
> -					ret = -1;
> +					ret = -EIO;

This is not a true EIO, the error conditions says that the caller
prepared nr_dest_pages for the compressed data but the compression wants
more.

The number of pages is at most 128k / PAGE_SIZE.

It's a soft error, the data are written uncompressed. The closest errno
here seems E2BIG that would apply in the following hunk as well.


>  					goto out;
>  				}
>  
> @@ -208,7 +208,7 @@ static int lzo_compress_pages(struct list_head *ws,
>  
>  		/* we're making it bigger, give up */
>  		if (tot_in > 8192 && tot_in < tot_out) {
> -			ret = -1;
> +			ret = -EIO;

Here, E2BIG.

>  			goto out;
>  		}
>  
> @@ -335,7 +335,7 @@ cont:
>  					break;
>  
>  				if (page_in_index + 1 >= total_pages_in) {
> -					ret = -1;
> +					ret = -EIO;

That looks like an internal error, we should never ask for more pages
than is in the input, so the buffer offset calculations are wrong.

>  					goto done;
>  				}
>  

Analogically the same applies to zlib. The rest of the EIOs look ok.

Re: [PATCH 1/3] btrfs: return errno instead of -1 from compression

[Zach Brown]

On Fri, May 09, 2014 at 03:39:26PM +0200, David Sterba wrote:
> On Thu, May 08, 2014 at 07:16:17PM -0400, Zach Brown wrote:
> > The compression layer seems to have been built to return -1 and have
> > callers make up errors that make sense.  This isn't great because there
> > are different classes of errors that originate down in the compression
> > layer.  Allocation failure and corrupt compressed data to name two.
> > 
> > --- a/fs/btrfs/lzo.c
> > +++ b/fs/btrfs/lzo.c
> > @@ -143,7 +143,7 @@ static int lzo_compress_pages(struct list_head *ws,
> >  		if (ret != LZO_E_OK) {
> >  			printk(KERN_DEBUG "BTRFS: deflate in loop returned %d\n",
> >  			       ret);
> > -			ret = -1;
> > +			ret = -EIO;
> >  			goto out;
> >  		}
> >  
> > @@ -189,7 +189,7 @@ static int lzo_compress_pages(struct list_head *ws,
> >  				kunmap(out_page);
> >  				if (nr_pages == nr_dest_pages) {
> >  					out_page = NULL;
> > -					ret = -1;
> > +					ret = -EIO;
> 
> This is not a true EIO, the error conditions says that the caller
> prepared nr_dest_pages for the compressed data but the compression wants
> more.
> 
> The number of pages is at most 128k / PAGE_SIZE.
> 
> It's a soft error, the data are written uncompressed. The closest errno
> here seems E2BIG that would apply in the following hunk as well.

Sure.  *Anything* is better than the raw -EPERM :).  I'll change these
and resend v2.

> > @@ -335,7 +335,7 @@ cont:
> >  					break;
> >  
> >  				if (page_in_index + 1 >= total_pages_in) {
> > -					ret = -1;
> > +					ret = -EIO;
> 
> That looks like an internal error, we should never ask for more pages
> than is in the input, so the buffer offset calculations are wrong.

Yeah, but EIO is still arguably the right thing.  There's nothing
userspace can do about broken kernel code.  We don't want to give them
an error that could be misinterpreted as them having used an interface
incorrectly.  We could wrap WARN_ON_ONCE() around it, I suppose.  I'm
inclined to leave it as is.

- z

Re: [PATCH 1/3] btrfs: return errno instead of -1 from compression

[David Sterba]

On Fri, May 09, 2014 at 01:40:15PM -0700, Zach Brown wrote:
> > > @@ -335,7 +335,7 @@ cont:
> > >  					break;
> > >  
> > >  				if (page_in_index + 1 >= total_pages_in) {
> > > -					ret = -1;
> > > +					ret = -EIO;
> > 
> > That looks like an internal error, we should never ask for more pages
> > than is in the input, so the buffer offset calculations are wrong.
> 
> Yeah, but EIO is still arguably the right thing.  There's nothing
> userspace can do about broken kernel code.  We don't want to give them
> an error that could be misinterpreted as them having used an interface
> incorrectly.  We could wrap WARN_ON_ONCE() around it, I suppose.  I'm
> inclined to leave it as is.

Ok. An EIO is better than an ASSERT or BUG_ON, the error paths will
handle it. I think adding the WARN_ON_ONCE is a good compromise, I'm not
expecting to see this error but the stack trace will help.