It is likely that I'm just missing something trivial, but I have
a hard time understanding 63662139e ("params: Fix potential
memory leak in add_sysfs_param()"). [I take it for granted that
krealloc() does not free its first argument when it fails, as
reading mm/slab_common.c seems to confirm.] Some of the relevant
lines from kernel/params.c:
621 /* Enlarge. */
622 new = krealloc(mk->mp,
623 sizeof(*mk->mp) + sizeof(mk->mp->attrs[0]) * (num+1),
624 GFP_KERNEL);
625 if (!new) {
626 kfree(attrs);
627 err = -ENOMEM;
628 goto fail;
629 }
630 /* Despite looking like the typical realloc() bug, this is safe.
631 * We *want* the old 'attrs' to be freed either way, and we'll store
632 * the new one in the success case. */
633 attrs = krealloc(attrs, sizeof(new->grp.attrs[0])*(num+2), GFP_KERNEL);
634 if (!attrs) {
635 err = -ENOMEM;
636 goto fail_free_new;
637 }
[...]
663 fail_free_new:
664 kfree(new);
665 fail:
666 mk->mp = NULL;
667 return err;
First, if the krealloc call on line 622 fails, mk->mp seems to be
leaked (unless the caller happens to have a copy of that pointer
somewhere), since it is NULL'ed on the way out. 63662139e removed
a kfree(mk->mp) call. That doesn't seem right.
Second, the comment seems misleading, again because krealloc()
does not free its argument on failure. So if the krealloc() call
fails, the old attrs does seem to be leaked (attrs may be
mk->mp->grp.attrs, and again mk->mp is NULL'ed on the error
path (not that the caller could use mk->mp for anything - it is
invalidated by the successful first krealloc() call)).
Rasmus
Rasmus Villemoes <[email protected]> writes:
> It is likely that I'm just missing something trivial, but I have
> a hard time understanding 63662139e ("params: Fix potential
> memory leak in add_sysfs_param()").
Yes, it was a bad commit, and we've been discussing it, see:
[PATCH] params: fix potential memory leak in add_sysfs_param()
The only error case we are about is when add_sysfs_param()
is called from module_param_sysfs_setup(): the in-kernel cases
at boot time are assumed not to fail.
That call should invoke free_module_param_attrs() when it fails,
rather than relying on add_sysfs_param() to clean up.
Don't patch bad code - rewrite it. (Kernigan and Plauger)
How's this?
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 <[email protected]>
diff --git a/kernel/params.c b/kernel/params.c
index db97b791390f..5b8005d01dfc 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -603,68 +603,58 @@ 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
@@ -695,8 +685,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;
}
On Wed, Oct 15 2014, Rusty Russell <[email protected]> wrote:
> Rasmus Villemoes <[email protected]> writes:
>> It is likely that I'm just missing something trivial, but I have
>> a hard time understanding 63662139e ("params: Fix potential
>> memory leak in add_sysfs_param()").
>
> Yes, it was a bad commit, and we've been discussing it, see:
>
> [PATCH] params: fix potential memory leak in add_sysfs_param()
>
> The only error case we are about is when add_sysfs_param()
> is called from module_param_sysfs_setup(): the in-kernel cases
> at boot time are assumed not to fail.
>
> That call should invoke free_module_param_attrs() when it fails,
> rather than relying on add_sysfs_param() to clean up.
>
> Don't patch bad code - rewrite it. (Kernigan and Plauger)
>
> How's this?
>
> 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...
>
I think kzalloc immediately followed by kreallocing the returned value
is rather ugly. Other than that:
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/kernel/params.c b/kernel/params.c
> index db97b791390f..5b8005d01dfc 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -603,68 +603,58 @@ 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;
free_module_param_attrs does not check mk->mp for being NULL before
kfree'ing mk->mp->grp.attrs, so this will oops.
Rasmus
Rasmus Villemoes <[email protected]> writes:
> On Wed, Oct 15 2014, Rusty Russell <[email protected]> wrote:
>
>> Rasmus Villemoes <[email protected]> writes:
>> The kzalloc-then-always-krealloc pattern is perhaps overly simplistic,
>> but this code has clearly confused people. It worked on me...
>>
>
> I think kzalloc immediately followed by kreallocing the returned value
> is rather ugly. Other than that:
Indeed, but it's an obvious pattern. "If not initialized, initialize".
>> - num = mk->mp->num;
>> - attrs = mk->mp->grp.attrs;
>> + /* First allocation. */
>> + mk->mp = kzalloc(sizeof(*mk->mp), GFP_KERNEL);
>> + if (!mk->mp)
>> + return -ENOMEM;
>
> free_module_param_attrs does not check mk->mp for being NULL before
> kfree'ing mk->mp->grp.attrs, so this will oops.
Nice catch, folded this in:
diff --git a/kernel/params.c b/kernel/params.c
index 3ebe6c64aa67..ee92e67f2cee 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -650,7 +650,8 @@ static __modinit int add_sysfs_param(struct module_kobject *mk,
#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;
}
Thanks!
Rusty.