| Message ID | 20190408042756.25969-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: tree-checker: Don't check device size as we can have valid 0 total_bytes | expand |
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, >
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.
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,
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(-)