Re: [PATCH] module: add per-module param_lock

[Rusty Russell <rusty@rustcorp.com.au>]

Dan Streetman <ddstreet@ieee.org> writes:
> You suggested giving direct access to the mutex, but this patch just
> provides lock/unlock functions to the appropriate mutex, which seemed
> cleaner and clearer to me, but if you prefer the direct mutex access
> I can update the patch for that.

Happy with your judgement on this, and I think we've done enough
bike-shedding that existant code wins.

> Also, this combines the making the kparam_lock into a per-module mutex,
> with also removing the kparam_block_sysfs_*() macros; I can split that up
> if you would prefer two patches.

Hmm, I've split the "make u16 perm const" into its own patch instead,
since that's a nice independent change.

> +/* Use the module's mutex, or if built-in use the built-in mutex */
> +#define KPARAM_MUTEX(mod)	((mod) ? &(mod)->param_lock : &param_lock)
> +#define KPARAM_IS_LOCKED(mod)	mutex_is_locked(KPARAM_MUTEX(mod))

In future I prefer inlines to macros.  In fact, since this is just in
params.c it's not too bad to just open-code them in the two places
they're each used.

But that's nit-picking.

Applied, thanks!
Rusty.


> +
>  /* This just allows us to keep track of which parameters are kmalloced. */
>  struct kmalloced_param {
>  	struct list_head list;
>  	char val[];
>  };
>  static LIST_HEAD(kmalloced_params);
> +static DEFINE_SPINLOCK(kmalloced_params_lock);
>  
>  static void *kmalloc_parameter(unsigned int size)
>  {
> @@ -43,7 +48,10 @@ static void *kmalloc_parameter(unsigned int size)
>  	if (!p)
>  		return NULL;
>  
> +	spin_lock(&kmalloced_params_lock);
>  	list_add(&p->list, &kmalloced_params);
> +	spin_unlock(&kmalloced_params_lock);
> +
>  	return p->val;
>  }
>  
> @@ -52,6 +60,7 @@ static void maybe_kfree_parameter(void *param)
>  {
>  	struct kmalloced_param *p;
>  
> +	spin_lock(&kmalloced_params_lock);
>  	list_for_each_entry(p, &kmalloced_params, list) {
>  		if (p->val == param) {
>  			list_del(&p->list);
> @@ -59,6 +68,7 @@ static void maybe_kfree_parameter(void *param)
>  			break;
>  		}
>  	}
> +	spin_unlock(&kmalloced_params_lock);
>  }
>  
>  static char dash2underscore(char c)
> @@ -118,10 +128,10 @@ static int parse_one(char *param,
>  				return -EINVAL;
>  			pr_debug("handling %s with %p\n", param,
>  				params[i].ops->set);
> -			mutex_lock(&param_lock);
> +			kernel_param_lock(params[i].mod);
>  			param_check_unsafe(&params[i]);
>  			err = params[i].ops->set(val, &params[i]);
> -			mutex_unlock(&param_lock);
> +			kernel_param_unlock(params[i].mod);
>  			return err;
>  		}
>  	}
> @@ -364,12 +374,11 @@ EXPORT_SYMBOL(param_ops_invbool);
>  
>  int param_set_bint(const char *val, const struct kernel_param *kp)
>  {
> -	struct kernel_param boolkp;
> +	/* Match bool exactly, by re-using it. */
> +	struct kernel_param boolkp = *kp;
>  	bool v;
>  	int ret;
>  
> -	/* Match bool exactly, by re-using it. */
> -	boolkp = *kp;
>  	boolkp.arg = &v;
>  
>  	ret = param_set_bool(val, &boolkp);
> @@ -387,7 +396,8 @@ struct kernel_param_ops param_ops_bint = {
>  EXPORT_SYMBOL(param_ops_bint);
>  
>  /* We break the rule and mangle the string. */
> -static int param_array(const char *name,
> +static int param_array(struct module *mod,
> +		       const char *name,
>  		       const char *val,
>  		       unsigned int min, unsigned int max,
>  		       void *elem, int elemsize,
> @@ -418,7 +428,7 @@ static int param_array(const char *name,
>  		/* nul-terminate and parse */
>  		save = val[len];
>  		((char *)val)[len] = '\0';
> -		BUG_ON(!mutex_is_locked(&param_lock));
> +		BUG_ON(!KPARAM_IS_LOCKED(mod));
>  		ret = set(val, &kp);
>  
>  		if (ret != 0)
> @@ -440,7 +450,7 @@ static int param_array_set(const char *val, const struct kernel_param *kp)
>  	const struct kparam_array *arr = kp->arr;
>  	unsigned int temp_num;
>  
> -	return param_array(kp->name, val, 1, arr->max, arr->elem,
> +	return param_array(kp->mod, kp->name, val, 1, arr->max, arr->elem,
>  			   arr->elemsize, arr->ops->set, kp->level,
>  			   arr->num ?: &temp_num);
>  }
> @@ -449,14 +459,13 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
>  {
>  	int i, off, ret;
>  	const struct kparam_array *arr = kp->arr;
> -	struct kernel_param p;
> +	struct kernel_param p = *kp;
>  
> -	p = *kp;
>  	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
>  		if (i)
>  			buffer[off++] = ',';
>  		p.arg = arr->elem + arr->elemsize * i;
> -		BUG_ON(!mutex_is_locked(&param_lock));
> +		BUG_ON(!KPARAM_IS_LOCKED(p.mod));
>  		ret = arr->ops->get(buffer + off, &p);
>  		if (ret < 0)
>  			return ret;
> @@ -539,9 +548,9 @@ static ssize_t param_attr_show(struct module_attribute *mattr,
>  	if (!attribute->param->ops->get)
>  		return -EPERM;
>  
> -	mutex_lock(&param_lock);
> +	kernel_param_lock(mk->mod);
>  	count = attribute->param->ops->get(buf, attribute->param);
> -	mutex_unlock(&param_lock);
> +	kernel_param_unlock(mk->mod);
>  	if (count > 0) {
>  		strcat(buf, "\n");
>  		++count;
> @@ -551,7 +560,7 @@ static ssize_t param_attr_show(struct module_attribute *mattr,
>  
>  /* sysfs always hands a nul-terminated string in buf.  We rely on that. */
>  static ssize_t param_attr_store(struct module_attribute *mattr,
> -				struct module_kobject *km,
> +				struct module_kobject *mk,
>  				const char *buf, size_t len)
>  {
>   	int err;
> @@ -560,10 +569,10 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
>  	if (!attribute->param->ops->set)
>  		return -EPERM;
>  
> -	mutex_lock(&param_lock);
> +	kernel_param_lock(mk->mod);
>  	param_check_unsafe(attribute->param);
>  	err = attribute->param->ops->set(buf, attribute->param);
> -	mutex_unlock(&param_lock);
> +	kernel_param_unlock(mk->mod);
>  	if (!err)
>  		return len;
>  	return err;
> @@ -576,18 +585,19 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
>  #define __modinit __init
>  #endif
>  
> -#ifdef CONFIG_SYSFS
> -void __kernel_param_lock(void)
> +void kernel_param_lock(struct module *mod)
>  {
> -	mutex_lock(&param_lock);
> +	mutex_lock(KPARAM_MUTEX(mod));
>  }
> -EXPORT_SYMBOL(__kernel_param_lock);
>  
> -void __kernel_param_unlock(void)
> +void kernel_param_unlock(struct module *mod)
>  {
> -	mutex_unlock(&param_lock);
> +	mutex_unlock(KPARAM_MUTEX(mod));
>  }
> -EXPORT_SYMBOL(__kernel_param_unlock);
> +
> +#ifdef CONFIG_SYSFS
> +EXPORT_SYMBOL(kernel_param_lock);
> +EXPORT_SYMBOL(kernel_param_unlock);
>  
>  /*
>   * add_sysfs_param - add a parameter to sysfs
> diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
> index d53355b..8544e2e 100644
> --- a/net/mac80211/rate.c
> +++ b/net/mac80211/rate.c
> @@ -103,7 +103,7 @@ ieee80211_rate_control_ops_get(const char *name)
>  	const struct rate_control_ops *ops;
>  	const char *alg_name;
>  
> -	kparam_block_sysfs_write(ieee80211_default_rc_algo);
> +	kernel_param_lock(THIS_MODULE);
>  	if (!name)
>  		alg_name = ieee80211_default_rc_algo;
>  	else
> @@ -117,7 +117,7 @@ ieee80211_rate_control_ops_get(const char *name)
>  	/* try built-in one if specific alg requested but not found */
>  	if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT))
>  		ops = ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT);
> -	kparam_unblock_sysfs_write(ieee80211_default_rc_algo);
> +	kernel_param_unlock(THIS_MODULE);
>  
>  	return ops;
>  }
> -- 
> 2.1.0