Skip to contentSkip to conversation

btrfs: fix RCU string sparse noise

PATCH In review Omar Sandoval 3 participants 8 messages Nov 30, 2014 – Dec 1, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover btrfs: fix RCU string sparse noise

Hi everyone,

These patches clean up the big stack of sparse RCU errors I introduced into the integration tree as reported by the kbuild test robot:

Quoted 10 lines · kbuild test robot
tree:   git://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs.git integration
head:   c7a37618b60026121255c69e042d74ae5631470c
commit: 37aad79d90a0cbf82a5eda62dfe3af4241f5aca3 [38/39] Move BTRFS RCU string to common library
reproduce:
  # apt-get install sparse
  git checkout 37aad79d90a0cbf82a5eda62dfe3af4241f5aca3
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__
Quoted 5 lines

sparse warnings: (new ones prefixed by >>)

Quoted 1 line
Quoted 1 line

fs/btrfs/check-integrity.c:848:25: sparse: incorrect type in argument 1 (different address spaces)

fs/btrfs/check-integrity.c:848:25: expected struct rcu_string [noderef] <asn:4>*rcu_str
fs/btrfs/check-integrity.c:848:25: got struct rcu_string *name

[snip, there's a lot of these]

As payment for my transgressions, this also clean ups the existing rcu_string usage to get rid of the preexisting noise.

The first patch fixes the __rcu annotations which I got wrong on the first go. The second fixes an incorrect use of RCU in the BTRFS_IOC_DEV_INFO ioctl. The third refactors the volume code's usage of rcu_string, fixing a questionable RCU or two in the process.

This patch series applies to Chris' integration branch.

Thanks!

Omar Sandoval (3):
rcustring: clean up botched __rcu annotations
btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO
btrfs: refactor btrfs_device->name updates

 fs/btrfs/ioctl.c          | 10 ++---
 fs/btrfs/volumes.c        | 93 ++++++++++++++++++++++++++++++++---------------
 fs/btrfs/volumes.h        |  2 +-
 include/linux/rcustring.h |  5 +--
 4 files changed, 72 insertions(+), 38 deletions(-)
Signature · 1 line
2.1.3
#1 lore raw

PATCH 1/3 rcustring: clean up botched __rcu annotations

The rcu_string returned by rcu_string_strdup isn't technically under RCU yet, and it makes more sense not to treat it as such. Additionally, an rcu_string passed to rcu_string_free should already be rcu_dereferenced and therefore not in the __rcu address space.

---
 include/linux/rcustring.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
include/linux/rcustring.h +2 -3
index 67277ab..28bd9bc 100644
@@ -37,8 +37,7 @@ struct rcu_string {
  * @src: The string to copy
  * @flags: Flags for kmalloc
  */
-static inline struct rcu_string __rcu *rcu_string_strdup(const char *src,
-							 gfp_t flags)
+static inline struct rcu_string *rcu_string_strdup(const char *src, gfp_t flags)
 {
 	struct rcu_string *ret;
 	size_t len = strlen(src) + 1;
@@ -54,7 +53,7 @@ static inline struct rcu_string __rcu *rcu_string_strdup(const char *src,
  * rcu_string_free() - free an RCU string
  * @str: The string
  */
-static inline void rcu_string_free(struct rcu_string __rcu *str)
+static inline void rcu_string_free(struct rcu_string *str)
 {
 	if (str)
 		kfree_rcu(str, rcu);
Signature · 1 line
2.1.3
#2 lore raw

PATCH 2/3 btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO

In reply to Omar Sandoval

A naked read of the value of an RCU pointer isn't safe. Put the whole access in an RCU critical section, not just the pointer dereference.

---
 fs/btrfs/ioctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
fs/btrfs/ioctl.c +5 -5
index ecdf68f..dd55844 100644
@@ -2706,6 +2706,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_root *root, void __user *arg)
 	struct btrfs_fs_devices *fs_devices = root->fs_info->fs_devices;
 	int ret = 0;
 	char *s_uuid = NULL;
+	struct rcu_string *name;
 
 	di_args = memdup_user(arg, sizeof(*di_args));
 	if (IS_ERR(di_args))
@@ -2726,17 +2727,16 @@ static long btrfs_ioctl_dev_info(struct btrfs_root *root, void __user *arg)
 	di_args->bytes_used = btrfs_device_get_bytes_used(dev);
 	di_args->total_bytes = btrfs_device_get_total_bytes(dev);
 	memcpy(di_args->uuid, dev->uuid, sizeof(di_args->uuid));
-	if (dev->name) {
-		struct rcu_string *name;
 
-		rcu_read_lock();
-		name = rcu_dereference(dev->name);
+	rcu_read_lock();
+	name = rcu_dereference(dev->name);
+	if (name) {
 		strncpy(di_args->path, name->str, sizeof(di_args->path));
-		rcu_read_unlock();
 		di_args->path[sizeof(di_args->path) - 1] = 0;
 	} else {
 		di_args->path[0] = '\0';
 	}
+	rcu_read_unlock();
 
 out:
 	mutex_unlock(&fs_devices->device_list_mutex);
Signature · 1 line
2.1.3
#3 lore raw

Re: [PATCH 2/3] btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO

Quoted 4 lines · Omar Sandoval

A naked read of the value of an RCU pointer isn't safe. Put the whole access in an RCU critical section, not just the pointer dereference.

You can use rcu_access_pointer() in the if() condition check rather than increasing the read critical section. We should try to keep the critical section as small as possible.

Also, since we have rcu_str_deref() we can use that instead of rcu_dereference() on device->name. Thoughts?

Quoted 46 lines
---
 fs/btrfs/ioctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
fs/btrfs/ioctl.c +5 -6
index ecdf68f..dd55844 100644
@@ -2706,6 +2706,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_root *root, void __user *arg)
        struct btrfs_fs_devices *fs_devices = root->fs_info->fs_devices;
        int ret = 0;
        char *s_uuid = NULL;
+       struct rcu_string *name;
 
        di_args = memdup_user(arg, sizeof(*di_args));
        if (IS_ERR(di_args))
@@ -2726,17 +2727,16 @@ static long btrfs_ioctl_dev_info(struct btrfs_root *root, void __user *arg)
        di_args->bytes_used = btrfs_device_get_bytes_used(dev);
        di_args->total_bytes = btrfs_device_get_total_bytes(dev);
        memcpy(di_args->uuid, dev->uuid, sizeof(di_args->uuid));
-       if (dev->name) {
-               struct rcu_string *name;
 
-               rcu_read_lock();
-               name = rcu_dereference(dev->name);
+       rcu_read_lock();
+       name = rcu_dereference(dev->name);
+       if (name) {
                strncpy(di_args->path, name->str, sizeof(di_args->path));
-               rcu_read_unlock();
                di_args->path[sizeof(di_args->path) - 1] = 0;
        } else {
                di_args->path[0] = '\0';
        }
+       rcu_read_unlock();
 
 out:
        mutex_unlock(&fs_devices->device_list_mutex);
--

2.1.3

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

#4 lore raw

Re: [PATCH 2/3] btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO

Quoted 4 lines · Pranith Kumar
Quoted 3 lines · Omar Sandoval

A naked read of the value of an RCU pointer isn't safe. Put the whole access in an RCU critical section, not just the pointer dereference.

Quoted 9 lines
Quoted 1 line

You can use rcu_access_pointer() in the if() condition check rather than increasing the read critical section. We should try to keep the critical section as small as possible.

Also, since we have rcu_str_deref() we can use that instead of rcu_dereference() on device->name. Thoughts?

That's right, I forgot about rcu_access_pointer. The difference is probably negligible, and I doubt the performance of this ioctl is very important. Since we're going to be dereferencing the pointer anyways in some (most?) cases, I think this is a bit more readable.

Signature · 1 line
Omar
#5 lore raw

PATCH 3/3 btrfs: refactor btrfs_device->name updates

In reply to 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.

---
 fs/btrfs/volumes.c | 93 +++++++++++++++++++++++++++++++++++++-----------------
 fs/btrfs/volumes.h |  2 +-
 2 files changed, 65 insertions(+), 30 deletions(-)
fs/btrfs/volumes.c +64 -29
index d13b253..6913bed 100644
@@ -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 void lock_chunks(struct btrfs_root *root)
 {
 	mutex_lock(&root->fs_info->chunk_mutex);
@@ -114,7 +153,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);
@@ -495,12 +534,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);
@@ -509,7 +546,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
@@ -547,17 +588,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
@@ -594,17 +633,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);
@@ -666,7 +700,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);
 	}
 
@@ -689,7 +723,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);
 }
 
