| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <8881abdb8836e2d70f705baacc0fe491e75d66d4.1417335583.git.osandov@osandov.com>
Date: Sun, 30 Nov 2014 00:26:49 -0800
From: Omar Sandoval <osandov@...ndov.com>
To: Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
Joe Perches <joe@...ches.com>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Josh Triplett <josh@...htriplett.org>,
Steven Rostedt <rostedt@...dmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
linux-kernel@...r.kernel.org, linux-btrfs@...r.kernel.org
Cc: Omar Sandoval <osandov@...ndov.com>
Subject: [PATCH 3/3] btrfs: refactor btrfs_device->name updates
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@...ndov.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists