Re: krealloc in kernel/params.c

From: Rasmus Villemoes
Date: Thu Oct 16 2014 - 05:49:37 EST


On Wed, Oct 15 2014, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:

> Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> 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 <rusty@xxxxxxxxxxxxxxx>
>
> 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
--
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/