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

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

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



On 2020/11/28 上午3:30, 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 childs:
> 
> 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 the subvolume full
> path, starting from '@'. Later on, btrfs_open_dir is called 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.
> 
> [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 returning
> the correct path accessible to the user. Using this approach we can solve
> the problems stated above by not trusting the subvolume name being the
> mountpoint, and not executing the lookup based only in the subvolume
> tree.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>


With the extra prompt for subvolume can't be accessed, it looks good to
me now.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>  cmds/inspect.c | 44 ++++++++++++++++++++++++++++++++++----------
>  common/utils.c | 29 +++++++++++++++++++++++------
>  common/utils.h |  5 ++++-
>  3 files changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/cmds/inspect.c b/cmds/inspect.c
> index 2530b904..cfa2f708 100644
> --- a/cmds/inspect.c
> +++ b/cmds/inspect.c
> @@ -236,6 +236,7 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
>  		DIR *dirs = NULL;
>  
>  		if (getpath) {
> +			char mount_path[PATH_MAX];
>  			name = btrfs_list_path_for_root(fd, root);
>  			if (IS_ERR(name)) {
>  				ret = PTR_ERR(name);
> @@ -244,23 +245,46 @@ static int cmd_inspect_logical_resolve(const struct cmd_struct *cmd,
>  			if (!name) {
>  				path_ptr[-1] = '\0';
>  				path_fd = fd;
> +
> +				strncpy(mount_path, full_path, PATH_MAX);
>  			} 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);
> -					goto out;
> +				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 == -ENOENT) {
> +					printf("inode %llu subvol %s could not be accessed: not mounted\n",
> +							inum, name);
> +					continue;
>  				}
> -				path_fd = btrfs_open_dir(full_path, &dirs, 1);
> +
> +				if (ret < 0)
> +					goto out;
> +
> +				strncpy(mount_path, mounted, PATH_MAX);
> +				free(mounted);
> +
> +				path_fd = btrfs_open_dir(mount_path, &dirs, 1);
>  				if (path_fd < 0) {
>  					ret = -ENOENT;
>  					goto out;
>  				}
>  			}
> -			ret = __ino_to_path_fd(inum, path_fd, full_path);
> +			ret = __ino_to_path_fd(inum, path_fd, mount_path);
>  			if (path_fd != fd)
>  				close_file_or_dir(path_fd, dirs);
>  		} else {
> diff --git a/common/utils.c b/common/utils.c
> index 1c264455..1562ac52 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -1259,9 +1259,6 @@ int find_mount_root(const char *path, const char *data, u8 flag, char **mount_ro
>  	int longest_matchlen = 0;
>  	char *longest_match = NULL;
>  	char *cmp_field = NULL;
> -	bool found;
> -
> -	BUG_ON(flag != BTRFS_FIND_ROOT_PATH);
>  
>  	fd = open(path, O_RDONLY | O_NOATIME);
>  	if (fd < 0)
> @@ -1273,12 +1270,32 @@ 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;
> +		bool found = false;
>  
> -		len = strlen(cmp_field);
> +		/* 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;
>  
> -		found = strncmp(cmp_field, data, len) == 0;
> +		len = strlen(cmp_field);
>  
> +		if (flag == BTRFS_FIND_ROOT_OPTS) {
> +			size_t dlen = strlen(data);
> +			char *tmp_str = strstr(cmp_field, data);
> +			/*
> +			 * Make sure that we are dealing with the wanted string,
> +			 * since strstr returns the start of the string found.
> +			 * Compare the end string position from data with the
> +			 * mount point found, and make sure that we have an
> +			 * option separator or string end.
> +			 */
> +			if (tmp_str)
> +				found = tmp_str[dlen] == ',' ||
> +					tmp_str[dlen] == 0;
> +		} else {
> +			found = strncmp(cmp_field, data, len) == 0;
> +		}
>  		if (found) {
>  			/* match found and use the latest match */
>  			if (longest_matchlen <= len) {
> 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 --]