Re: [PATCH] Fix up a few missed minor rcu errors in uevent_helper

[Paul E. McKenney]

On Tue, Feb 16, 2010 at 10:52:43AM -0500, Neil Horman wrote:
> 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

>From and RCU perspective:

Acked-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>

> Reported-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Neil Horman <nhorman@xxxxxxxxxxxxx>
> Tested-by: Neil Horman <nhorman@xxxxxxxxxxxxx>
> 
> 
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/