[PATCH v2 0/2] btrfs: enhance btrfs device path rename

[PATCH v2 0/2] btrfs: enhance btrfs device path rename

[Qu Wenruo]

[CHANGELOG]
v2:
- Update the example of the first patch
  Instead of using the special C program, just touch the soft link
  to trigger the rescan.

- Add Link and reported-by tags for the first patch

- Fix a "weird" -> "wild" typo
  This is really weird...

- Remove the debugging pr_info()

- Use safer strscpy() to replace strncpy()

- Use the shortcut version of conditional operator when possible

Fabian Vogt reported a very interesting case where we can use
"/proc/self/fd/3" to mount a btrfs, then that /proc name will remain as
the device path for the btrfs.

Sometimes udev scan can trigger a rename, and get a proper name back,
but if that doesn't win the race, we got the "/proc/self/fd/3" in the
mtab which makes no sense, since "/proc/self" is bounded to the current
process, no one is going to know where the real device is.

So to enhance the handling:

- Do proper path comparison to determine if we want to update device
  path
  Currently the path comparison is done by strcmp(), which will never
  handle different hard nor soft links.

  Instead go with path_equal() on the resolved paths, which is way more
  reliable.
  This should reduce the unnecessary path name updates in the first
  place.

- Canonicalize the device path if it's not inside "/dev/"
  If we pass a soft link which is not inside "/dev/", we just follow the
  soft link and use the final destination instead.

  It can be something like "/dev/dm-2", other than more readable LVM
  names like "/dev/test/scratch1", but it's still way better than
  "/proc/self/fd/3".

  If the soft link itself is already inside "/dev/", then we can
  directly use the path.

  This is to allow fstest run properly without forced to use
  "/dev/dm-*" instead of the "/dev/mapper/test-scratch1" as test
  devices.
  Otherwise fs tests will reject runs because it believe the btrfs
  is mounted somewhere else.

Qu Wenruo (2):
  btrfs: avoid unnecessary device path update for the same device
  btrfs: canonicalize the device path before adding it

 fs/btrfs/volumes.c | 107 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 2 deletions(-)

-- 
2.46.1

[PATCH v2 1/2] btrfs: avoid unnecessary device path update for the same device

[Qu Wenruo]

[PROBLEM]
It is very common for udev to trigger device scan, and every time a
mounted btrfs device got re-scan from different soft links, we will get
some of unnecessary device path updates, this is especially common
for LVM based storage:

 # lvs
  scratch1 test -wi-ao---- 10.00g
  scratch2 test -wi-a----- 10.00g
  scratch3 test -wi-a----- 10.00g
  scratch4 test -wi-a----- 10.00g
  scratch5 test -wi-a----- 10.00g
  test     test -wi-a----- 10.00g

 # mkfs.btrfs -f /dev/test/scratch1
 # mount /dev/test/scratch1 /mnt/btrfs
 # dmesg -c
 [  205.705234] BTRFS: device fsid 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9 devid 1 transid 6 /dev/mapper/test-scratch1 (253:4) scanned by mount (1154)
 [  205.710864] BTRFS info (device dm-4): first mount of filesystem 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9
 [  205.711923] BTRFS info (device dm-4): using crc32c (crc32c-intel) checksum algorithm
 [  205.713856] BTRFS info (device dm-4): using free-space-tree
 [  205.722324] BTRFS info (device dm-4): checking UUID tree

So far so good, but even if we just touched any soft link of
"dm-4", we will get quite some unnecessary device path updates.

 # touch /dev/mapper/test-scratch1
 # dmesg -c
 [  469.295796] BTRFS info: devid 1 device path /dev/mapper/test-scratch1 changed to /dev/dm-4 scanned by (udev-worker) (1221)
 [  469.300494] BTRFS info: devid 1 device path /dev/dm-4 changed to /dev/mapper/test-scratch1 scanned by (udev-worker) (1221)

Such device path rename is unnecessary and can lead to random path
change due to the udev race.

[CAUSE]
Inside device_list_add(), we are using a very primitive way checking if
the device has changed, strcmp().

Which can never handle links well, no matter if it's hard or soft links.

So every different link of the same device will be treated as a different
device, causing the unnecessary device path update.

[FIX]
Introduce a helper, is_same_device(), and use path_equal() to properly
detect the same block device.
So that the different soft links won't trigger the rename race.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
Reported-by: Fabian Vogt <fvogt@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 995b0647f538..b713e4ebb362 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -732,6 +732,32 @@ const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb)
 	return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
 }
 
