params: cleanup sysfs allocation

commit 63662139e519ce06090b2759cf4a1d291b9cc0e2 attempted to patch a
leak (which would only happen on OOM, ie. never), but it didn't quite
work.

This rewrites the code to be as simple as possible.  add_sysfs_param()
adds a parameter.  If it fails, it's the caller's responsibility to
clean up the parameters which already exist.

The kzalloc-then-always-krealloc pattern is perhaps overly simplistic,
but this code has clearly confused people.  It worked on me...

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/kernel/params.c b/kernel/params.c
index db97b791390fa0ef3f4aa10d1bd5e53fe6e921b8..795321aba29f39f2b89b280965a73c4a7ad9a707 100644 (file)
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -603,74 +603,65 @@ static __modinit int add_sysfs_param(struct module_kobject *mk,
                                     const struct kernel_param *kp,
                                     const char *name)
 {
-       struct module_param_attrs *new;
-       struct attribute **attrs;
-       int err, num;
+       struct module_param_attrs *new_mp;
+       struct attribute **new_attrs;
+       unsigned int i;
 
        /* We don't bother calling this with invisible parameters. */
        BUG_ON(!kp->perm);
 
        if (!mk->mp) {
-               num = 0;
-               attrs = NULL;
-       } else {
-               num = mk->mp->num;
-               attrs = mk->mp->grp.attrs;
+               /* First allocation. */
+               mk->mp = kzalloc(sizeof(*mk->mp), GFP_KERNEL);
+               if (!mk->mp)
+                       return -ENOMEM;
+               mk->mp->grp.name = "parameters";
+               /* NULL-terminated attribute array. */
+               mk->mp->grp.attrs = kzalloc(sizeof(mk->mp->grp.attrs[0]),
+                                           GFP_KERNEL);
+               /* Caller will cleanup via free_module_param_attrs */
+               if (!mk->mp->grp.attrs)
+                       return -ENOMEM;
        }
 
-       /* Enlarge. */
-       new = krealloc(mk->mp,
-                      sizeof(*mk->mp) + sizeof(mk->mp->attrs[0]) * (num+1),
-                      GFP_KERNEL);
-       if (!new) {
-               kfree(attrs);
-               err = -ENOMEM;
-               goto fail;
-       }
-       /* Despite looking like the typical realloc() bug, this is safe.
-        * We *want* the old 'attrs' to be freed either way, and we'll store
-        * the new one in the success case. */
-       attrs = krealloc(attrs, sizeof(new->grp.attrs[0])*(num+2), GFP_KERNEL);
-       if (!attrs) {
-               err = -ENOMEM;
-               goto fail_free_new;
-       }
+       /* Enlarge allocations. */
+       new_mp = krealloc(mk->mp,
+                         sizeof(*mk->mp) +
+                         sizeof(mk->mp->attrs[0]) * (mk->mp->num + 1),
+                         GFP_KERNEL);
+       if (!new_mp)
+               return -ENOMEM;
+       mk->mp = new_mp;
 
-       /* Sysfs wants everything zeroed. */
-       memset(new, 0, sizeof(*new));
-       memset(&new->attrs[num], 0, sizeof(new->attrs[num]));
-       memset(&attrs[num], 0, sizeof(attrs[num]));
-       new->grp.name = "parameters";
-       new->grp.attrs = attrs;
+       /* Extra pointer for NULL terminator */
+       new_attrs = krealloc(mk->mp->grp.attrs,
+                            sizeof(mk->mp->grp.attrs[0]) * (mk->mp->num + 2),
+                            GFP_KERNEL);
+       if (!new_attrs)
+               return -ENOMEM;
+       mk->mp->grp.attrs = new_attrs;
 
        /* Tack new one on the end. */
-       sysfs_attr_init(&new->attrs[num].mattr.attr);
-       new->attrs[num].param = kp;
-       new->attrs[num].mattr.show = param_attr_show;
-       new->attrs[num].mattr.store = param_attr_store;
-       new->attrs[num].mattr.attr.name = (char *)name;
-       new->attrs[num].mattr.attr.mode = kp->perm;
-       new->num = num+1;
+       sysfs_attr_init(&mk->mp->attrs[mk->mp->num].mattr.attr);
+       mk->mp->attrs[mk->mp->num].param = kp;
+       mk->mp->attrs[mk->mp->num].mattr.show = param_attr_show;
+       mk->mp->attrs[mk->mp->num].mattr.store = param_attr_store;
+       mk->mp->attrs[mk->mp->num].mattr.attr.name = (char *)name;
+       mk->mp->attrs[mk->mp->num].mattr.attr.mode = kp->perm;
+       mk->mp->num++;
 
        /* Fix up all the pointers, since krealloc can move us */
-       for (num = 0; num < new->num; num++)
-               new->grp.attrs[num] = &new->attrs[num].mattr.attr;
-       new->grp.attrs[num] = NULL;
-
-       mk->mp = new;
+       for (i = 0; i < mk->mp->num; i++)
+               mk->mp->grp.attrs[i] = &mk->mp->attrs[i].mattr.attr;
+       mk->mp->grp.attrs[mk->mp->num] = NULL;
        return 0;
-
-fail_free_new:
-       kfree(new);
-fail:
-       mk->mp = NULL;
-       return err;
 }
 
 #ifdef CONFIG_MODULES
 static void free_module_param_attrs(struct module_kobject *mk)
 {
-       kfree(mk->mp->grp.attrs);
+       if (mk->mp)
+               kfree(mk->mp->grp.attrs);
        kfree(mk->mp);
        mk->mp = NULL;
 }
@@ -695,8 +686,10 @@ int module_param_sysfs_setup(struct module *mod,
                if (kparam[i].perm == 0)
                        continue;
                err = add_sysfs_param(&mod->mkobj, &kparam[i], kparam[i].name);
-               if (err)
+               if (err) {
+                       free_module_param_attrs(&mod->mkobj);
                        return err;
+               }
                params = true;
        }