audit-related slab memory leak in recent -mm kernels?

[Dustin Kirkland]

On Fri, 2006-02-24 at 14:40 -0500, Amy Griffis wrote:
> On Fri, Feb 24, 2006 at 01:41:24PM -0500, Valdis.Kletnieks at vt.edu wrote:
> > For a while, I've been seeing a pretty serious leak in slab-32 entries in -mm
> > kernels. Doing a quilt bisection on -mm calls out git-audit.patch as the
> > offender.
> > 
> > In kernel/auditsc.c, we have audit_inode_context(), which does:
> > 
> >         ctx = kmalloc(len, GFP_KERNEL);
> > 	...
> >         context->names[idx].ctx = ctx;
> > 
> > but the only obvious kfree() I can find is in audit_free_names(), but that
> > one is (a) inside an if statement along with a printk(KERN_ERR) and (b) has
> > a '#if AUDIT_DEBUG == 2' around it.
> > 
> > [/usr/src/linux-2.6.16-rc4-mm2/kernel]1 grep -n '\.ctx' *.c
> > auditsc.c:384:                  kfree(context->names[i].ctx);
> > auditsc.c:686:          if (context->names[i].ctx) {
> > auditsc.c:688:                                  context->names[i].ctx);
> > auditsc.c:961:  context->names[idx].ctx = ctx;
> > 
> > Is this my memory leak?  If so, who is supposed to be freeing it?
> 
> That kfree is misplaced.  It should be here.  Does this solve it on
> your end?
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4626c35..1867fc3 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -381,7 +381,6 @@ static inline void audit_free_names(stru
>  			printk(KERN_ERR "names[%d] = %p = %s\n", i,
>  			       context->names[i].name,
>  			       context->names[i].name ?: "(null)");
> -			kfree(context->names[i].ctx);
>  		}
>  		dump_stack();
>  		return;
> @@ -392,9 +391,11 @@ static inline void audit_free_names(stru
>  	context->ino_count  = 0;
>  #endif
>  
> -	for (i = 0; i < context->name_count; i++)
> +	for (i = 0; i < context->name_count; i++) {
>  		if (context->names[i].name)
>  			__putname(context->names[i].name);
> +		kfree(context->names[i].ctx);
> +	}
>  	context->name_count = 0;
>  	if (context->pwd)
>  		dput(context->pwd);
> 


That's exactly the change I was about to suggest.  However, I'm in the
process of figuring out what the heck audit_free_names() does...  It
appears to mask its freeing functionality and I'm digging deeper into
it.


:-Dustin