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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <[email protected]> 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 <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <[email protected]> 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 <[email protected]>
> 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
On Sun, Nov 30, 2014 at 10:26:43AM -0500, Pranith Kumar wrote:
> On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <[email protected]> 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 <[email protected]>
> > 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
On Sun, Nov 30, 2014 at 10:11:41AM -0500, Pranith Kumar wrote:
> On Sun, Nov 30, 2014 at 3:26 AM, Omar Sandoval <[email protected]> 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 <[email protected]>
>
> 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