Messages in this thread | | | From | Rasmus Villemoes <> | Subject | Re: krealloc in kernel/params.c | Date | Thu, 16 Oct 2014 11:49:28 +0200 |
| |
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
| |