[PATCH v2 0/3] btrfs-progs: Fix logical-resolve

[PATCH v2 0/3] btrfs-progs: Fix logical-resolve

[Marcos Paulo de Souza]

From: Marcos Paulo de Souza <mpdesouza@suse.com>

New issues were found while testing the v1, and new problems with the current
implementation too. Now focusing on a generic way of detecting where to do the
file lookup based in the logical offset of the file.

Now there is a test to avoid this problem from appearing again in the future.

Please let me know if this can be improved even further.

Changes from v1:
* Patches 2 and 3 added
* Test created (David)
* Discard changed on btrfs_list_path_for_root and changing find_mount_root
  instead

First version:
https://lore.kernel.org/linux-btrfs/20201112011400.6866-1-marcos@mpdesouza.com/

Marcos Paulo de Souza (3):
  btrfs-progs: Adapt find_mount_root to verify other fields of mntent
    struct
  btrfs-progs: inspect: Fix logical-resolve file path lookup
  btrfs-progs: tests: Add new logical-resolve test

 cmds/inspect.c                                | 30 +++++++---
 cmds/receive.c                                |  3 +-
 cmds/send.c                                   |  6 +-
 common/utils.c                                | 19 +++++-
 common/utils.h                                | 11 +++-
 .../test.sh                                   | 60 +++++++++++++++++++
 6 files changed, 114 insertions(+), 15 deletions(-)
 create mode 100755 tests/misc-tests/042-inspect-internal-logical-resolve/test.sh

-- 
2.26.2

[PATCH v2 1/3] btrfs-progs: Adapt find_mount_root to verify other fields of mntent struct

[Marcos Paulo de Souza]

From: Marcos Paulo de Souza <mpdesouza@suse.com>

Currently find_mount_root searches for all btrfs filesystems
mounted and comparing <path> with mnt_dir of each mountpoint.

But there are cases when we need to find the mountpoint for a determined
subvolid or subvol path, and these informations are present in mnt_opts
of mntent struct.

This patch adds two arguments to find_mount_root (data and flag). The
data argument hold the information that we want to compare, and the flag
argument specifies which field of mntent struct that we want to compare.
Currently there is only one flag, BTRFS_FIND_ROOT_PATH, implementing the
current behavior. The next patch will add a new flag to expand the functionality.

Users of find_mount_root were changed, having the data argument the same
as path, since they are only trying to find the mountpoint based on path alone.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 cmds/receive.c |  3 ++-
 cmds/send.c    |  6 ++++--
 common/utils.c | 15 ++++++++++++---
 common/utils.h |  8 +++++++-
 4 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/cmds/receive.c b/cmds/receive.c
