Date:	Tue, 16 Feb 2010 10:52:43 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	akpm@...ux-foundation.org
Cc:	paulmck@...ux.vnet.ibm.com, andi@...stfloor.org,
	jirislaby@...il.com, linux-kernel@...r.kernel.org, gregkh@...e.de,
	kay.sievers@...e.de, nhorman@...driver.com
Subject: [PATCH] Fix up a few missed minor rcu errors in uevent_helper

Fix up a few remaining nits in uevent_helper

Fixes the remaining missed lock points in uevent_helper, as reported by Paul
McKenney in his review.  Specifically adds a spin lock to guard the write side
of usevent_helper from concurrent writes, and fixes two remaining missed read
side points that failed to use rcu_read_lock properly.  Also fixe up a minor
string parsing issue in uvent_helper_set (was terminating the string on the
wrong buffer).

I've done minimal testing on this (as the things this fixes were reported by
code audit, not any actual oops or other failure).  I did run this for a few
hours though, with several processes reading/writing different values to
/sys/kernel/uevent_helper, while issued a few module loads that required issuing
udev events (which, when overlapping with a proper write to /sys/kernel/uevent),
triggered a fork in the appropriate binary.  Nothing has fallen over in that
time, so I'm satisfied with this.  Applies as an incremental on top of the
latest -mm
Neil


Reported-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
Signed-off-by: Neil Horman <nhorman@...driver.com>
Tested-by: Neil Horman <nhorman@...driver.com>


 kernel/ksysfs.c      |   17 ++++++++++++-----
 lib/kobject_uevent.c |    3 ++-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 66d1e5b..401d1f0 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -16,6 +16,7 @@
 #include <linux/kexec.h>
 #include <linux/profile.h>
 #include <linux/sched.h>
+#include <linux/spinlock.h>
 
 #define KERNEL_ATTR_RO(_name) \
 static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
@@ -37,13 +38,18 @@ KERNEL_ATTR_RO(uevent_seqnum);
 static ssize_t uevent_helper_show(struct kobject *kobj,
 				  struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%s\n", rcu_dereference(uevent_helper));
+	ssize_t count;
+	rcu_read_lock();
+	count = sprintf(buf, "%s\n", rcu_dereference(uevent_helper));
+	rcu_read_unlock();
+	return count;
 }
 
 struct uevent_helper_rcu {
 	char *oldptr;
 	struct rcu_head rcu;
 };
+static DEFINE_SPINLOCK(uevent_helper_lock);
 
 static void free_old_uevent_ptr(struct rcu_head *list)
 {
@@ -68,15 +74,16 @@ static ssize_t uevent_helper_store(struct kobject *kobj,
 	kbuf = kstrndup(buf, UEVENT_HELPER_PATH_LEN, GFP_KERNEL);
 	if (!kbuf)
 		return -ENOMEM;
-	uevent_helper[count] = '\0';
-	if (count && uevent_helper[count-1] == '\n')
-		uevent_helper[count-1] = '\0';
+	kbuf[count] = '\0';
+	if (count && kbuf[count-1] == '\n')
+		kbuf[count-1] = '\0';
 	old = kmalloc(sizeof(struct uevent_helper_rcu), GFP_KERNEL);
 	if (!old)
 		goto out_free;
-
+	spin_lock(&uevent_helper_lock);
 	old->oldptr = rcu_dereference(uevent_helper);
 	rcu_assign_pointer(uevent_helper, kbuf);
+	spin_unlock(&uevent_helper_lock);
 	call_rcu(&old->rcu, free_old_uevent_ptr);
 
 	return count;
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 211f846..a7520d0 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -273,10 +273,11 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 #endif
 
 	/* call uevent_helper, usually only enabled during early boot */
+	rcu_read_lock();
 	helper = rcu_dereference(uevent_helper);
 	if (helper[0])
 		retval = uevent_call_helper(subsystem, env);
-
+	rcu_read_unlock();
 exit:
 	kfree(devpath);
 	kfree(env);
--
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