btrfs: fix RCU string sparse noise
Subsystems (2)
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 structrcu_string[noderef] <asn:4>*rcu_str
fs/btrfs/check-integrity.c:848:25: got structrcu_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
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
@@ -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
PATCH 2/3 btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO
In reply to Omar SandovalA 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
@@ -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
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.
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?
Quoted 46 lines
--- fs/btrfs/ioctl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)fs/btrfs/ioctl.c +5
-6@@ -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/
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
Signed-off-by Omar Sandoval <[email protected]>You can use
rcu_access_pointer()in theif()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 ofrcu_dereference()ondevice->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
PATCH 3/3 btrfs: refactor btrfs_device->name updates
In reply to Omar SandovalThe 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
@@ -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
@@ -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
Re: [PATCH 3/3] btrfs: refactor btrfs_device->name updates
Quoted 12 lines · Omar Sandoval
The
rcu_stringAPI introduced some new sparse errors but also revealed existing ones. First of all, the name in structbtrfs_deviceshould be annotated as__rcuto prevent unsafe reads. Additionally, updates should go throughrcu_dereference_protectedto 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.h +0
-0
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
Re: [PATCH 3/3] btrfs: refactor btrfs_device->name updates
Quoted 19 lines · Pranith Kumar
Quoted 18 lines · Omar Sandoval
The
rcu_stringAPI introduced some new sparse errors but also revealed existing ones. First of all, the name in structbtrfs_deviceshould be annotated as__rcuto prevent unsafe reads. Additionally, updates should go throughrcu_dereference_protectedto 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.h +1
-1@@ -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
-0u64 generation;Since
rcu_stringsare rcu specific, why not annotate the char pointer in 'structrcu_string' with__rcuannotation? That should catch all error-prone users ofrcu_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