btrfs: tree-checker: Don't check device size as we can have valid 0 total_bytes
diff mbox series

Message ID 20190408042756.25969-1-wqu@suse.com
State New
Headers show
Series
  • btrfs: tree-checker: Don't check device size as we can have valid 0 total_bytes
Related show

Commit Message

Qu Wenruo April 8, 2019, 4:27 a.m. UTC
Under the following call traces, btrfs can commit device with 0
total_bytes onto disk:
btrfs_rm_device()
|- btrfs_shrink_device()
|  |- btrfs_device_set_total_bytes(device, 0)
|  |- btrfs_update_device()
|  |- btrfs_commit_transaction() #1
|- btrfs_rm_dev_item()

This will trigger write time tree checker warning.
And further more, this can create valid btrfs with device->total_bytes
== 0 and triggering read time tree-checker if power loss happens after
above transaction #1 but before next transaction.

So this dev item check is too restrict.

Please fold this patch into commit 87d87c6dcbbe ("btrfs: tree-checker:
Verify dev item") in misc-next branch.

The fuzzed image can be already addressed by commit 1b3922a8bc74
("btrfs: Use real device structure to verify dev extent") thus we don't
need that strict check anymore.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Comments

Qu Wenruo April 8, 2019, 4:34 a.m. UTC | #1
On 2019/4/8 下午12:27, Qu Wenruo wrote:
> Under the following call traces, btrfs can commit device with 0
> total_bytes onto disk:
> btrfs_rm_device()
> |- btrfs_shrink_device()
> |  |- btrfs_device_set_total_bytes(device, 0)
> |  |- btrfs_update_device()
> |  |- btrfs_commit_transaction() #1
> |- btrfs_rm_dev_item()
> 
> This will trigger write time tree checker warning.
> And further more, this can create valid btrfs with device->total_bytes
> == 0 and triggering read time tree-checker if power loss happens after
> above transaction #1 but before next transaction.
> 
> So this dev item check is too restrict.
> 
> Please fold this patch into commit 87d87c6dcbbe ("btrfs: tree-checker:
> Verify dev item") in misc-next branch.
> 
> The fuzzed image can be already addressed by commit 1b3922a8bc74
> ("btrfs: Use real device structure to verify dev extent") thus we don't
> need that strict check anymore.

Just a kind reminder, folding the patch will be much easier than my
original purpose to discard that patch completely.

Discarding causes at least 4 conflicts, and will be a pain in the ass.

Thanks,
Qu

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/tree-checker.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index d2c3c1f8870d..974208ac56da 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -657,16 +657,11 @@ static int check_dev_item(struct extent_buffer *leaf,
>  	}
>  
>  	/*
> -	 * Since btrfs device add doesn't check device size at all, we could
> -	 * have device item whose size is smaller than 1M which is useless, but
> -	 * still valid.
> -	 * So here we can only check the obviously wrong case.
> +	 * For device total_bytes, we don't have solid way to check it, as it can
> +	 * be 0 for device removal.
> +	 * device size check can only be done by dev extents check.
>  	 */
> -	if (btrfs_device_total_bytes(leaf, ditem) == 0) {
> -		dev_item_err(leaf, slot,
> -			     "invalid total bytes: have 0");
> -		return -EUCLEAN;
> -	}
> +
>  	if (btrfs_device_bytes_used(leaf, ditem) >
>  	    btrfs_device_total_bytes(leaf, ditem)) {
>  		dev_item_err(leaf, slot,
>
David Sterba April 8, 2019, 9:49 p.m. UTC | #2
On Mon, Apr 08, 2019 at 12:34:48PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/4/8 下午12:27, Qu Wenruo wrote:
> > Under the following call traces, btrfs can commit device with 0
> > total_bytes onto disk:
> > btrfs_rm_device()
> > |- btrfs_shrink_device()
> > |  |- btrfs_device_set_total_bytes(device, 0)
> > |  |- btrfs_update_device()
> > |  |- btrfs_commit_transaction() #1
> > |- btrfs_rm_dev_item()
> > 
> > This will trigger write time tree checker warning.
> > And further more, this can create valid btrfs with device->total_bytes
> > == 0 and triggering read time tree-checker if power loss happens after
> > above transaction #1 but before next transaction.
> > 
> > So this dev item check is too restrict.
> > 
> > Please fold this patch into commit 87d87c6dcbbe ("btrfs: tree-checker:
> > Verify dev item") in misc-next branch.
> > 
> > The fuzzed image can be already addressed by commit 1b3922a8bc74
> > ("btrfs: Use real device structure to verify dev extent") thus we don't
> > need that strict check anymore.
> 
> Just a kind reminder, folding the patch will be much easier than my
> original purpose to discard that patch completely.

Agreed and patch folded, thanks.

Patch
diff mbox series

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index d2c3c1f8870d..974208ac56da 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -657,16 +657,11 @@  static int check_dev_item(struct extent_buffer *leaf,
 	}
 
 	/*
-	 * Since btrfs device add doesn't check device size at all, we could
-	 * have device item whose size is smaller than 1M which is useless, but
-	 * still valid.
-	 * So here we can only check the obviously wrong case.
+	 * For device total_bytes, we don't have solid way to check it, as it can
+	 * be 0 for device removal.
+	 * device size check can only be done by dev extents check.
 	 */
-	if (btrfs_device_total_bytes(leaf, ditem) == 0) {
-		dev_item_err(leaf, slot,
-			     "invalid total bytes: have 0");
-		return -EUCLEAN;
-	}
+
 	if (btrfs_device_bytes_used(leaf, ditem) >
 	    btrfs_device_total_bytes(leaf, ditem)) {
 		dev_item_err(leaf, slot,