[PATCH 0/3] btrfs: fix RCU string sparse noise

[PATCH 0/3] btrfs: fix RCU string sparse noise

[Omar Sandoval]

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:

On Thu, Nov 27, 2014 at 06:45:20AM +0800, kbuild test robot wrote:
> 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__
>
>
> sparse warnings: (new ones prefixed by >>)
>
> >> 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(-)

-- 
2.1.3

[PATCH 1/3] rcustring: clean up botched __rcu annotations

[Omar Sandoval]

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.

Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 include/linux/rcustring.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/rcustring.h b/include/linux/rcustring.h
index 67277ab..28bd9bc 100644
--- a/include/linux/rcustring.h
+++ b/include/linux/rcustring.h
@@ -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);
-- 
2.1.3

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

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

Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 fs/btrfs/ioctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ecdf68f..dd55844 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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

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

[Pranith Kumar]

On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <osandov@osandov.com> wrote:
> 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.
>
> Signed-off-by: Omar Sandoval <osandov@osandov.com>

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?

> ---
>  fs/btrfs/ioctl.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index ecdf68f..dd55844 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Omar Sandoval]

On Sun, Nov 30, 2014 at 10:11:41AM -0500, Pranith Kumar wrote:
> On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <osandov@osandov.com> wrote:
> > 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.
> >
> > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> 
> 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.

-- 
Omar

[PATCH 3/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@osandov.com>
---
 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 d13b253..6913bed 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 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:
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6e04f27..2298a70 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.1.3

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

[Pranith Kumar]

On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <osandov@osandov.com> wrote:
> 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@osandov.com>
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 6e04f27..2298a70 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;
>

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.

-- 
Pranith

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

[Omar Sandoval]

On Sun, Nov 30, 2014 at 10:26:43AM -0500, Pranith Kumar wrote:
> On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <osandov@osandov.com> wrote:
> > 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@osandov.com>
> > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> > index 6e04f27..2298a70 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;
> >
> 
> 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>

-- 
Omar