Re: [PATCH v2 1/2] btrfs: harden identification of the stale device

[Su Yue <l@damenly.su>]

On Wed 05 Jan 2022 at 19:31, Anand Jain <anand.jain@oracle.com> 
wrote:

> On 05/01/2022 02:56, David Sterba wrote:
>> On Sat, Dec 11, 2021 at 02:15:29AM +0800, Anand Jain wrote:
>>> Identifying and removing the stale device from the fs_uuids 
>>> list is done
>>> by the function btrfs_free_stale_devices().
>>> btrfs_free_stale_devices() in turn depends on the function
>>> device_path_matched() to check if the device repeats in more 
>>> than one
>>> btrfs_device structure.
>>>
>>> The matching of the device happens by its path, the device 
>>> path. However,
>>> when dm mapper is in use, the dm device paths are nothing but 
>>> a link to
>>> the actual block device, which leads to the 
>>> device_path_matched() failing
>>> to match.
>>>
>>> Fix this by matching the dev_t as provided by lookup_bdev() 
>>> instead of
>>> plain strcmp() the device paths.
>>>
>>> Reported-by: Josef Bacik <josef@toxicpanda.com>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>
>>> v2: Fix
>>>       sparse: warning: incorrect type in argument 1 (different 
>>>       address spaces)
>>>       For using device->name->str
>>>
>>>      Fix Josef suggestion to pass dev_t instead of device-path 
>>>      in the
>>>       patch 2/2.
>>>
>>>   fs/btrfs/volumes.c | 41 
>>>   ++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 1b02c03a882c..559fdb0c4a0e 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -534,15 +534,46 @@ btrfs_get_bdev_and_sb(const char 
>>> *device_path, fmode_t flags, void *holder,
>>>   	return ret;
>>>   }
>>>   -static bool device_path_matched(const char *path, struct 
>>>   btrfs_device
>>> *device)
>>> +/*
>>> + * Check if the device in the 'path' matches with the device 
>>> in the given
>>> + * struct btrfs_device '*device'.
>>> + * Returns:
>>> + *	0	If it is the same device.
>>> + *	1	If it is not the same device.
>>> + *	-errno	For error.
>>> + */
>>> +static int device_matched(struct btrfs_device *device, const 
>>> char *path)
>>>   {
>>> -	int found;
>>> +	char *device_name;
>>> +	dev_t dev_old;
>>> +	dev_t dev_new;
>>> +	int ret;
>>> +
>>> +	device_name = kzalloc(BTRFS_PATH_NAME_MAX, GFP_KERNEL);
>>> +	if (!device_name)
>>> +		return -ENOMEM;
>>>     	rcu_read_lock();
>>> -	found = strcmp(rcu_str_deref(device->name), path);
>>> +	ret = sprintf(device_name, "%s", 
>>> rcu_str_deref(device->name));
>> I wonder if the temporary allocation could be avoided, as it's 
>> the
>> exactly same path of the device, so it could be passed to 
>> lookup_bdev.
>
>  Yeah, I tried but to no avail. Unless I drop the rcu read lock 
>  and
>  use the str directly as below.
>
>  lookup_bdev(device->name->str, &dev_old);
>
>  We do skip rcu access for device->name at a few locations.
>
>  Also, pls note lookup_bdev() can't be called within 
>  rcu_read_lock(),
>  (calling sleep function warning).
>

Got it. And another evil and dirty solution is that open code the 
logic in
the only user btrfs_free_stale_devices(). Do the memory allocation 
and
lookup_bdev(path) before the big big loop so we won't be disturbed
error handling and save some times of lookup_bdev ;)

--
Su
>
>>>   	rcu_read_unlock();
>>> +	if (!ret) {
>>> +		kfree(device_name);
>>> +		return -EINVAL;
>>> +	}
>>>   -	return found == 0;
>>> +	ret = lookup_bdev(device_name, &dev_old);
>>> +	kfree(device_name);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = lookup_bdev(path, &dev_new);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (dev_old == dev_new)
>>> +		return 0;
>>> +
>>> +	return 1;
>>>   }
>>>     /*
>>> @@ -577,7 +608,7 @@ static int btrfs_free_stale_devices(const 
>>> char *path,
>>>   				continue;
>>>   			if (path && !device->name)
>>>   				continue;
>>> -			if (path && !device_path_matched(path, 
>>> device))
>>> +			if (path && device_matched(device, path) 
>>> != 0)
>> You've changed the fuction to return errors but it's not 
>> checked,
>> besides the memory allocation failure, the lookup functions 
>> could fail
>> for various reasons so I don't think it's safe to ignore the 
>> errors.
>
> IMO there isn't much that the parent function should do even if 
> the
> device_matched() returns an error for the reasons you stated.
>
> Mainly because btrfs_free_stale_devices() OR 
> btrfs_forget_devices()
> is used as a cleanup ops in the primary task functions such as
> btrfs_scan_one_device() and btrfs_init_new_device(). Even if we 
> don't
> remove the stale there is no harm.
>
> Further btrfs_forget_devices() is called from 
> btrfs_control_ioctl()
> which is a userland call for forget devices. So as we traverse 
> the
> device list, if a device lookup fails IMO, it is ok to skip to 
> the next
> device in the list and return the status of the device match.
>
> Even more, IMO we should save the dev_t in the struct 
> btrfs_device,
> upon which the device_matched() will go away altogether. This 
> change
> is outside of the bug that we intended to fix here. I will clean 
> that
> up separately.
>
> Thanks, Anand
>
>>>   				continue;
>>>   			if (fs_devices->opened) {
>>>   				/* for an already deleted device 
>>>   return 0 */
>>> -- 2.33.1