Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

[Neil Horman]

On Thu, Feb 11, 2010 at 09:27:08PM -0800, Andrew Morton wrote:
> On Fri, 12 Feb 2010 06:21:26 +0100 Andi Kleen <andi@xxxxxxxxxxxxxx> wrote:
> 
> > > urgh, must I?  That trashes Neil's
> > > kmod-add-init-function-to-usermodehelper.patch and
> > > kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> > > and probably requires repairing other stuff and sets the testing status
> > > back to "square one".
> > > 
> > > If you have patches queued, please make the time to support them!
> > 
> > Ok, understood. I'll try to look into it today.
> 
> Ta.  As I mentioned to Neil, if it looks serious then let's shelve it
> all and revisit for 2.6.35.
> 
> > You want incrementals?
> 
> If convenient, please.  Otherwise we can drop--and-remerge.
> 
> 


Ok, this fixes the oops Jiri reported for me.  Its been tested by me, but only
minimally, and my rcu-foo is not the greatest, so through reviews appreciated.
The patch is incremental against the latest mmotm as of this AM.

Thanks!
Neil


Fix up remaining references to uevent_helper to play nice with Andi's
uevent_helper/rcu changes.

Some changes were made recently which modified uevent_helper to be an rcu
protected pointer, rather than a static char array.  This has led to a few
missed points in which the sysfs path still assumed that:
1) the uevent_helper symbol could still be accessed safely without
rcu_dereference
2) that the sysfs path could copy data to that pointer safely.

I've fixed this by chaging the sysfs path so that it duplicates the string on
uevent_helper_store, and freeing it (only if it doesn't point to the
CONFIG_DEFAULT_UEVENT_HELPER string), in a call_rcu post-quiescent point.  I've
also fixed up the remaining references to the uevent_helper pointers to use
rcu_dereference.

Signed-off-by: Neil Horman <nhorman@xxxxxxxxxxxxx>


 kernel/ksysfs.c      |   38 ++++++++++++++++++++++++++++++++++++--
 lib/kobject_uevent.c |    4 +++-
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 21fe3c4..66d1e5b 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -37,19 +37,53 @@ 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", uevent_helper);
+	return sprintf(buf, "%s\n", rcu_dereference(uevent_helper));
 }
+
+struct uevent_helper_rcu {
+	char *oldptr;
+	struct rcu_head rcu;
+};
+
+static void free_old_uevent_ptr(struct rcu_head *list)
+{
+	struct uevent_helper_rcu *ptr;
+	char *dfl = CONFIG_UEVENT_HELPER_PATH;
+	ptr = container_of(list, struct uevent_helper_rcu, rcu);
+	if (ptr->oldptr && (ptr->oldptr != dfl))
+		kfree(ptr->oldptr);
+
+	kfree(ptr);
+}
+
 static ssize_t uevent_helper_store(struct kobject *kobj,
 				   struct kobj_attribute *attr,
 				   const char *buf, size_t count)
 {
+	char *kbuf;
+	struct uevent_helper_rcu *old;
+
 	if (count+1 > UEVENT_HELPER_PATH_LEN)
 		return -ENOENT;
-	memcpy(uevent_helper, buf, count);
+	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';
+	old = kmalloc(sizeof(struct uevent_helper_rcu), GFP_KERNEL);
+	if (!old)
+		goto out_free;
+
+	old->oldptr = rcu_dereference(uevent_helper);
+	rcu_assign_pointer(uevent_helper, kbuf);
+	call_rcu(&old->rcu, free_old_uevent_ptr);
+
 	return count;
+
+out_free:
+	kfree(kbuf);
+	return -ENOMEM;
 }
 KERNEL_ATTR_RW(uevent_helper);
 #endif
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index c2383f3..211f846 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -126,6 +126,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	struct kset *kset;
 	const struct kset_uevent_ops *uevent_ops;
 	u64 seq;
+	const char *helper;
 	int i = 0;
 	int retval = 0;
 
@@ -272,7 +273,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 #endif
 
 	/* call uevent_helper, usually only enabled during early boot */
-	if (uevent_helper[0])
+	helper = rcu_dereference(uevent_helper);
+	if (helper[0])
 		retval = uevent_call_helper(subsystem, env);
 
 exit:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/