index 2aaba3ff..dc64480e 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -1079,7 +1079,8 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
 	if (realmnt[0]) {
 		rctx->root_path = realmnt;
 	} else {
-		ret = find_mount_root(dest_dir_full_path, &rctx->root_path);
+		ret = find_mount_root(dest_dir_full_path, dest_dir_full_path,
+				BTRFS_FIND_ROOT_PATH, &rctx->root_path);
 		if (ret < 0) {
 			errno = -ret;
 			error("failed to determine mount point for %s: %m",
diff --git a/cmds/send.c b/cmds/send.c
index b8e3ba12..7757f0da 100644
--- a/cmds/send.c
+++ b/cmds/send.c
@@ -329,7 +329,8 @@ static int init_root_path(struct btrfs_send *sctx, const char *subvol)
 	if (sctx->root_path)
 		goto out;
 
-	ret = find_mount_root(subvol, &sctx->root_path);
+	ret = find_mount_root(subvol, subvol, BTRFS_FIND_ROOT_PATH,
+				&sctx->root_path);
 	if (ret < 0) {
 		errno = -ret;
 		error("failed to determine mount point for %s: %m", subvol);
@@ -659,7 +660,8 @@ static int cmd_send(const struct cmd_struct *cmd, int argc, char **argv)
 			goto out;
 		}
 
-		ret = find_mount_root(subvol, &mount_root);
+		ret = find_mount_root(subvol, subvol, BTRFS_FIND_ROOT_PATH,
+					&mount_root);
 		if (ret < 0) {
 			errno = -ret;
 			error("find_mount_root failed on %s: %m", subvol);
diff --git a/common/utils.c b/common/utils.c
index 1253e87d..1c264455 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -1248,7 +1248,7 @@ int ask_user(const char *question)
  * return 1 if a mount point is found but not btrfs
  * return <0 if something goes wrong
  */
-int find_mount_root(const char *path, char **mount_root)
+int find_mount_root(const char *path, const char *data, u8 flag, char **mount_root)
 {
 	FILE *mnttab;
 	int fd;
@@ -1258,6 +1258,10 @@ int find_mount_root(const char *path, char **mount_root)
 	int not_btrfs = 1;
 	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)
@@ -1269,8 +1273,13 @@ int find_mount_root(const char *path, char **mount_root)
 		return -errno;
 
 	while ((ent = getmntent(mnttab))) {
-		len = strlen(ent->mnt_dir);
-		if (strncmp(ent->mnt_dir, path, len) == 0) {
+		cmp_field = ent->mnt_dir;
+
+		len = strlen(cmp_field);
+
+		found = strncmp(cmp_field, data, len) == 0;
+
+		if (found) {
 			/* match found and use the latest match */
 			if (longest_matchlen <= len) {
 				free(longest_match);
diff --git a/common/utils.h b/common/utils.h
index 119c3881..449e1d3e 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -52,6 +52,11 @@
 #define UNITS_HUMAN			(UNITS_HUMAN_BINARY)
 #define UNITS_DEFAULT			(UNITS_HUMAN)
 
+enum btrfs_find_root_flags {
+	/* check mnt_dir of mntent */
+	BTRFS_FIND_ROOT_PATH = 0
+};
+
 void units_set_mode(unsigned *units, unsigned mode);
 void units_set_base(unsigned *units, unsigned base);
 
@@ -93,7 +98,8 @@ int csum_tree_block(struct btrfs_fs_info *root, struct extent_buffer *buf,
 int ask_user(const char *question);
 int lookup_path_rootid(int fd, u64 *rootid);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
-int find_mount_root(const char *path, char **mount_root);
+int find_mount_root(const char *path, const char *data, u8 flag,
+		char **mount_root);
 int get_device_info(int fd, u64 devid,
 		struct btrfs_ioctl_dev_info_args *di_args);
 int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret);
-- 
2.26.2

Re: [PATCH v2 1/3] btrfs-progs: Adapt find_mount_root to verify other fields of mntent struct

[Qu Wenruo]

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



On 2020/11/17 上午1:32, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> Currently find_mount_root searches for all btrfs filesystems
> mounted and comparing <path> with mnt_dir of each mountpoint.
> 
> But there are cases when we need to find the mountpoint for a determined
> subvolid or subvol path, and these informations are present in mnt_opts
> of mntent struct.

Mind to share why we want that?

The main concern here is, I didn't see the point that how the mount
point of a subvolume would affect anything.

Thanks,
Qu

> 
> This patch adds two arguments to find_mount_root (data and flag). The
> data argument hold the information that we want to compare, and the flag
> argument specifies which field of mntent struct that we want to compare.
> Currently there is only one flag, BTRFS_FIND_ROOT_PATH, implementing the
> current behavior. The next patch will add a new flag to expand the functionality.
> 
> Users of find_mount_root were changed, having the data argument the same
> as path, since they are only trying to find the mountpoint based on path alone.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  cmds/receive.c |  3 ++-
>  cmds/send.c    |  6 ++++--
>  common/utils.c | 15 ++++++++++++---
>  common/utils.h |  8 +++++++-
>  4 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/cmds/receive.c b/cmds/receive.c
> index 2aaba3ff..dc64480e 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -1079,7 +1079,8 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
>  	if (realmnt[0]) {
>  		rctx->root_path = realmnt;
>  	} else {
> -		ret = find_mount_root(dest_dir_full_path, &rctx->root_path);
> +		ret = find_mount_root(dest_dir_full_path, dest_dir_full_path,
> +				BTRFS_FIND_ROOT_PATH, &rctx->root_path);
>  		if (ret < 0) {
>  			errno = -ret;
>  			error("failed to determine mount point for %s: %m",
> diff --git a/cmds/send.c b/cmds/send.c
> index b8e3ba12..7757f0da 100644
> --- a/cmds/send.c
> +++ b/cmds/send.c
> @@ -329,7 +329,8 @@ static int init_root_path(struct btrfs_send *sctx, const char *subvol)
>  	if (sctx->root_path)
>  		goto out;
>  
> -	ret = find_mount_root(subvol, &sctx->root_path);
> +	ret = find_mount_root(subvol, subvol, BTRFS_FIND_ROOT_PATH,
> +				&sctx->root_path);
>  	if (ret < 0) {
>  		errno = -ret;
>  		error("failed to determine mount point for %s: %m", subvol);
> @@ -659,7 +660,8 @@ static int cmd_send(const struct cmd_struct *cmd, int argc, char **argv)
>  			goto out;
>  		}
>  
> -		ret = find_mount_root(subvol, &mount_root);
> +		ret = find_mount_root(subvol, subvol, BTRFS_FIND_ROOT_PATH,
> +					&mount_root);
>  		if (ret < 0) {
>  			errno = -ret;
>  			error("find_mount_root failed on %s: %m", subvol);
> diff --git a/common/utils.c b/common/utils.c
> index 1253e87d..1c264455 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -1248,7 +1248,7 @@ int ask_user(const char *question)
>   * return 1 if a mount point is found but not btrfs
>   * return <0 if something goes wrong
>   */
> -int find_mount_root(const char *path, char **mount_root)
> +int find_mount_root(const char *path, const char *data, u8 flag, char **mount_root)
>  {
>  	FILE *mnttab;
>  	int fd;
> @@ -1258,6 +1258,10 @@ int find_mount_root(const char *path, char **mount_root)
>  	int not_btrfs = 1;
>  	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)
> @@ -1269,8 +1273,13 @@ int find_mount_root(const char *path, char **mount_root)
>  		return -errno;
>  
>  	while ((ent = getmntent(mnttab))) {
> -		len = strlen(ent->mnt_dir);
> -		if (strncmp(ent->mnt_dir, path, len) == 0) {
> +		cmp_field = ent->mnt_dir;
> +
> +		len = strlen(cmp_field);
> +
> +		found = strncmp(cmp_field, data, len) == 0;
> +
> +		if (found) {
>  			/* match found and use the latest match */
>  			if (longest_matchlen <= len) {
>  				free(longest_match);
> diff --git a/common/utils.h b/common/utils.h
> index 119c3881..449e1d3e 100644
> --- a/common/utils.h
> +++ b/common/utils.h
> @@ -52,6 +52,11 @@
>  #define UNITS_HUMAN			(UNITS_HUMAN_BINARY)
>  #define UNITS_DEFAULT			(UNITS_HUMAN)
>  
> +enum btrfs_find_root_flags {
> +	/* check mnt_dir of mntent */
> +	BTRFS_FIND_ROOT_PATH = 0
> +};
> +
>  void units_set_mode(unsigned *units, unsigned mode);
>  void units_set_base(unsigned *units, unsigned base);
>  
> @@ -93,7 +98,8 @@ int csum_tree_block(struct btrfs_fs_info *root, struct extent_buffer *buf,
>  int ask_user(const char *question);
>  int lookup_path_rootid(int fd, u64 *rootid);
>  int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
> -int find_mount_root(const char *path, char **mount_root);
> +int find_mount_root(const char *path, const char *data, u8 flag,
> +		char **mount_root);
>  int get_device_info(int fd, u64 devid,
>  		struct btrfs_ioctl_dev_info_args *di_args);
>  int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret);
> 


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

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

[Marcos Paulo de Souza]

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;
-				}
+
+				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);
-- 
2.26.2

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

[Qu Wenruo]

[-- 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 --]

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

[Qu Wenruo]

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



On 2020/11/20 下午4:32, 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/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.

Another thing is, if we can't access some subvolume, we just exist right
now, without checking next possible entry.

Furthermore, exiting without mentioning that the resolved path is not
the only path can give user some false illusion, e.g. deleting that file
is ensured to release that logical.

So at least we should do some prompt to inform the user there is some
other files referring to the logical bytenr, but we can't access right
now. (Maybe also output the subvolume path and inode)

Thanks,
Qu
> 
> 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 --]

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

[Marcos Paulo de Souza]

On Fri, 2020-11-20 at 16:43 +0800, Qu Wenruo wrote:
> 
> On 2020/11/20 下午4:32, 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/
> > > 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/i3
> > > 86-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.
> 
> Another thing is, if we can't access some subvolume, we just exist
> right
> now, without checking next possible entry.

That's a problem indeed.

> 
> Furthermore, exiting without mentioning that the resolved path is not
> the only path can give user some false illusion, e.g. deleting that
> file
> is ensured to release that logical.
> 
> So at least we should do some prompt to inform the user there is some
> other files referring to the logical bytenr, but we can't access
> right
> now. (Maybe also output the subvolume path and inode)

Yes, this too. I'm reworking the code to check for ENOENT on
find_mount_root and print a message showing inode and subvol path in
the case the subvol is not mounted.

Both issues will be solved in the next version.

Thanks for your review!

> 
> Thanks,
> Qu
> > 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);
> > > 

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

[Marcos Paulo de Souza]

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

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

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

[Qu Wenruo]

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

[PATCH v2 3/3] btrfs-progs: tests: Add new logical-resolve test

[Marcos Paulo de Souza]

From: Marcos Paulo de Souza <mpdesouza@suse.com>

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 .../test.sh                                   | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100755 tests/misc-tests/042-inspect-internal-logical-resolve/test.sh

diff --git a/tests/misc-tests/042-inspect-internal-logical-resolve/test.sh b/tests/misc-tests/042-inspect-internal-logical-resolve/test.sh
new file mode 100755
index 00000000..adb52289
--- /dev/null
+++ b/tests/misc-tests/042-inspect-internal-logical-resolve/test.sh
@@ -0,0 +1,60 @@
+#!/bin/bash
+# Check if logical-resolve is resolving the paths correctly for different
+# subvolume tree configurations. This used to fails when a child subvolume was
+# mounted without the parent subvolume being accessible.
+
+source "$TEST_TOP/common"
+
+setup_root_helper
+prepare_test_dev
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+
+check_logical_offset_filename()
+{
+	local filename
+	local offset
+	offset="$1"
+	filename="$2"
+
+	out=$($TOP/btrfs inspect-internal logical-resolve "$offset" "$TEST_MNT")
+	if [ ! $filename = $out ]; then
+		_fail "logical-resolve failed. Expected $filename but returned $out"
+	fi
+}
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+
+# create top subvolume called '@'
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/@"
+
+# create a file in eacch subvolume of @, and each file will have 2 EXTENT_DATA item
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/@/vol1"
+vol1id=$($SUDO_HELPER "$TOP/btrfs" inspect-internal rootid "$TEST_MNT/@/vol1")
+run_check $SUDO_HELPER dd if=/dev/zero bs=1M count=150 of="$TEST_MNT/@/vol1/file1"
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/@/vol1/subvol1"
+subvol1id=$($SUDO_HELPER "$TOP/btrfs" inspect-internal rootid "$TEST_MNT/@/vol1/subvol1")
+run_check $SUDO_HELPER dd if=/dev/zero bs=1M count=150 of="$TEST_MNT/@/vol1/subvol1/file2"
+
+"$TOP/btrfs" filesystem sync "$TEST_MNT"
+
+run_check_umount_test_dev
+
+$SUDO_HELPER mount -o subvol=/@/vol1 $TEST_DEV "$TEST_MNT"
+for offset in $("$TOP/btrfs" inspect-internal dump-tree -t "$vol1id" \
+		"$TEST_DEV" | awk '/disk byte/ { print $5 }'); do
+	check_logical_offset_filename "$offset" "$TEST_MNT/file1"
+done
+
+run_check_umount_test_dev
+
+$SUDO_HELPER mount -o subvol=/@/vol1/subvol1 $TEST_DEV "$TEST_MNT"
+for offset in $("$TOP/btrfs" inspect-internal dump-tree -t "$subvol1id" \
+		"$TEST_DEV" | awk '/disk byte/ { print $5 }'); do
+	check_logical_offset_filename "$offset" "$TEST_MNT/file2"
+done
+
+run_check_umount_test_dev
-- 
2.26.2