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

From: Rusty Russell
Date: Sun Jun 14 2015 - 20:31:17 EST

Dan Streetman <ddstreet@xxxxxxxx> 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
--
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/