linux-kernel - Re: [PATCH] btrfs: stripe-tree: correctly truncate stripe extents on delete

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240821171545.GA1998418@perftesting>
Date: Wed, 21 Aug 2024 13:15:45 -0400
From: Josef Bacik <josef@...icpanda.com>
To: Johannes Thumshirn <jth@...nel.org>
Cc: Chris Mason <clm@...com>, David Sterba <dsterba@...e.com>,
	"open list:BTRFS FILE SYSTEM" <linux-btrfs@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	Johannes Thumshirn <johannes.thumshirn@....com>
Subject: Re: [PATCH] btrfs: stripe-tree: correctly truncate stripe extents on
 delete

On Tue, Aug 20, 2024 at 04:34:33PM +0200, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@....com>
> 
> In our CI system, we're seeing the following ASSERT()ion to trigger when
> running RAID stripe-tree tests on non-zoned devices:
> 
>  assertion failed: found_start >= start && found_end <= end, in fs/btrfs/raid-stripe-tree.c:64
> 
> This ASSERT()ion triggers, because for the initial design of RAID stripe-tree,
> I had the "one ordered-extent equals one bio" rule of zoned btrfs in mind.
> 
> But for a RAID stripe-tree based system, that is not hosted on a zoned
> storage device, but on a regular device this rule doesn't apply.
> 
> So in case the range we want to delete starts in the middle of the
> previous item, grab the item and "truncate" it's length. That is, subtract
> the deleted portion from the key's offset.
> 
> In case the range to delete ends in the middle of an item, we have to
> adjust both the item's key as well as the stripe extents.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@....com>
> ---
>  fs/btrfs/raid-stripe-tree.c | 50 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
> index 4c859b550f6c..c8365d14271f 100644
> --- a/fs/btrfs/raid-stripe-tree.c
> +++ b/fs/btrfs/raid-stripe-tree.c
> @@ -61,7 +61,55 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>  		trace_btrfs_raid_extent_delete(fs_info, start, end,
>  					       found_start, found_end);
>  
> -		ASSERT(found_start >= start && found_end <= end);
> +		if (found_start < start) {
> +			struct btrfs_key prev;
> +			u64 diff = start - found_start;
> +
> +			ret = btrfs_previous_item(stripe_root, path, start,
> +						  BTRFS_RAID_STRIPE_KEY);

This is only safe if we're not path->slots[0] == 0, otherwise we'll do
btrfs_prev_leaf(), which doesn't modify anything, adn then we'll be in trouble.
If this is safe then a comment indicating why we expect this to only back up one
slot, and maybe an

ASSERT(path->slots[0] > 0);

before the btrfs_previous_item to make sure we don't screw this up later.
Thanks,

Josef