@@ -731,11 +765,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);
@@ -794,18 +827,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;
@@ -2146,7 +2181,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;
@@ -2283,7 +2318,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:
fs/btrfs/volumes.h +1 -1
index 6e04f27..2298a70 100644
@@ -54,7 +54,7 @@ struct btrfs_device {
 
 	struct btrfs_root *dev_root;
 
-	struct rcu_string *name;
+	struct rcu_string __rcu *name;
 
 	u64 generation;
 
Signature · 1 line
2.1.3
#6 lore raw

Re: [PATCH 3/3] btrfs: refactor btrfs_device->name updates

Quoted 12 lines · 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.

fs/btrfs/volumes.h +0 -0
index 6e04f27..2298a70 100644

fs/btrfs/volumes.h struct btrfs_device { · line 60

Quoted 8 lines
fs/btrfs/volumes.h +1 -1
@@ -54,7 +54,7 @@ struct btrfs_device {
 
        struct btrfs_root *dev_root;
 
-       struct rcu_string *name;
+       struct rcu_string __rcu *name;
 
        u64 generation;
 

Since rcu_strings are rcu specific, why not annotate the char pointer in 'struct rcu_string' with __rcu annotation? That should catch all error-prone users of rcu_string.

Signature · 1 line
Pranith
#7 lore raw

Re: [PATCH 3/3] btrfs: refactor btrfs_device->name updates

Quoted 19 lines · Pranith Kumar
Quoted 18 lines · 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.

fs/btrfs/volumes.h +1 -1
index 6e04f27..2298a70 100644
@@ -54,7 +54,7 @@ struct btrfs_device {
 
        struct btrfs_root *dev_root;
 
-       struct rcu_string *name;
+       struct rcu_string __rcu *name;
 
Quoted 7 lines

fs/btrfs/volumes.h struct btrfs_device { · line 60

Quoted 2 lines
fs/btrfs/volumes.h +0 -0
        u64 generation;
 

Since rcu_strings are rcu specific, why not annotate the char pointer in 'struct rcu_string' with __rcu annotation? That should catch all error-prone users of rcu_string.

Because the whole structure is RCU'd, not just the str part of it. If str is annotated as __rcu, when we (correctly) rcu_dereference an rcu_string and then access the str member, we'll still get sparse warnings.

In any case, the above code does what I want it to do. See the following (non-sense but illustrative) example:

#include <linux/rcustring.h>

static void example_func(void)

{
	struct rcu_string __rcu *example;
	char *str;
	str = example->str;
}

CHECK /home/osandov/linux/example/example.c
/home/osandov/linux/example/example.c:7:13: warning: incorrect type in assignment (different address spaces)
/home/osandov/linux/example/example.c:7:13: expected char *str
/home/osandov/linux/example/example.c:7:13: got char [noderef] <asn:4>*<noident>

Signature · 1 line
Omar