Re: [PATCH v2 2/3] btrfs-progs: inspect: Fix logical-resolve file path lookup

[Qu Wenruo <wqu@suse.com>]

On 2020/11/21 下午8:35, Marcos Paulo de Souza wrote:
> On Fri, 2020-11-20 at 16:32 +0800, Qu Wenruo wrote:
>>
>> On 2020/11/17 上午1:32, Marcos Paulo de Souza wrote:
>>> From: Marcos Paulo de Souza <mpdesouza@suse.com>
>>>
>>> [BUG]
>>> logical-resolve is currently broken on systems that have a child
>>> subvolume being mounted without access to the parent subvolume.
>>> This is the default for SLE/openSUSE installations. openSUSE has
>>> the
>>> subvolume '@' as the parent of all other subvolumes like /boot,
>>> /home.
>>> The subvolume '@' is never mounted, and accessed, but only it's
>>> child:
>>>
>>> mount | grep btrfs
>>> /dev/sda2 on / type btrfs
>>> (rw,relatime,ssd,space_cache,subvolid=267,subvol=/@/.snapshots/1/sn
>>> apshot)
>>> /dev/sda2 on /opt type btrfs
>>> (rw,relatime,ssd,space_cache,subvolid=262,subvol=/@/opt)
>>> /dev/sda2 on /boot/grub2/i386-pc type btrfs
>>> (rw,relatime,ssd,space_cache,subvolid=265,subvol=/@/boot/grub2/i386
>>> -pc)
>>>
>>> logical-resolve command calls btrfs_list_path_for_root, that
>>> returns the
>>> subvolume full-path that corresponds to the tree id of the logical
>>> address. As the name implies, the 'full-path' returns all
>>> subvolumes,
>>> starting from '@'. Later on, btrfs_open_dir is calles using the
>>> path
>>> returned, but it fails to resolve it since it contains the '@' and
>>> this
>>> subvolume cannot be accessed.
>>>
>>> The same problem can be triggered to any user that calls for
>>> logical-resolve on a child subvolume that has the parent subvolume
>>> not accessible.
>>>
>>> Another problem in the current approach is that it believes that a
>>> subvolume will be mounted in a directory with the same name e.g
>>> /@/boot
>>> being mounted in /boot. When this is not true, the code also fails,
>>> since it uses the subvolume name as the path accessible by the
>>> user.
>>>
>>> [FIX]
>>> Extent the find_mount_root function by allowing it to check for
>>> mnt_opts
>>> member of mntent struct. Using this new approach we can change
>>> logical-resolve command to search for subvolid=XXX,subvol=YYY. This
>>> is
>>> the two problems stated above by not trusting the subvolume name
>>> being
>>> the mountpoint name, and not following the subvolume tree blindly.
>>>
>>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>>> ---
>>>  cmds/inspect.c | 30 ++++++++++++++++++++++--------
>>>  common/utils.c | 13 +++++++++----
>>>  common/utils.h |  5 ++++-
>>>  3 files changed, 35 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/cmds/inspect.c b/cmds/inspect.c
>>> index 2530b904..0dc62d18 100644
>>> --- a/cmds/inspect.c
>>> +++ b/cmds/inspect.c
>>> @@ -245,15 +245,29 @@ static int cmd_inspect_logical_resolve(const
>>> struct cmd_struct *cmd,
>>>  				path_ptr[-1] = '\0';
>>>  				path_fd = fd;
>>>  			} else {
>>> -				path_ptr[-1] = '/';
>>> -				ret = snprintf(path_ptr, bytes_left,
>>> "%s",
>>> -						name);
>>> -				free(name);
>>> -				if (ret >= bytes_left) {
>>> -					error("path buffer too small:
>>> %d bytes",
>>> -							bytes_left -
>>> ret);
>>> +				char *mounted = NULL;
>>> +				char volid_str[PATH_MAX];
>>> +
>>> +				/*
>>> +				 * btrfs_list_path_for_root returns the
>>> full
>>> +				 * path to the subvolume pointed by
>>> root, but the
>>> +				 * subvolume can be mounted in a
>>> directory name
>>> +				 * different from the subvolume name.
>>> In this
>>> +				 * case we need to find the correct
>>> mountpoint
>>> +				 * using same subvol path and subvol id
>>> found
>>> +				 * before.
>>> +				 */
>>> +				snprintf(volid_str, PATH_MAX,
>>> "subvolid=%llu,subvol=/%s",
>>> +						root, name);
>>> +
>>> +				ret = find_mount_root(full_path,
>>> volid_str,
>>> +						BTRFS_FIND_ROOT_OPTS,
>>> &mounted);
>>> +				if (ret < 0)
>>>  					goto out;
>>> -				}
>>
>> OK, I see how you utilize the new parameter now.
>>
>> But considering there is only one user for BTRFS_FIND_ROOT_OPTS, i
>> really hope to not touching the existing callers.
>> Anyway this is just a nitpick, and mostly personal taste.
> 
> I tried to avoid creating a new function for only to only check a
> different field of mntent, so I slightly changed the callers. But let
> me know if you have a better idea for this case.
> 
>>
>> Another thing is, what if we bind mount a dir of btrfs to another
>> location.
>> Wouldn't this trick the find_mount_root() again?
> 
> I don't see why, since they point to the same fs. The only difference
> is that find_mount_root can then print the mnt_dir of the bind mount in
> the case that the path that was matched was bigger than the other mount
> points (longest_matchlen in find_mount_root).

So you mean it can handle such case?

# mount $dev $mnt
# mkdir $mnt/dir
# mount -o bind $mnt/dir $mnt2
# umount $mnt

Then in $mnt2 we can only access what inside dir, even something inside
the same subvolume can't be accessed.

In that case, can the function still handle it?

Thanks,
Qu
> 
>>
>> Personally speaking, we should only permit subvolume entry to be
>> passes
>> to the btrfs-logical-resolve command to avoid such problems.
> 
> Doesn't it limit the usage of logical-resolve?
> 
> If the user receive this a message about checksum problems like
> 
> BTRFS error (device dasda3): unable to fixup (regular) error at logical
> 519704576 on dev 
> 
> How would the user know which in which subvolume the address is being
> referred to? Maybe we could add this info on the kernel side to print
> the subvol id too? IMHO it would be better if we could find the correct
> subvolume in logical-address as this gives more flexibility to the
> user. But I'm all ears :)
> 
>>
>> Thanks,
>> Qu
>>> +
>>> +				strncpy(full_path, mounted, PATH_MAX);
>>> +				free(mounted);
>>> +
>>>  				path_fd = btrfs_open_dir(full_path,
>>> &dirs, 1);
>>>  				if (path_fd < 0) {
>>>  					ret = -ENOENT;
>>> diff --git a/common/utils.c b/common/utils.c
>>> index 1c264455..7e6f406b 100644
>>> --- a/common/utils.c
>>> +++ b/common/utils.c
>>> @@ -1261,8 +1261,6 @@ int find_mount_root(const char *path, const
>>> char *data, u8 flag, char **mount_ro
>>>  	char *cmp_field = NULL;
>>>  	bool found;
>>>  
>>> -	BUG_ON(flag != BTRFS_FIND_ROOT_PATH);
>>> -
>>>  	fd = open(path, O_RDONLY | O_NOATIME);
>>>  	if (fd < 0)
>>>  		return -errno;
>>> @@ -1273,11 +1271,18 @@ int find_mount_root(const char *path, const
>>> char *data, u8 flag, char **mount_ro
>>>  		return -errno;
>>>  
>>>  	while ((ent = getmntent(mnttab))) {
>>> -		cmp_field = ent->mnt_dir;
>>> +		/* BTRFS_FIND_ROOT_PATH is the default behavior */
>>> +		if (flag == BTRFS_FIND_ROOT_OPTS)
>>> +			cmp_field = ent->mnt_opts;
>>> +		else
>>> +			cmp_field = ent->mnt_dir;
>>>  
>>>  		len = strlen(cmp_field);
>>>  
>>> -		found = strncmp(cmp_field, data, len) == 0;
>>> +		if (flag == BTRFS_FIND_ROOT_OPTS)
>>> +			found = strstr(cmp_field, data) != NULL;
>>> +		else
>>> +			found = strncmp(cmp_field, data, len) == 0;
>>>  
>>>  		if (found) {
>>>  			/* match found and use the latest match */
>>> diff --git a/common/utils.h b/common/utils.h
>>> index 449e1d3e..b5d256c6 100644
>>> --- a/common/utils.h
>>> +++ b/common/utils.h
>>> @@ -54,7 +54,10 @@
>>>  
>>>  enum btrfs_find_root_flags {
>>>  	/* check mnt_dir of mntent */
>>> -	BTRFS_FIND_ROOT_PATH = 0
>>> +	BTRFS_FIND_ROOT_PATH = 0,
>>> +
>>> +	/* check mnt_opts of mntent */
>>> +	BTRFS_FIND_ROOT_OPTS
>>>  };
>>>  
>>>  void units_set_mode(unsigned *units, unsigned mode);
>>>
>