[PATCH RESEND v8 2/3] btrfs: refactor btrfs_device->name updates

[Omar Sandoval]

The rcu_string API introduced some new sparse errors but also revealed
existing ones. First of all, the name in struct btrfs_device should be
annotated as __rcu to prevent unsafe reads. Additionally, updates should
go through rcu_dereference_protected to make it clear what's going on.
This introduces some helper functions that factor out this
functionality.

Signed-off-by: Omar Sandoval <osandov@xxxxxxxxxxx>
---
 fs/btrfs/volumes.c | 93 +++++++++++++++++++++++++++++++++++++-----------------
 fs/btrfs/volumes.h |  2 +-
 2 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2231275..ddd7483 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -53,6 +53,45 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *device);
 DEFINE_MUTEX(uuid_mutex);
 static LIST_HEAD(fs_uuids);
 
+/*
+ * Dereference the device name under the uuid_mutex.
+ */
+static inline struct rcu_string *
+btrfs_dev_rcu_protected_name(struct btrfs_device *dev)
+__must_hold(&uuid_mutex)
+{
+	return rcu_dereference_protected(dev->name,
+					 lockdep_is_held(&uuid_mutex));
+}
+
+/*
+ * Use when the caller is the only possible updater.
+ */
+static inline struct rcu_string *
+btrfs_dev_rcu_only_name(struct btrfs_device *dev)
+{
+	return rcu_dereference_protected(dev->name, 1);
+}
+
+/*
+ * Rename a device under the uuid_mutex.
+ */
+static inline int btrfs_dev_rename(struct btrfs_device *dev, const char *name)
+__must_hold(&uuid_mutex)
+{
+	struct rcu_string *old_name, *new_name;
+
+	new_name = rcu_string_strdup(name, GFP_NOFS);
+	if (!new_name)
+		return -ENOMEM;
+
+	old_name = btrfs_dev_rcu_protected_name(dev);
+	rcu_assign_pointer(dev->name, new_name);
+	rcu_string_free(old_name);
+
+	return 0;
+}
+
 static struct btrfs_fs_devices *__alloc_fs_devices(void)
 {
 	struct btrfs_fs_devices *fs_devs;
@@ -104,7 +143,7 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
 		device = list_entry(fs_devices->devices.next,
 				    struct btrfs_device, dev_list);
 		list_del(&device->dev_list);
-		rcu_string_free(device->name);
+		rcu_string_free(btrfs_dev_rcu_only_name(device));
 		kfree(device);
 	}
 	kfree(fs_devices);
@@ -485,12 +524,10 @@ static noinline int device_list_add(const char *path,
 			return PTR_ERR(device);
 		}
 
-		name = rcu_string_strdup(path, GFP_NOFS);
-		if (!name) {
+		if (btrfs_dev_rename(device, path)) {
 			kfree(device);
 			return -ENOMEM;
 		}
-		rcu_assign_pointer(device->name, name);
 
 		mutex_lock(&fs_devices->device_list_mutex);
 		list_add_rcu(&device->dev_list, &fs_devices->devices);
@@ -499,7 +536,11 @@ static noinline int device_list_add(const char *path,
 
 		ret = 1;
 		device->fs_devices = fs_devices;
-	} else if (!device->name || strcmp(device->name->str, path)) {
+	} else {
+		name = btrfs_dev_rcu_protected_name(device);
+		if (name && strcmp(name->str, path) == 0)
+			goto out;
+
 		/*
 		 * When FS is already mounted.
 		 * 1. If you are here and if the device->name is NULL that
@@ -537,17 +578,15 @@ static noinline int device_list_add(const char *path,
 			return -EEXIST;
 		}
 
-		name = rcu_string_strdup(path, GFP_NOFS);
-		if (!name)
+		if (btrfs_dev_rename(device, path))
 			return -ENOMEM;
-		rcu_string_free(device->name);
-		rcu_assign_pointer(device->name, name);
 		if (device->missing) {
 			fs_devices->missing_devices--;
 			device->missing = 0;
 		}
 	}
 
+out:
 	/*
 	 * Unmount does not free the btrfs_device struct but would zero
 	 * generation along with most of the other members. So just update
@@ -584,17 +623,12 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 		if (IS_ERR(device))
 			goto error;
 
-		/*
-		 * This is ok to do without rcu read locked because we hold the
-		 * uuid mutex so nothing we touch in here is going to disappear.
-		 */
-		if (orig_dev->name) {
-			name = rcu_string_strdup(orig_dev->name->str, GFP_NOFS);
-			if (!name) {
+		name = btrfs_dev_rcu_protected_name(orig_dev);
+		if (name) {
+			if (btrfs_dev_rename(device, name->str)) {
 				kfree(device);
 				goto error;
 			}
-			rcu_assign_pointer(device->name, name);
 		}
 
 		list_add(&device->dev_list, &fs_devices->devices);
@@ -656,7 +690,7 @@ again:
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;
-		rcu_string_free(device->name);
+		rcu_string_free(btrfs_dev_rcu_only_name(device));
 		kfree(device);
 	}
 
@@ -679,7 +713,7 @@ static void __free_device(struct work_struct *work)
 	if (device->bdev)
 		blkdev_put(device->bdev, device->mode);
 
-	rcu_string_free(device->name);
+	rcu_string_free(btrfs_dev_rcu_only_name(device));
 	kfree(device);
 }
 
@@ -721,11 +755,10 @@ static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 						device->uuid);
 		BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
 
-		/* Safe because we are under uuid_mutex */
-		if (device->name) {
-			name = rcu_string_strdup(device->name->str, GFP_NOFS);
-			BUG_ON(!name); /* -ENOMEM */
-			rcu_assign_pointer(new_device->name, name);
+		name = btrfs_dev_rcu_protected_name(device);
+		if (name) {
+			if (btrfs_dev_rename(new_device, name->str))
+				BUG_ON(1); /* -ENOMEM */
 		}
 
 		list_replace_rcu(&device->dev_list, &new_device->dev_list);
@@ -784,18 +817,20 @@ static int __btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 	u64 devid;
 	int seeding = 1;
 	int ret = 0;
+	struct rcu_string *name;
 
 	flags |= FMODE_EXCL;
 
 	list_for_each_entry(device, head, dev_list) {
 		if (device->bdev)
 			continue;
-		if (!device->name)
+		name = btrfs_dev_rcu_protected_name(device);
+		if (!name)
 			continue;
 
 		/* Just open everything we can; ignore failures here */
-		if (btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
-					    &bdev, &bh))
+		if (btrfs_get_bdev_and_sb(name->str, flags, holder, 1, &bdev,
+					  &bh))
 			continue;
 
 		disk_super = (struct btrfs_super_block *)bh->b_data;
@@ -2142,7 +2177,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
-		rcu_string_free(device->name);
+		rcu_string_free(btrfs_dev_rcu_only_name(device));
 		kfree(device);
 		ret = PTR_ERR(trans);
 		goto error;
@@ -2279,7 +2314,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
 
 error_trans:
 	btrfs_end_transaction(trans, root);
-	rcu_string_free(device->name);
+	rcu_string_free(btrfs_dev_rcu_only_name(device));
 	btrfs_kobj_rm_device(root->fs_info, device);
 	kfree(device);
 error:
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index d6fe73c..234fd20 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -54,7 +54,7 @@ struct btrfs_device {
 
 	struct btrfs_root *dev_root;
 
-	struct rcu_string *name;
+	struct rcu_string __rcu *name;
 
 	u64 generation;
 
-- 
2.2.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/