Re: krealloc in kernel/params.c

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Wed, Oct 15 2014, Rusty Russell <rusty@rustcorp.com.au> wrote:

> Rasmus Villemoes <linux@rasmusvillemoes.dk> 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@rustcorp.com.au>
>
> 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