btrfs: return errno instead of -1 from compression

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 <[email protected]>

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?

Signed-off-by Zach Brown <[email protected]>

Quoted 6 lines · 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.

Quoted 3 lines

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.

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 <[email protected]>

---
 fs/btrfs/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

fs/btrfs/inode.c +5 -2

index 0c0bb45..fc89fa7 100644
@@ -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,

Signature · 6 lines

1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · 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?

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.

Quoted 15 lines · Liu Bo

Quoted 9 lines · 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?

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.

Quoted 2 lines

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.

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.

fs/btrfs/lzo.c +1 -1

@@ -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;
 		}
 

Quoted 17 lines · 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.

fs/btrfs/lzo.c +1 -1

@@ -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;
 		}
 

fs/btrfs/lzo.c static int lzo_compress_pages(struct list_head *ws, · line 192

Quoted 5 lines

fs/btrfs/lzo.c +1 -1

@@ -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.

fs/btrfs/lzo.c cont: · line 338

Quoted 6 lines

fs/btrfs/lzo.c +1 -1

@@ -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.

Quoted 9 lines

fs/btrfs/lzo.c cont: · line 338

Quoted 6 lines

fs/btrfs/lzo.c +1 -1

@@ -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.