+static bool is_same_device(struct btrfs_device *device, const char *new_path)
+{
+	struct path old = { .mnt = NULL, .dentry = NULL };
+	struct path new = { .mnt = NULL, .dentry = NULL };
+	char *old_path;
+	bool is_same = false;
+	int ret;
+
+	if (!device->name)
+		goto out;
+
+	old_path = rcu_str_deref(device->name);
+	ret = kern_path(old_path, LOOKUP_FOLLOW, &old);
+	if (ret)
+		goto out;
+	ret = kern_path(new_path, LOOKUP_FOLLOW, &new);
+	if (ret)
+		goto out;
+	if (path_equal(&old, &new))
+		is_same = true;
+out:
+	path_put(&old);
+	path_put(&new);
+	return is_same;
+}
+
 /*
  * Add new device to list of registered devices
  *
@@ -852,7 +878,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 				MAJOR(path_devt), MINOR(path_devt),
 				current->comm, task_pid_nr(current));
 
-	} else if (!device->name || strcmp(device->name->str, path)) {
+	} else if (!device->name || !is_same_device(device, path)) {
 		/*
 		 * When FS is already mounted.
 		 * 1. If you are here and if the device->name is NULL that
-- 
2.46.1

[PATCH v2 2/2] btrfs: canonicalize the device path before adding it

[Qu Wenruo]

[PROBLEM]
Currently btrfs accepts any file path for its device, resulting some
weird situation:

 # ./mount_by_fd /dev/test/scratch1  /mnt/btrfs/

The program has the following source code:

 #include <fcntl.h>
 #include <stdio.h>
 #include <sys/mount.h>

 int main(int argc, char *argv[]) {
	int fd = open(argv[1], O_RDWR);
	char path[256];
	snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
	return mount(path, argv[2], "btrfs", 0, NULL);
 }

Then we can have the following weird device path:

 BTRFS: device fsid 2378be81-fe12-46d2-a9e8-68cf08dd98d5 devid 1 transid 7 /proc/self/fd/3 (253:2) scanned by mount_by_fd (18440)

Normally it's not a big deal, and later udev can trigger a device path
rename. But if udev didn't trigger, the device path "/proc/self/fd/3"
will show up in mtab.

[CAUSE]
For filename "/proc/self/fd/3", it means the opened file descriptor 3.
In above case, it's exactly the device we want to open, aka points to
"/dev/test/scratch1" which is another softlink pointing to "/dev/dm-2".

Inside kernel we solve the mount source using LOOKUP_FOLLOW, which
follows the symbolic link and grab the proper block device.

But inside btrfs we also save the filename into btrfs_device::name, and
utilize that member to report our mount source, which leads to the above
situation.

[FIX]
Instead of unconditionally trust the path, check if the original file
(not following the symbolic link) is inside "/dev/", if not, then
manually lookup the path to its final destination, and use that as our
device path.

This allows us to still use symbolic links, like
"/dev/mapper/test-scratch" from LVM2, which is required for fstests runs
with LVM2 setup.

And for really weird names, like the above case, we solve it to
"/dev/dm-2" instead.

Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
Reported-by: Fabian Vogt <fvogt@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b713e4ebb362..668138451f7c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -732,6 +732,70 @@ const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb)
 	return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
 }
 
+/*
+ * We can have very weird soft links passed in.
+ * One example is "/proc/self/fd/<fd>", which can be a soft link to
+ * a block device.
+ *
+ * But it's never a good idea to use those weird names.
+ * Here we check if the path (not following symlinks) is a good one inside
+ * "/dev/".
+ */
+static bool is_good_dev_path(const char *dev_path)
+{
+	struct path path = { .mnt = NULL, .dentry = NULL };
+	char *path_buf = NULL;
+	char *resolved_path;
+	bool is_good = false;
+	int ret;
+
+	path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (!path_buf)
+		goto out;
+
+	/*
+	 * Do not follow soft link, just check if the original path is inside
+	 * "/dev/".
+	 */
+	ret = kern_path(dev_path, 0, &path);
+	if (ret)
+		goto out;
+	resolved_path = d_path(&path, path_buf, PATH_MAX);
+	if (IS_ERR(resolved_path))
+		goto out;
+	if (strncmp(resolved_path, "/dev/", strlen("/dev/")))
+		goto out;
+	is_good = true;
+out:
+	kfree(path_buf);
+	path_put(&path);
+	return is_good;
+}
+
+static int get_canonical_dev_path(const char *dev_path, char *canonical)
+{
+	struct path path = { .mnt = NULL, .dentry = NULL };
+	char *path_buf = NULL;
+	char *resolved_path;
+	int ret;
+
+	path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (!path_buf) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = kern_path(dev_path, LOOKUP_FOLLOW, &path);
+	if (ret)
+		goto out;
+	resolved_path = d_path(&path, path_buf, PATH_MAX);
+	ret = strscpy(canonical, resolved_path, PATH_MAX);
+out:
+	kfree(path_buf);
+	path_put(&path);
+	return ret;
+}
+
 static bool is_same_device(struct btrfs_device *device, const char *new_path)
 {
 	struct path old = { .mnt = NULL, .dentry = NULL };
@@ -1408,12 +1472,23 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 	bool new_device_added = false;
 	struct btrfs_device *device = NULL;
 	struct file *bdev_file;
+	char *canonical_path = NULL;
 	u64 bytenr;
 	dev_t devt;
 	int ret;
 
 	lockdep_assert_held(&uuid_mutex);
 
+	if (!is_good_dev_path(path)) {
+		canonical_path = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (canonical_path) {
+			ret = get_canonical_dev_path(path, canonical_path);
+			if (ret < 0) {
+				kfree(canonical_path);
+				canonical_path = NULL;
+			}
+		}
+	}
 	/*
 	 * Avoid an exclusive open here, as the systemd-udev may initiate the
 	 * device scan which may race with the user's mount or mkfs command,
@@ -1458,7 +1533,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 		goto free_disk_super;
 	}
 
-	device = device_list_add(path, disk_super, &new_device_added);
+	device = device_list_add(canonical_path ? : path, disk_super,
+				 &new_device_added);
 	if (!IS_ERR(device) && new_device_added)
 		btrfs_free_stale_devices(device->devt, device);
 
@@ -1467,6 +1543,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 
 error_bdev_put:
 	fput(bdev_file);
+	kfree(canonical_path);
 
 	return device;
 }
-- 
2.46.1

Re: [PATCH v2 2/2] btrfs: canonicalize the device path before adding it

[Filipe Manana]

On Wed, Sep 25, 2024 at 1:06 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [PROBLEM]
> Currently btrfs accepts any file path for its device, resulting some
> weird situation:
>
>  # ./mount_by_fd /dev/test/scratch1  /mnt/btrfs/
>
> The program has the following source code:
>
>  #include <fcntl.h>
>  #include <stdio.h>
>  #include <sys/mount.h>
>
>  int main(int argc, char *argv[]) {
>         int fd = open(argv[1], O_RDWR);
>         char path[256];
>         snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
>         return mount(path, argv[2], "btrfs", 0, NULL);
>  }
>
> Then we can have the following weird device path:
>
>  BTRFS: device fsid 2378be81-fe12-46d2-a9e8-68cf08dd98d5 devid 1 transid 7 /proc/self/fd/3 (253:2) scanned by mount_by_fd (18440)
>
> Normally it's not a big deal, and later udev can trigger a device path
> rename. But if udev didn't trigger, the device path "/proc/self/fd/3"
> will show up in mtab.
>
> [CAUSE]
> For filename "/proc/self/fd/3", it means the opened file descriptor 3.
> In above case, it's exactly the device we want to open, aka points to
> "/dev/test/scratch1" which is another softlink pointing to "/dev/dm-2".
>
> Inside kernel we solve the mount source using LOOKUP_FOLLOW, which
> follows the symbolic link and grab the proper block device.
>
> But inside btrfs we also save the filename into btrfs_device::name, and
> utilize that member to report our mount source, which leads to the above
> situation.
>
> [FIX]
> Instead of unconditionally trust the path, check if the original file
> (not following the symbolic link) is inside "/dev/", if not, then
> manually lookup the path to its final destination, and use that as our
> device path.
>
> This allows us to still use symbolic links, like
> "/dev/mapper/test-scratch" from LVM2, which is required for fstests runs
> with LVM2 setup.
>
> And for really weird names, like the above case, we solve it to
> "/dev/dm-2" instead.
>
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
> Reported-by: Fabian Vogt <fvogt@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/volumes.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index b713e4ebb362..668138451f7c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -732,6 +732,70 @@ const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb)
>         return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
>  }
>
> +/*
> + * We can have very weird soft links passed in.
> + * One example is "/proc/self/fd/<fd>", which can be a soft link to
> + * a block device.
> + *
> + * But it's never a good idea to use those weird names.
> + * Here we check if the path (not following symlinks) is a good one inside
> + * "/dev/".
> + */
> +static bool is_good_dev_path(const char *dev_path)
> +{
> +       struct path path = { .mnt = NULL, .dentry = NULL };
> +       char *path_buf = NULL;
> +       char *resolved_path;
> +       bool is_good = false;
> +       int ret;
> +
> +       path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
> +       if (!path_buf)
> +               goto out;
> +
> +       /*
> +        * Do not follow soft link, just check if the original path is inside
> +        * "/dev/".
> +        */
> +       ret = kern_path(dev_path, 0, &path);
> +       if (ret)
> +               goto out;
> +       resolved_path = d_path(&path, path_buf, PATH_MAX);
> +       if (IS_ERR(resolved_path))
> +               goto out;
> +       if (strncmp(resolved_path, "/dev/", strlen("/dev/")))
> +               goto out;
> +       is_good = true;
> +out:
> +       kfree(path_buf);
> +       path_put(&path);
> +       return is_good;
> +}
> +
> +static int get_canonical_dev_path(const char *dev_path, char *canonical)
> +{
> +       struct path path = { .mnt = NULL, .dentry = NULL };
> +       char *path_buf = NULL;
> +       char *resolved_path;
> +       int ret;
> +
> +       path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
> +       if (!path_buf) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       ret = kern_path(dev_path, LOOKUP_FOLLOW, &path);
> +       if (ret)
> +               goto out;
> +       resolved_path = d_path(&path, path_buf, PATH_MAX);
> +       ret = strscpy(canonical, resolved_path, PATH_MAX);
> +out:
> +       kfree(path_buf);
> +       path_put(&path);
> +       return ret;
> +}
> +
>  static bool is_same_device(struct btrfs_device *device, const char *new_path)
>  {
>         struct path old = { .mnt = NULL, .dentry = NULL };
> @@ -1408,12 +1472,23 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>         bool new_device_added = false;
>         struct btrfs_device *device = NULL;
>         struct file *bdev_file;
> +       char *canonical_path = NULL;
>         u64 bytenr;
>         dev_t devt;
>         int ret;
>
>         lockdep_assert_held(&uuid_mutex);
>
> +       if (!is_good_dev_path(path)) {
> +               canonical_path = kmalloc(PATH_MAX, GFP_KERNEL);
> +               if (canonical_path) {
> +                       ret = get_canonical_dev_path(path, canonical_path);
> +                       if (ret < 0) {
> +                               kfree(canonical_path);
> +                               canonical_path = NULL;
> +                       }
> +               }
> +       }
>         /*
>          * Avoid an exclusive open here, as the systemd-udev may initiate the
>          * device scan which may race with the user's mount or mkfs command,
> @@ -1458,7 +1533,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>                 goto free_disk_super;
>         }
>
> -       device = device_list_add(path, disk_super, &new_device_added);
> +       device = device_list_add(canonical_path ? : path, disk_super,
> +                                &new_device_added);
>         if (!IS_ERR(device) && new_device_added)
>                 btrfs_free_stale_devices(device->devt, device);
>
> @@ -1467,6 +1543,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>
>  error_bdev_put:
>         fput(bdev_file);
> +       kfree(canonical_path);
>
>         return device;
>  }
> --
> 2.46.1
>
>

Re: [PATCH v2 2/2] btrfs: canonicalize the device path before adding it

[Anand Jain]

On 25/9/24 5:36 am, Qu Wenruo wrote:
> [PROBLEM]
> Currently btrfs accepts any file path for its device, resulting some
> weird situation:
> 


>   # ./mount_by_fd /dev/test/scratch1  /mnt/btrfs/
> 
> The program has the following source code:
> 
>   #include <fcntl.h>
>   #include <stdio.h>
>   #include <sys/mount.h>
> 
>   int main(int argc, char *argv[]) {
> 	int fd = open(argv[1], O_RDWR);
> 	char path[256];
> 	snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
> 	return mount(path, argv[2], "btrfs", 0, NULL);
>   }
> 

Sorry for the late comments. Patches look good.

Quick question: both XFS and ext4 fail this test case—any idea why?
Can a generic/btrfs specific testcase be added to fstests?

Thanks, Anand

> Then we can have the following weird device path:
> 
>   BTRFS: device fsid 2378be81-fe12-46d2-a9e8-68cf08dd98d5 devid 1 transid 7 /proc/self/fd/3 (253:2) scanned by mount_by_fd (18440)
> 
> Normally it's not a big deal, and later udev can trigger a device path
> rename. But if udev didn't trigger, the device path "/proc/self/fd/3"
> will show up in mtab.
> 
> [CAUSE]
> For filename "/proc/self/fd/3", it means the opened file descriptor 3.
> In above case, it's exactly the device we want to open, aka points to
> "/dev/test/scratch1" which is another softlink pointing to "/dev/dm-2".
> 
> Inside kernel we solve the mount source using LOOKUP_FOLLOW, which
> follows the symbolic link and grab the proper block device.
> 
> But inside btrfs we also save the filename into btrfs_device::name, and
> utilize that member to report our mount source, which leads to the above
> situation.
> 
> [FIX]
> Instead of unconditionally trust the path, check if the original file
> (not following the symbolic link) is inside "/dev/", if not, then
> manually lookup the path to its final destination, and use that as our
> device path.
> 
> This allows us to still use symbolic links, like
> "/dev/mapper/test-scratch" from LVM2, which is required for fstests runs
> with LVM2 setup.
> 
> And for really weird names, like the above case, we solve it to
> "/dev/dm-2" instead.
> 
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
> Reported-by: Fabian Vogt <fvogt@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/volumes.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index b713e4ebb362..668138451f7c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -732,6 +732,70 @@ const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb)
>   	return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
>   }
>   
> +/*
> + * We can have very weird soft links passed in.
> + * One example is "/proc/self/fd/<fd>", which can be a soft link to
> + * a block device.
> + *
> + * But it's never a good idea to use those weird names.
> + * Here we check if the path (not following symlinks) is a good one inside
> + * "/dev/".
> + */
> +static bool is_good_dev_path(const char *dev_path)
> +{
> +	struct path path = { .mnt = NULL, .dentry = NULL };
> +	char *path_buf = NULL;
> +	char *resolved_path;
> +	bool is_good = false;
> +	int ret;
> +
> +	path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!path_buf)
> +		goto out;
> +
> +	/*
> +	 * Do not follow soft link, just check if the original path is inside
> +	 * "/dev/".
> +	 */
> +	ret = kern_path(dev_path, 0, &path);
> +	if (ret)
> +		goto out;
> +	resolved_path = d_path(&path, path_buf, PATH_MAX);
> +	if (IS_ERR(resolved_path))
> +		goto out;
> +	if (strncmp(resolved_path, "/dev/", strlen("/dev/")))
> +		goto out;
> +	is_good = true;
> +out:
> +	kfree(path_buf);
> +	path_put(&path);
> +	return is_good;
> +}
> +
> +static int get_canonical_dev_path(const char *dev_path, char *canonical)
> +{
> +	struct path path = { .mnt = NULL, .dentry = NULL };
> +	char *path_buf = NULL;
> +	char *resolved_path;
> +	int ret;
> +
> +	path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!path_buf) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = kern_path(dev_path, LOOKUP_FOLLOW, &path);
> +	if (ret)
> +		goto out;
> +	resolved_path = d_path(&path, path_buf, PATH_MAX);
> +	ret = strscpy(canonical, resolved_path, PATH_MAX);
> +out:
> +	kfree(path_buf);
> +	path_put(&path);
> +	return ret;
> +}
> +
>   static bool is_same_device(struct btrfs_device *device, const char *new_path)
>   {
>   	struct path old = { .mnt = NULL, .dentry = NULL };
> @@ -1408,12 +1472,23 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>   	bool new_device_added = false;
>   	struct btrfs_device *device = NULL;
>   	struct file *bdev_file;
> +	char *canonical_path = NULL;
>   	u64 bytenr;
>   	dev_t devt;
>   	int ret;
>   
>   	lockdep_assert_held(&uuid_mutex);
>   
> +	if (!is_good_dev_path(path)) {
> +		canonical_path = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (canonical_path) {
> +			ret = get_canonical_dev_path(path, canonical_path);
> +			if (ret < 0) {
> +				kfree(canonical_path);
> +				canonical_path = NULL;
> +			}
> +		}
> +	}
>   	/*
>   	 * Avoid an exclusive open here, as the systemd-udev may initiate the
>   	 * device scan which may race with the user's mount or mkfs command,
> @@ -1458,7 +1533,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>   		goto free_disk_super;
>   	}
>   
> -	device = device_list_add(path, disk_super, &new_device_added);
> +	device = device_list_add(canonical_path ? : path, disk_super,
> +				 &new_device_added);
>   	if (!IS_ERR(device) && new_device_added)
>   		btrfs_free_stale_devices(device->devt, device);
>   
> @@ -1467,6 +1543,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>   
>   error_bdev_put:
>   	fput(bdev_file);
> +	kfree(canonical_path);
>   
>   	return device;
>   }

Re: [PATCH v2 2/2] btrfs: canonicalize the device path before adding it

[Qu Wenruo]

在 2024/9/30 14:56, Anand Jain 写道:
> On 25/9/24 5:36 am, Qu Wenruo wrote:
>> [PROBLEM]
>> Currently btrfs accepts any file path for its device, resulting some
>> weird situation:
>>
> 
> 
>>   # ./mount_by_fd /dev/test/scratch1  /mnt/btrfs/
>>
>> The program has the following source code:
>>
>>   #include <fcntl.h>
>>   #include <stdio.h>
>>   #include <sys/mount.h>
>>
>>   int main(int argc, char *argv[]) {
>>     int fd = open(argv[1], O_RDWR);
>>     char path[256];
>>     snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
>>     return mount(path, argv[2], "btrfs", 0, NULL);
>>   }
>>
> 
> Sorry for the late comments. Patches look good.
> 
> Quick question: both XFS and ext4 fail this test case—any idea why?

Because they are single device filesystems (except the log device), thus 
they do not need to nor are able to update their device path halfway.

> Can a generic/btrfs specific testcase be added to fstests?

I can add it as a btrfs specific one, but I guess not a generic one?

Thanks,
Qu

> 
> Thanks, Anand
> 
>> Then we can have the following weird device path:
>>
>>   BTRFS: device fsid 2378be81-fe12-46d2-a9e8-68cf08dd98d5 devid 1 
>> transid 7 /proc/self/fd/3 (253:2) scanned by mount_by_fd (18440)
>>
>> Normally it's not a big deal, and later udev can trigger a device path
>> rename. But if udev didn't trigger, the device path "/proc/self/fd/3"
>> will show up in mtab.
>>
>> [CAUSE]
>> For filename "/proc/self/fd/3", it means the opened file descriptor 3.
>> In above case, it's exactly the device we want to open, aka points to
>> "/dev/test/scratch1" which is another softlink pointing to "/dev/dm-2".
>>
>> Inside kernel we solve the mount source using LOOKUP_FOLLOW, which
>> follows the symbolic link and grab the proper block device.
>>
>> But inside btrfs we also save the filename into btrfs_device::name, and
>> utilize that member to report our mount source, which leads to the above
>> situation.
>>
>> [FIX]
>> Instead of unconditionally trust the path, check if the original file
>> (not following the symbolic link) is inside "/dev/", if not, then
>> manually lookup the path to its final destination, and use that as our
>> device path.
>>
>> This allows us to still use symbolic links, like
>> "/dev/mapper/test-scratch" from LVM2, which is required for fstests runs
>> with LVM2 setup.
>>
>> And for really weird names, like the above case, we solve it to
>> "/dev/dm-2" instead.
>>
>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
>> Reported-by: Fabian Vogt <fvogt@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/volumes.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 78 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index b713e4ebb362..668138451f7c 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -732,6 +732,70 @@ const u8 *btrfs_sb_fsid_ptr(const struct 
>> btrfs_super_block *sb)
>>       return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
>>   }
>> +/*
>> + * We can have very weird soft links passed in.
>> + * One example is "/proc/self/fd/<fd>", which can be a soft link to
>> + * a block device.
>> + *
>> + * But it's never a good idea to use those weird names.
>> + * Here we check if the path (not following symlinks) is a good one 
>> inside
>> + * "/dev/".
>> + */
>> +static bool is_good_dev_path(const char *dev_path)
>> +{
>> +    struct path path = { .mnt = NULL, .dentry = NULL };
>> +    char *path_buf = NULL;
>> +    char *resolved_path;
>> +    bool is_good = false;
>> +    int ret;
>> +
>> +    path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
>> +    if (!path_buf)
>> +        goto out;
>> +
>> +    /*
>> +     * Do not follow soft link, just check if the original path is 
>> inside
>> +     * "/dev/".
>> +     */
>> +    ret = kern_path(dev_path, 0, &path);
>> +    if (ret)
>> +        goto out;
>> +    resolved_path = d_path(&path, path_buf, PATH_MAX);
>> +    if (IS_ERR(resolved_path))
>> +        goto out;
>> +    if (strncmp(resolved_path, "/dev/", strlen("/dev/")))
>> +        goto out;
>> +    is_good = true;
>> +out:
>> +    kfree(path_buf);
>> +    path_put(&path);
>> +    return is_good;
>> +}
>> +
>> +static int get_canonical_dev_path(const char *dev_path, char *canonical)
>> +{
>> +    struct path path = { .mnt = NULL, .dentry = NULL };
>> +    char *path_buf = NULL;
>> +    char *resolved_path;
>> +    int ret;
>> +
>> +    path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
>> +    if (!path_buf) {
>> +        ret = -ENOMEM;
>> +        goto out;
>> +    }
>> +
>> +    ret = kern_path(dev_path, LOOKUP_FOLLOW, &path);
>> +    if (ret)
>> +        goto out;
>> +    resolved_path = d_path(&path, path_buf, PATH_MAX);
>> +    ret = strscpy(canonical, resolved_path, PATH_MAX);
>> +out:
>> +    kfree(path_buf);
>> +    path_put(&path);
>> +    return ret;
>> +}
>> +
>>   static bool is_same_device(struct btrfs_device *device, const char 
>> *new_path)
>>   {
>>       struct path old = { .mnt = NULL, .dentry = NULL };
>> @@ -1408,12 +1472,23 @@ struct btrfs_device 
>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>       bool new_device_added = false;
>>       struct btrfs_device *device = NULL;
>>       struct file *bdev_file;
>> +    char *canonical_path = NULL;
>>       u64 bytenr;
>>       dev_t devt;
>>       int ret;
>>       lockdep_assert_held(&uuid_mutex);
>> +    if (!is_good_dev_path(path)) {
>> +        canonical_path = kmalloc(PATH_MAX, GFP_KERNEL);
>> +        if (canonical_path) {
>> +            ret = get_canonical_dev_path(path, canonical_path);
>> +            if (ret < 0) {
>> +                kfree(canonical_path);
>> +                canonical_path = NULL;
>> +            }
>> +        }
>> +    }
>>       /*
>>        * Avoid an exclusive open here, as the systemd-udev may 
>> initiate the
>>        * device scan which may race with the user's mount or mkfs 
>> command,
>> @@ -1458,7 +1533,8 @@ struct btrfs_device *btrfs_scan_one_device(const 
>> char *path, blk_mode_t flags,
>>           goto free_disk_super;
>>       }
>> -    device = device_list_add(path, disk_super, &new_device_added);
>> +    device = device_list_add(canonical_path ? : path, disk_super,
>> +                 &new_device_added);
>>       if (!IS_ERR(device) && new_device_added)
>>           btrfs_free_stale_devices(device->devt, device);
>> @@ -1467,6 +1543,7 @@ struct btrfs_device *btrfs_scan_one_device(const 
>> char *path, blk_mode_t flags,
>>   error_bdev_put:
>>       fput(bdev_file);
>> +    kfree(canonical_path);
>>       return device;
>>   }
> 

Re: [PATCH v2 2/2] btrfs: canonicalize the device path before adding it

[Anand Jain]

On 30/9/24 11:01 am, Qu Wenruo wrote:
> 
> 
> 在 2024/9/30 14:56, Anand Jain 写道:
>> On 25/9/24 5:36 am, Qu Wenruo wrote:
>>> [PROBLEM]
>>> Currently btrfs accepts any file path for its device, resulting some
>>> weird situation:
>>>
>>
>>
>>>   # ./mount_by_fd /dev/test/scratch1  /mnt/btrfs/
>>>
>>> The program has the following source code:
>>>
>>>   #include <fcntl.h>
>>>   #include <stdio.h>
>>>   #include <sys/mount.h>
>>>
>>>   int main(int argc, char *argv[]) {
>>>     int fd = open(argv[1], O_RDWR);
>>>     char path[256];
>>>     snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
>>>     return mount(path, argv[2], "btrfs", 0, NULL);
>>>   }
>>>
>>
>> Sorry for the late comments. Patches look good.
>>

>> Quick question: both XFS and ext4 fail this test case—any idea why?
> 
> Because they are single device filesystems (except the log device), thus 
> they do not need to nor are able to update their device path halfway.
> 

How is a single or multi-device filesystem relevant here?

>> Can a generic/btrfs specific testcase be added to fstests?
> 
> I can add it as a btrfs specific one, but I guess not a generic one?
> 

Yes, you can make it a Btrfs-specific test case, as XFS and EXT4
currently fail on this test.

Thanks, Anand

> Thanks,
> Qu
> 
>>
>> Thanks, Anand
>>
>>> Then we can have the following weird device path:
>>>
>>>   BTRFS: device fsid 2378be81-fe12-46d2-a9e8-68cf08dd98d5 devid 1 
>>> transid 7 /proc/self/fd/3 (253:2) scanned by mount_by_fd (18440)
>>>
>>> Normally it's not a big deal, and later udev can trigger a device path
>>> rename. But if udev didn't trigger, the device path "/proc/self/fd/3"
>>> will show up in mtab.
>>>
>>> [CAUSE]
>>> For filename "/proc/self/fd/3", it means the opened file descriptor 3.
>>> In above case, it's exactly the device we want to open, aka points to
>>> "/dev/test/scratch1" which is another softlink pointing to "/dev/dm-2".
>>>
>>> Inside kernel we solve the mount source using LOOKUP_FOLLOW, which
>>> follows the symbolic link and grab the proper block device.
>>>
>>> But inside btrfs we also save the filename into btrfs_device::name, and
>>> utilize that member to report our mount source, which leads to the above
>>> situation.
>>>
>>> [FIX]
>>> Instead of unconditionally trust the path, check if the original file
>>> (not following the symbolic link) is inside "/dev/", if not, then
>>> manually lookup the path to its final destination, and use that as our
>>> device path.
>>>
>>> This allows us to still use symbolic links, like
>>> "/dev/mapper/test-scratch" from LVM2, which is required for fstests runs
>>> with LVM2 setup.
>>>
>>> And for really weird names, like the above case, we solve it to
>>> "/dev/dm-2" instead.
>>>
>>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
>>> Reported-by: Fabian Vogt <fvogt@suse.com>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   fs/btrfs/volumes.c | 79 +++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 78 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index b713e4ebb362..668138451f7c 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -732,6 +732,70 @@ const u8 *btrfs_sb_fsid_ptr(const struct 
>>> btrfs_super_block *sb)
>>>       return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
>>>   }
>>> +/*
>>> + * We can have very weird soft links passed in.
>>> + * One example is "/proc/self/fd/<fd>", which can be a soft link to
>>> + * a block device.
>>> + *
>>> + * But it's never a good idea to use those weird names.
>>> + * Here we check if the path (not following symlinks) is a good one 
>>> inside
>>> + * "/dev/".
>>> + */
>>> +static bool is_good_dev_path(const char *dev_path)
>>> +{
>>> +    struct path path = { .mnt = NULL, .dentry = NULL };
>>> +    char *path_buf = NULL;
>>> +    char *resolved_path;
>>> +    bool is_good = false;
>>> +    int ret;
>>> +
>>> +    path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
>>> +    if (!path_buf)
>>> +        goto out;
>>> +
>>> +    /*
>>> +     * Do not follow soft link, just check if the original path is 
>>> inside
>>> +     * "/dev/".
>>> +     */
>>> +    ret = kern_path(dev_path, 0, &path);
>>> +    if (ret)
>>> +        goto out;
>>> +    resolved_path = d_path(&path, path_buf, PATH_MAX);
>>> +    if (IS_ERR(resolved_path))
>>> +        goto out;
>>> +    if (strncmp(resolved_path, "/dev/", strlen("/dev/")))
>>> +        goto out;
>>> +    is_good = true;
>>> +out:
>>> +    kfree(path_buf);
>>> +    path_put(&path);
>>> +    return is_good;
>>> +}
>>> +
>>> +static int get_canonical_dev_path(const char *dev_path, char 
>>> *canonical)
>>> +{
>>> +    struct path path = { .mnt = NULL, .dentry = NULL };
>>> +    char *path_buf = NULL;
>>> +    char *resolved_path;
>>> +    int ret;
>>> +
>>> +    path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
>>> +    if (!path_buf) {
>>> +        ret = -ENOMEM;
>>> +        goto out;
>>> +    }
>>> +
>>> +    ret = kern_path(dev_path, LOOKUP_FOLLOW, &path);
>>> +    if (ret)
>>> +        goto out;
>>> +    resolved_path = d_path(&path, path_buf, PATH_MAX);
>>> +    ret = strscpy(canonical, resolved_path, PATH_MAX);
>>> +out:
>>> +    kfree(path_buf);
>>> +    path_put(&path);
>>> +    return ret;
>>> +}
>>> +
>>>   static bool is_same_device(struct btrfs_device *device, const char 
>>> *new_path)
>>>   {
>>>       struct path old = { .mnt = NULL, .dentry = NULL };
>>> @@ -1408,12 +1472,23 @@ struct btrfs_device 
>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>       bool new_device_added = false;
>>>       struct btrfs_device *device = NULL;
>>>       struct file *bdev_file;
>>> +    char *canonical_path = NULL;
>>>       u64 bytenr;
>>>       dev_t devt;
>>>       int ret;
>>>       lockdep_assert_held(&uuid_mutex);
>>> +    if (!is_good_dev_path(path)) {
>>> +        canonical_path = kmalloc(PATH_MAX, GFP_KERNEL);
>>> +        if (canonical_path) {
>>> +            ret = get_canonical_dev_path(path, canonical_path);
>>> +            if (ret < 0) {
>>> +                kfree(canonical_path);
>>> +                canonical_path = NULL;
>>> +            }
>>> +        }
>>> +    }
>>>       /*
>>>        * Avoid an exclusive open here, as the systemd-udev may 
>>> initiate the
>>>        * device scan which may race with the user's mount or mkfs 
>>> command,
>>> @@ -1458,7 +1533,8 @@ struct btrfs_device 
>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>           goto free_disk_super;
>>>       }
>>> -    device = device_list_add(path, disk_super, &new_device_added);
>>> +    device = device_list_add(canonical_path ? : path, disk_super,
>>> +                 &new_device_added);
>>>       if (!IS_ERR(device) && new_device_added)
>>>           btrfs_free_stale_devices(device->devt, device);
>>> @@ -1467,6 +1543,7 @@ struct btrfs_device 
>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>   error_bdev_put:
>>>       fput(bdev_file);
>>> +    kfree(canonical_path);
>>>       return device;
>>>   }
>>
> 

Re: [PATCH v2 2/2] btrfs: canonicalize the device path before adding it

[Qu Wenruo]

在 2024/9/30 22:10, Anand Jain 写道:
[...]
>>> Quick question: both XFS and ext4 fail this test case—any idea why?
>>
>> Because they are single device filesystems (except the log device),
>> thus they do not need to nor are able to update their device path
>> halfway.
>>
>
> How is a single or multi-device filesystem relevant here?

Since it's a single device fs, they won't bother about device scanning
nor updating their device path to handle re-occuring device.

Thus they just accept whatever path passed in.

Thanks,
Qu
>
>>> Can a generic/btrfs specific testcase be added to fstests?
>>
>> I can add it as a btrfs specific one, but I guess not a generic one?
>>
>
> Yes, you can make it a Btrfs-specific test case, as XFS and EXT4
> currently fail on this test.
>
> Thanks, Anand
>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks, Anand
>>>
>>>> Then we can have the following weird device path:
>>>>
>>>>   BTRFS: device fsid 2378be81-fe12-46d2-a9e8-68cf08dd98d5 devid 1
>>>> transid 7 /proc/self/fd/3 (253:2) scanned by mount_by_fd (18440)
>>>>
>>>> Normally it's not a big deal, and later udev can trigger a device path
>>>> rename. But if udev didn't trigger, the device path "/proc/self/fd/3"
>>>> will show up in mtab.
>>>>
>>>> [CAUSE]
>>>> For filename "/proc/self/fd/3", it means the opened file descriptor 3.
>>>> In above case, it's exactly the device we want to open, aka points to
>>>> "/dev/test/scratch1" which is another softlink pointing to "/dev/dm-2".
>>>>
>>>> Inside kernel we solve the mount source using LOOKUP_FOLLOW, which
>>>> follows the symbolic link and grab the proper block device.
>>>>
>>>> But inside btrfs we also save the filename into btrfs_device::name, and
>>>> utilize that member to report our mount source, which leads to the
>>>> above
>>>> situation.
>>>>
>>>> [FIX]
>>>> Instead of unconditionally trust the path, check if the original file
>>>> (not following the symbolic link) is inside "/dev/", if not, then
>>>> manually lookup the path to its final destination, and use that as our
>>>> device path.
>>>>
>>>> This allows us to still use symbolic links, like
>>>> "/dev/mapper/test-scratch" from LVM2, which is required for fstests
>>>> runs
>>>> with LVM2 setup.
>>>>
>>>> And for really weird names, like the above case, we solve it to
>>>> "/dev/dm-2" instead.
>>>>
>>>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
>>>> Reported-by: Fabian Vogt <fvogt@suse.com>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>   fs/btrfs/volumes.c | 79 ++++++++++++++++++++++++++++++++++++++++++
>>>> +++-
>>>>   1 file changed, 78 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index b713e4ebb362..668138451f7c 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -732,6 +732,70 @@ const u8 *btrfs_sb_fsid_ptr(const struct
>>>> btrfs_super_block *sb)
>>>>       return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
>>>>   }
>>>> +/*
>>>> + * We can have very weird soft links passed in.
>>>> + * One example is "/proc/self/fd/<fd>", which can be a soft link to
>>>> + * a block device.
>>>> + *
>>>> + * But it's never a good idea to use those weird names.
>>>> + * Here we check if the path (not following symlinks) is a good one
>>>> inside
>>>> + * "/dev/".
>>>> + */
>>>> +static bool is_good_dev_path(const char *dev_path)
>>>> +{
>>>> +    struct path path = { .mnt = NULL, .dentry = NULL };
>>>> +    char *path_buf = NULL;
>>>> +    char *resolved_path;
>>>> +    bool is_good = false;
>>>> +    int ret;
>>>> +
>>>> +    path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
>>>> +    if (!path_buf)
>>>> +        goto out;
>>>> +
>>>> +    /*
>>>> +     * Do not follow soft link, just check if the original path is
>>>> inside
>>>> +     * "/dev/".
>>>> +     */
>>>> +    ret = kern_path(dev_path, 0, &path);
>>>> +    if (ret)
>>>> +        goto out;
>>>> +    resolved_path = d_path(&path, path_buf, PATH_MAX);
>>>> +    if (IS_ERR(resolved_path))
>>>> +        goto out;
>>>> +    if (strncmp(resolved_path, "/dev/", strlen("/dev/")))
>>>> +        goto out;
>>>> +    is_good = true;
>>>> +out:
>>>> +    kfree(path_buf);
>>>> +    path_put(&path);
>>>> +    return is_good;
>>>> +}
>>>> +
>>>> +static int get_canonical_dev_path(const char *dev_path, char
>>>> *canonical)
>>>> +{
>>>> +    struct path path = { .mnt = NULL, .dentry = NULL };
>>>> +    char *path_buf = NULL;
>>>> +    char *resolved_path;
>>>> +    int ret;
>>>> +
>>>> +    path_buf = kmalloc(PATH_MAX, GFP_KERNEL);
>>>> +    if (!path_buf) {
>>>> +        ret = -ENOMEM;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    ret = kern_path(dev_path, LOOKUP_FOLLOW, &path);
>>>> +    if (ret)
>>>> +        goto out;
>>>> +    resolved_path = d_path(&path, path_buf, PATH_MAX);
>>>> +    ret = strscpy(canonical, resolved_path, PATH_MAX);
>>>> +out:
>>>> +    kfree(path_buf);
>>>> +    path_put(&path);
>>>> +    return ret;
>>>> +}
>>>> +
>>>>   static bool is_same_device(struct btrfs_device *device, const char
>>>> *new_path)
>>>>   {
>>>>       struct path old = { .mnt = NULL, .dentry = NULL };
>>>> @@ -1408,12 +1472,23 @@ struct btrfs_device
>>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>>       bool new_device_added = false;
>>>>       struct btrfs_device *device = NULL;
>>>>       struct file *bdev_file;
>>>> +    char *canonical_path = NULL;
>>>>       u64 bytenr;
>>>>       dev_t devt;
>>>>       int ret;
>>>>       lockdep_assert_held(&uuid_mutex);
>>>> +    if (!is_good_dev_path(path)) {
>>>> +        canonical_path = kmalloc(PATH_MAX, GFP_KERNEL);
>>>> +        if (canonical_path) {
>>>> +            ret = get_canonical_dev_path(path, canonical_path);
>>>> +            if (ret < 0) {
>>>> +                kfree(canonical_path);
>>>> +                canonical_path = NULL;
>>>> +            }
>>>> +        }
>>>> +    }
>>>>       /*
>>>>        * Avoid an exclusive open here, as the systemd-udev may
>>>> initiate the
>>>>        * device scan which may race with the user's mount or mkfs
>>>> command,
>>>> @@ -1458,7 +1533,8 @@ struct btrfs_device
>>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>>           goto free_disk_super;
>>>>       }
>>>> -    device = device_list_add(path, disk_super, &new_device_added);
>>>> +    device = device_list_add(canonical_path ? : path, disk_super,
>>>> +                 &new_device_added);
>>>>       if (!IS_ERR(device) && new_device_added)
>>>>           btrfs_free_stale_devices(device->devt, device);
>>>> @@ -1467,6 +1543,7 @@ struct btrfs_device
>>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>>   error_bdev_put:
>>>>       fput(bdev_file);
>>>> +    kfree(canonical_path);
>>>>       return device;
>>>>   }
>>>
>>
>
>

Re: [PATCH v2 1/2] btrfs: avoid unnecessary device path update for the same device

[Filipe Manana]

On Wed, Sep 25, 2024 at 1:14 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [PROBLEM]
> It is very common for udev to trigger device scan, and every time a
> mounted btrfs device got re-scan from different soft links, we will get
> some of unnecessary device path updates, this is especially common
> for LVM based storage:
>
>  # lvs
>   scratch1 test -wi-ao---- 10.00g
>   scratch2 test -wi-a----- 10.00g
>   scratch3 test -wi-a----- 10.00g
>   scratch4 test -wi-a----- 10.00g
>   scratch5 test -wi-a----- 10.00g
>   test     test -wi-a----- 10.00g
>
>  # mkfs.btrfs -f /dev/test/scratch1
>  # mount /dev/test/scratch1 /mnt/btrfs
>  # dmesg -c
>  [  205.705234] BTRFS: device fsid 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9 devid 1 transid 6 /dev/mapper/test-scratch1 (253:4) scanned by mount (1154)
>  [  205.710864] BTRFS info (device dm-4): first mount of filesystem 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9
>  [  205.711923] BTRFS info (device dm-4): using crc32c (crc32c-intel) checksum algorithm
>  [  205.713856] BTRFS info (device dm-4): using free-space-tree
>  [  205.722324] BTRFS info (device dm-4): checking UUID tree
>
> So far so good, but even if we just touched any soft link of
> "dm-4", we will get quite some unnecessary device path updates.
>
>  # touch /dev/mapper/test-scratch1
>  # dmesg -c
>  [  469.295796] BTRFS info: devid 1 device path /dev/mapper/test-scratch1 changed to /dev/dm-4 scanned by (udev-worker) (1221)
>  [  469.300494] BTRFS info: devid 1 device path /dev/dm-4 changed to /dev/mapper/test-scratch1 scanned by (udev-worker) (1221)
>
> Such device path rename is unnecessary and can lead to random path
> change due to the udev race.
>
> [CAUSE]
> Inside device_list_add(), we are using a very primitive way checking if
> the device has changed, strcmp().
>
> Which can never handle links well, no matter if it's hard or soft links.
>
> So every different link of the same device will be treated as a different
> device, causing the unnecessary device path update.
>
> [FIX]
> Introduce a helper, is_same_device(), and use path_equal() to properly
> detect the same block device.
> So that the different soft links won't trigger the rename race.
>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
> Reported-by: Fabian Vogt <fvogt@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/volumes.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 995b0647f538..b713e4ebb362 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -732,6 +732,32 @@ const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb)
>         return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
>  }
>
> +static bool is_same_device(struct btrfs_device *device, const char *new_path)
> +{
> +       struct path old = { .mnt = NULL, .dentry = NULL };
> +       struct path new = { .mnt = NULL, .dentry = NULL };
> +       char *old_path;
> +       bool is_same = false;
> +       int ret;
> +
> +       if (!device->name)
> +               goto out;
> +
> +       old_path = rcu_str_deref(device->name);

There's a missing rcu_read_lock / unlock btw. It's triggering warnings
in the test vms.

Thanks.

> +       ret = kern_path(old_path, LOOKUP_FOLLOW, &old);
> +       if (ret)
> +               goto out;
> +       ret = kern_path(new_path, LOOKUP_FOLLOW, &new);
> +       if (ret)
> +               goto out;
> +       if (path_equal(&old, &new))
> +               is_same = true;
> +out:
> +       path_put(&old);
> +       path_put(&new);
> +       return is_same;
> +}
> +
>  /*
>   * Add new device to list of registered devices
>   *
> @@ -852,7 +878,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>                                 MAJOR(path_devt), MINOR(path_devt),
>                                 current->comm, task_pid_nr(current));
>
> -       } else if (!device->name || strcmp(device->name->str, path)) {
> +       } else if (!device->name || !is_same_device(device, path)) {
>                 /*
>                  * When FS is already mounted.
>                  * 1. If you are here and if the device->name is NULL that
> --
> 2.46.1
>
>

Re: [PATCH v2 1/2] btrfs: avoid unnecessary device path update for the same device

[Qu Wenruo]

在 2024/10/2 23:44, Filipe Manana 写道:
> On Wed, Sep 25, 2024 at 1:14 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [PROBLEM]
>> It is very common for udev to trigger device scan, and every time a
>> mounted btrfs device got re-scan from different soft links, we will get
>> some of unnecessary device path updates, this is especially common
>> for LVM based storage:
>>
>>   # lvs
>>    scratch1 test -wi-ao---- 10.00g
>>    scratch2 test -wi-a----- 10.00g
>>    scratch3 test -wi-a----- 10.00g
>>    scratch4 test -wi-a----- 10.00g
>>    scratch5 test -wi-a----- 10.00g
>>    test     test -wi-a----- 10.00g
>>
>>   # mkfs.btrfs -f /dev/test/scratch1
>>   # mount /dev/test/scratch1 /mnt/btrfs
>>   # dmesg -c
>>   [  205.705234] BTRFS: device fsid 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9 devid 1 transid 6 /dev/mapper/test-scratch1 (253:4) scanned by mount (1154)
>>   [  205.710864] BTRFS info (device dm-4): first mount of filesystem 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9
>>   [  205.711923] BTRFS info (device dm-4): using crc32c (crc32c-intel) checksum algorithm
>>   [  205.713856] BTRFS info (device dm-4): using free-space-tree
>>   [  205.722324] BTRFS info (device dm-4): checking UUID tree
>>
>> So far so good, but even if we just touched any soft link of
>> "dm-4", we will get quite some unnecessary device path updates.
>>
>>   # touch /dev/mapper/test-scratch1
>>   # dmesg -c
>>   [  469.295796] BTRFS info: devid 1 device path /dev/mapper/test-scratch1 changed to /dev/dm-4 scanned by (udev-worker) (1221)
>>   [  469.300494] BTRFS info: devid 1 device path /dev/dm-4 changed to /dev/mapper/test-scratch1 scanned by (udev-worker) (1221)
>>
>> Such device path rename is unnecessary and can lead to random path
>> change due to the udev race.
>>
>> [CAUSE]
>> Inside device_list_add(), we are using a very primitive way checking if
>> the device has changed, strcmp().
>>
>> Which can never handle links well, no matter if it's hard or soft links.
>>
>> So every different link of the same device will be treated as a different
>> device, causing the unnecessary device path update.
>>
>> [FIX]
>> Introduce a helper, is_same_device(), and use path_equal() to properly
>> detect the same block device.
>> So that the different soft links won't trigger the rename race.
>>
>> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
>> Reported-by: Fabian Vogt <fvogt@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/volumes.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 995b0647f538..b713e4ebb362 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -732,6 +732,32 @@ const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb)
>>          return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
>>   }
>>
>> +static bool is_same_device(struct btrfs_device *device, const char *new_path)
>> +{
>> +       struct path old = { .mnt = NULL, .dentry = NULL };
>> +       struct path new = { .mnt = NULL, .dentry = NULL };
>> +       char *old_path;
>> +       bool is_same = false;
>> +       int ret;
>> +
>> +       if (!device->name)
>> +               goto out;
>> +
>> +       old_path = rcu_str_deref(device->name);
>
> There's a missing rcu_read_lock / unlock btw. It's triggering warnings
> in the test vms.

Thanks a lot, I was also wondering if it's the case, but the zoned code
gave me a bad example of not calling rcu_read_lock().

Shouldn't all btrfs_dev_name() and the usages inside zoned code also be
fixed?

Thanks,
Qu
>
> Thanks.
>
>> +       ret = kern_path(old_path, LOOKUP_FOLLOW, &old);
>> +       if (ret)
>> +               goto out;
>> +       ret = kern_path(new_path, LOOKUP_FOLLOW, &new);
>> +       if (ret)
>> +               goto out;
>> +       if (path_equal(&old, &new))
>> +               is_same = true;
>> +out:
>> +       path_put(&old);
>> +       path_put(&new);
>> +       return is_same;
>> +}
>> +
>>   /*
>>    * Add new device to list of registered devices
>>    *
>> @@ -852,7 +878,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>>                                  MAJOR(path_devt), MINOR(path_devt),
>>                                  current->comm, task_pid_nr(current));
>>
>> -       } else if (!device->name || strcmp(device->name->str, path)) {
>> +       } else if (!device->name || !is_same_device(device, path)) {
>>                  /*
>>                   * When FS is already mounted.
>>                   * 1. If you are here and if the device->name is NULL that
>> --
>> 2.46.1
>>
>>
>

Re: [PATCH v2 1/2] btrfs: avoid unnecessary device path update for the same device

[Filipe Manana]

On Wed, Oct 2, 2024 at 10:09 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2024/10/2 23:44, Filipe Manana 写道:
> > On Wed, Sep 25, 2024 at 1:14 AM Qu Wenruo <wqu@suse.com> wrote:
> >>
> >> [PROBLEM]
> >> It is very common for udev to trigger device scan, and every time a
> >> mounted btrfs device got re-scan from different soft links, we will get
> >> some of unnecessary device path updates, this is especially common
> >> for LVM based storage:
> >>
> >>   # lvs
> >>    scratch1 test -wi-ao---- 10.00g
> >>    scratch2 test -wi-a----- 10.00g
> >>    scratch3 test -wi-a----- 10.00g
> >>    scratch4 test -wi-a----- 10.00g
> >>    scratch5 test -wi-a----- 10.00g
> >>    test     test -wi-a----- 10.00g
> >>
> >>   # mkfs.btrfs -f /dev/test/scratch1
> >>   # mount /dev/test/scratch1 /mnt/btrfs
> >>   # dmesg -c
> >>   [  205.705234] BTRFS: device fsid 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9 devid 1 transid 6 /dev/mapper/test-scratch1 (253:4) scanned by mount (1154)
> >>   [  205.710864] BTRFS info (device dm-4): first mount of filesystem 7be2602f-9e35-4ecf-a6ff-9e91d2c182c9
> >>   [  205.711923] BTRFS info (device dm-4): using crc32c (crc32c-intel) checksum algorithm
> >>   [  205.713856] BTRFS info (device dm-4): using free-space-tree
> >>   [  205.722324] BTRFS info (device dm-4): checking UUID tree
> >>
> >> So far so good, but even if we just touched any soft link of
> >> "dm-4", we will get quite some unnecessary device path updates.
> >>
> >>   # touch /dev/mapper/test-scratch1
> >>   # dmesg -c
> >>   [  469.295796] BTRFS info: devid 1 device path /dev/mapper/test-scratch1 changed to /dev/dm-4 scanned by (udev-worker) (1221)
> >>   [  469.300494] BTRFS info: devid 1 device path /dev/dm-4 changed to /dev/mapper/test-scratch1 scanned by (udev-worker) (1221)
> >>
> >> Such device path rename is unnecessary and can lead to random path
> >> change due to the udev race.
> >>
> >> [CAUSE]
> >> Inside device_list_add(), we are using a very primitive way checking if
> >> the device has changed, strcmp().
> >>
> >> Which can never handle links well, no matter if it's hard or soft links.
> >>
> >> So every different link of the same device will be treated as a different
> >> device, causing the unnecessary device path update.
> >>
> >> [FIX]
> >> Introduce a helper, is_same_device(), and use path_equal() to properly
> >> detect the same block device.
> >> So that the different soft links won't trigger the rename race.
> >>
> >> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230641
> >> Reported-by: Fabian Vogt <fvogt@suse.com>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>   fs/btrfs/volumes.c | 28 +++++++++++++++++++++++++++-
> >>   1 file changed, 27 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index 995b0647f538..b713e4ebb362 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -732,6 +732,32 @@ const u8 *btrfs_sb_fsid_ptr(const struct btrfs_super_block *sb)
> >>          return has_metadata_uuid ? sb->metadata_uuid : sb->fsid;
> >>   }
> >>
> >> +static bool is_same_device(struct btrfs_device *device, const char *new_path)
> >> +{
> >> +       struct path old = { .mnt = NULL, .dentry = NULL };
> >> +       struct path new = { .mnt = NULL, .dentry = NULL };
> >> +       char *old_path;
> >> +       bool is_same = false;
> >> +       int ret;
> >> +
> >> +       if (!device->name)
> >> +               goto out;
> >> +
> >> +       old_path = rcu_str_deref(device->name);
> >
> > There's a missing rcu_read_lock / unlock btw. It's triggering warnings
> > in the test vms.
>
> Thanks a lot, I was also wondering if it's the case, but the zoned code
> gave me a bad example of not calling rcu_read_lock().

It doesn't call rcu_read_lock() explicitly because it uses the
btrfs_*_in_rcu() log functions, which do the locking.
Except for one place for which I've sent a patch earlier today.

>
> Shouldn't all btrfs_dev_name() and the usages inside zoned code also be
> fixed?

Except for that single case, there's no other case anywhere else,
zoned code or non-zoned code.

>
> Thanks,
> Qu
> >
> > Thanks.
> >
> >> +       ret = kern_path(old_path, LOOKUP_FOLLOW, &old);
> >> +       if (ret)
> >> +               goto out;
> >> +       ret = kern_path(new_path, LOOKUP_FOLLOW, &new);
> >> +       if (ret)
> >> +               goto out;
> >> +       if (path_equal(&old, &new))
> >> +               is_same = true;
> >> +out:
> >> +       path_put(&old);
> >> +       path_put(&new);
> >> +       return is_same;
> >> +}
> >> +
> >>   /*
> >>    * Add new device to list of registered devices
> >>    *
> >> @@ -852,7 +878,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
> >>                                  MAJOR(path_devt), MINOR(path_devt),
> >>                                  current->comm, task_pid_nr(current));
> >>
> >> -       } else if (!device->name || strcmp(device->name->str, path)) {
> >> +       } else if (!device->name || !is_same_device(device, path)) {
> >>                  /*
> >>                   * When FS is already mounted.
> >>                   * 1. If you are here and if the device->name is NULL that
> >> --
> >> 2.46.1
> >>
> >>
> >
>