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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5741 bytes --]



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/snapshot)
> /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.

Another thing is, what if we bind mount a dir of btrfs to another location.
Wouldn't this trick the find_mount_root() again?

Personally speaking, we should only permit subvolume entry to be passes
to the btrfs-logical-resolve command to avoid such problems.

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);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]