linux-kernel - Re: [PATCH v3 1/5] btrfs: replace stripe extents

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] [thread-next>] [day] [month] [year] [list]

Date: Mon, 1 Jul 2024 15:08:22 +0000
From: Johannes Thumshirn <Johannes.Thumshirn@....com>
To: Josef Bacik <josef@...icpanda.com>, Johannes Thumshirn <jth@...nel.org>
CC: Chris Mason <clm@...com>, David Sterba <dsterba@...e.com>,
	"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/5] btrfs: replace stripe extents

On 01.07.24 15:58, Josef Bacik wrote:
> On Mon, Jul 01, 2024 at 12:25:15PM +0200, Johannes Thumshirn wrote:
>> From: Johannes Thumshirn <johannes.thumshirn@....com>
>>
>> If we can't insert a stripe extent in the RAID stripe tree, because
>> the key that points to the specific position in the stripe tree is
>> already existing, we have to remove the item and then replace it by a
>> new item.
>>
>> This can happen for example on device replace operations.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@....com>
>> ---
>>   fs/btrfs/raid-stripe-tree.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
>> index e6f7a234b8f6..3020820dd6e2 100644
>> --- a/fs/btrfs/raid-stripe-tree.c
>> +++ b/fs/btrfs/raid-stripe-tree.c
>> @@ -73,6 +73,37 @@ int btrfs_delete_raid_extent(struct btrfs_trans_handle *trans, u64 start, u64 le
>>   	return ret;
>>   }
>>   
>> +static int replace_raid_extent_item(struct btrfs_trans_handle *trans,
>> +				    struct btrfs_key *key,
>> +				    struct btrfs_stripe_extent *stripe_extent,
>> +				    const size_t item_size)
>> +{
>> +	struct btrfs_fs_info *fs_info = trans->fs_info;
>> +	struct btrfs_root *stripe_root = fs_info->stripe_root;
>> +	struct btrfs_path *path;
>> +	int ret;
>> +
>> +	path = btrfs_alloc_path();
>> +	if (!path)
>> +		return -ENOMEM;
>> +
>> +	ret = btrfs_search_slot(trans, stripe_root, key, path, -1, 1);
>> +	if (ret)
>> +		goto err;
> 
> This will leak 1 and we'll get an awkward btrfs_abort_transaction() call.  This
> should be
> 
> if (ret) {
> 	ret = (ret == 1) ? -ENOENT : ret;
> 	goto err;
> }
> 
> or whatever.  Thanks,

I wonder why I've never seen this in my testing. Could it be, that due 
to the fact that btrfs_insert_item() returns -EEXIST on the same 
key.objectid, we're more or less guaranteed it'll exist.