| Message ID | 166e39f69a8693e5fe10784cdbd950d68f98d4fb.1638953372.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: harden identification of the stale device | expand |
On Wed, Dec 08, 2021 at 10:05:29PM +0800, Anand Jain wrote: > Identifying and removing the stale device from the fs_uuids list is done > by the function btrfs_free_stale_devices(). > btrfs_free_stale_devices() in turn depends on the function > device_path_matched() to check if the device repeats in more than one > btrfs_device structure. > > The matching of the device happens by its path, the device path. However, > when dm mapper is in use, the dm device paths are nothing but a link to > the actual block device, which leads to the device_path_matched() failing > to match. > > Fix this by matching the dev_t as provided by lookup_bdev() instead of > plain strcmp() the device paths. > > Reported-by: Josef Bacik <josef@toxicpanda.com> > Signed-off-by: Anand Jain <anand.jain@oracle.com> We already have the bdev for the device in most of the callers here, the only exception is btrfs_forget_devices. Can we change this up to pass in the dev_t of the device we're trying to remove, that way we don't have to do the lookup over and over for the path of the device we're trying to forget? Thanks, Josef
Hi Anand,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.16-rc4 next-20211208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Anand-Jain/btrfs-harden-identification-of-the-stale-device/20211208-220757
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-s001-20211207 (https://download.01.org/0day-ci/archive/20211209/202112091123.ETjD5KQj-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/03b597640967afb3d37dc37f0a685fed95594b83
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Anand-Jain/btrfs-harden-identification-of-the-stale-device/20211208-220757
git checkout 03b597640967afb3d37dc37f0a685fed95594b83
# save the config file to linux build tree
mkdir build_dir
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
fs/btrfs/volumes.c:402:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct rcu_string *str @@ got struct rcu_string [noderef] __rcu *name @@
fs/btrfs/volumes.c:402:31: sparse: expected struct rcu_string *str
fs/btrfs/volumes.c:402:31: sparse: got struct rcu_string [noderef] __rcu *name
>> fs/btrfs/volumes.c:549:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *pathname @@ got char [noderef] __rcu * @@
fs/btrfs/volumes.c:549:35: sparse: expected char const *pathname
fs/btrfs/volumes.c:549:35: sparse: got char [noderef] __rcu *
fs/btrfs/volumes.c:643:43: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *device_path @@ got char [noderef] __rcu * @@
fs/btrfs/volumes.c:643:43: sparse: expected char const *device_path
fs/btrfs/volumes.c:643:43: sparse: got char [noderef] __rcu *
fs/btrfs/volumes.c:904:50: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *cs @@ got char [noderef] __rcu * @@
fs/btrfs/volumes.c:904:50: sparse: expected char const *cs
fs/btrfs/volumes.c:904:50: sparse: got char [noderef] __rcu *
fs/btrfs/volumes.c:984:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct rcu_string *str @@ got struct rcu_string [noderef] __rcu *name @@
fs/btrfs/volumes.c:984:39: sparse: expected struct rcu_string *str
fs/btrfs/volumes.c:984:39: sparse: got struct rcu_string [noderef] __rcu *name
fs/btrfs/volumes.c:1040:58: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *src @@ got char [noderef] __rcu * @@
fs/btrfs/volumes.c:1040:58: sparse: expected char const *src
fs/btrfs/volumes.c:1040:58: sparse: got char [noderef] __rcu *
fs/btrfs/volumes.c:2235:49: sparse: sparse: incorrect type in argument 3 (different address spaces) @@ expected char const *device_path @@ got char [noderef] __rcu * @@
fs/btrfs/volumes.c:2235:49: sparse: expected char const *device_path
fs/btrfs/volumes.c:2235:49: sparse: got char [noderef] __rcu *
fs/btrfs/volumes.c:2350:41: sparse: sparse: incorrect type in argument 3 (different address spaces) @@ expected char const *device_path @@ got char [noderef] __rcu * @@
fs/btrfs/volumes.c:2350:41: sparse: expected char const *device_path
fs/btrfs/volumes.c:2350:41: sparse: got char [noderef] __rcu *
vim +549 fs/btrfs/volumes.c
532
533 /*
534 * Check if the device in the 'path' matches with the device in the given
535 * struct btrfs_device '*device'.
536 * Returns:
537 * 0 If it is the same device.
538 * 1 If it is not the same device.
539 * -errno For error.
540 */
541 static int device_matched(struct btrfs_device *device, const char *path)
542 {
543 dev_t dev_old;
544 dev_t dev_new;
545 int error;
546
547 lockdep_assert_held(&device->fs_devices->device_list_mutex);
548 /* rcu is not required as we are inside the device_list_mutex */
> 549 error = lookup_bdev(device->name->str, &dev_old);
550 if (error)
551 return error;
552
553 error = lookup_bdev(path, &dev_new);
554 if (error)
555 return error;
556
557 if (dev_old == dev_new)
558 return 0;
559
560 return 1;
561 }
562
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> sparse warnings: (new ones prefixed by >>) The new Warning that this patch created is the same as the other 7 old Warnings. It is all about how we have used the device:name. I will fix the new Warning. Looks like we need to fix the older Warning as well. Thanks, Anand > fs/btrfs/volumes.c:402:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct rcu_string *str @@ got struct rcu_string [noderef] __rcu *name @@ > fs/btrfs/volumes.c:402:31: sparse: expected struct rcu_string *str > fs/btrfs/volumes.c:402:31: sparse: got struct rcu_string [noderef] __rcu *name rcu_string_free(device->name); >>> fs/btrfs/volumes.c:549:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *pathname @@ got char [noderef] __rcu * @@ > fs/btrfs/volumes.c:549:35: sparse: expected char const *pathname > fs/btrfs/volumes.c:549:35: sparse: got char [noderef] __rcu * error = lookup_bdev(device->name->str, &dev_old); > fs/btrfs/volumes.c:643:43: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *device_path @@ got char [noderef] __rcu * @@ > fs/btrfs/volumes.c:643:43: sparse: expected char const *device_path > fs/btrfs/volumes.c:643:43: sparse: got char [noderef] __rcu * ret = btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1, &bdev, &disk_super); > fs/btrfs/volumes.c:904:50: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *cs @@ got char [noderef] __rcu * @@ > fs/btrfs/volumes.c:904:50: sparse: expected char const *cs > fs/btrfs/volumes.c:904:50: sparse: got char [noderef] __rcu * } else if (!device->name || strcmp(device->name->str, path)) { > fs/btrfs/volumes.c:984:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct rcu_string *str @@ got struct rcu_string [noderef] __rcu *name @@ > fs/btrfs/volumes.c:984:39: sparse: expected struct rcu_string *str > fs/btrfs/volumes.c:984:39: sparse: got struct rcu_string [noderef] __rcu *name rcu_string_free(device->name); > fs/btrfs/volumes.c:1040:58: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected char const *src @@ got char [noderef] __rcu * @@ > fs/btrfs/volumes.c:1040:58: sparse: expected char const *src > fs/btrfs/volumes.c:1040:58: sparse: got char [noderef] __rcu * name = rcu_string_strdup(orig_dev->name->str, GFP_KERNEL); > fs/btrfs/volumes.c:2235:49: sparse: sparse: incorrect type in argument 3 (different address spaces) @@ expected char const *device_path @@ got char [noderef] __rcu * @@ > fs/btrfs/volumes.c:2235:49: sparse: expected char const *device_path > fs/btrfs/volumes.c:2235:49: sparse: got char [noderef] __rcu * btrfs_scratch_superblocks(fs_info, device->bdev, device->name->str); > fs/btrfs/volumes.c:2350:41: sparse: sparse: incorrect type in argument 3 (different address spaces) @@ expected char const *device_path @@ got char [noderef] __rcu * @@ > fs/btrfs/volumes.c:2350:41: sparse: expected char const *device_path > fs/btrfs/volumes.c:2350:41: sparse: got char [noderef] __rcu * btrfs_scratch_superblocks(tgtdev->fs_info, tgtdev->bdev, tgtdev->name->str);
On 08/12/2021 22:29, Josef Bacik wrote: > On Wed, Dec 08, 2021 at 10:05:29PM +0800, Anand Jain wrote: >> Identifying and removing the stale device from the fs_uuids list is done >> by the function btrfs_free_stale_devices(). >> btrfs_free_stale_devices() in turn depends on the function >> device_path_matched() to check if the device repeats in more than one >> btrfs_device structure. >> >> The matching of the device happens by its path, the device path. However, >> when dm mapper is in use, the dm device paths are nothing but a link to >> the actual block device, which leads to the device_path_matched() failing >> to match. >> >> Fix this by matching the dev_t as provided by lookup_bdev() instead of >> plain strcmp() the device paths. >> >> Reported-by: Josef Bacik <josef@toxicpanda.com> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > We already have the bdev for the device in most of the callers here, the only > exception is btrfs_forget_devices. Can we change this up to pass in the dev_t > of the device we're trying to remove, that way we don't have to do the lookup > over and over for the path of the device we're trying to forget? Thanks, Right. I have made the related changes and, in v2, this change is on top of this patch. Thanks, Anand > > Josef
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 553ee97078f6..cedadc81c851 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -535,15 +535,34 @@ btrfs_get_bdev_and_sb(const char *device_path, fmode_t flags, void *holder, return ret; } -static bool device_path_matched(const char *path, struct btrfs_device *device) +/* + * Check if the device in the 'path' matches with the device in the given + * struct btrfs_device '*device'. + * Returns: + * 0 If it is the same device. + * 1 If it is not the same device. + * -errno For error. + */ +static int device_matched(struct btrfs_device *device, const char *path) { - int found; + dev_t dev_old; + dev_t dev_new; + int error; - rcu_read_lock(); - found = strcmp(rcu_str_deref(device->name), path); - rcu_read_unlock(); + lockdep_assert_held(&device->fs_devices->device_list_mutex); + /* rcu is not required as we are inside the device_list_mutex */ + error = lookup_bdev(device->name->str, &dev_old); + if (error) + return error; - return found == 0; + error = lookup_bdev(path, &dev_new); + if (error) + return error; + + if (dev_old == dev_new) + return 0; + + return 1; } /* @@ -578,7 +597,7 @@ static int btrfs_free_stale_devices(const char *path, continue; if (path && !device->name) continue; - if (path && !device_path_matched(path, device)) + if (path && device_matched(device, path) != 0) continue; if (fs_devices->opened) { /* for an already deleted device return 0 */
Identifying and removing the stale device from the fs_uuids list is done by the function btrfs_free_stale_devices(). btrfs_free_stale_devices() in turn depends on the function device_path_matched() to check if the device repeats in more than one btrfs_device structure. The matching of the device happens by its path, the device path. However, when dm mapper is in use, the dm device paths are nothing but a link to the actual block device, which leads to the device_path_matched() failing to match. Fix this by matching the dev_t as provided by lookup_bdev() instead of plain strcmp() the device paths. Reported-by: Josef Bacik <josef@toxicpanda.com> Signed-off-by: Anand Jain <anand.jain@oracle.com> --- This patch should go to Stable-5.4.y and up but, there is a conflict. I will send a separate patch for the stable. fs/btrfs/volumes.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)