[PATCH 11/11] AppArmor: use the kernel shared workqueue to free vmalloc'ed dfas

[Andy Whitcroft]

On Tue, Apr 13, 2010 at 01:40:02AM -0700, John Johansen wrote:
> On 04/13/2010 01:22 AM, Stefan Bader wrote:
> > Dunno, this looks a bit scary. Assuming the size of table is big enough to
> > contain a work struct, but is the struct really never touched again after
> > calling the worker function?
> >
> attached is a patch that adds the size condition
> 	tsize = tsize < sizeof(struct work_struct) ?
> 		sizeof(struct work_struct) : tsize;
> 
> to the allocation path
> 

> AppArmor: use the kernel shared workqueue to free vmalloc'ed dfas
> 
> AppArmor falls back to allocating dfas with vmalloc when memory becomes
> fragmented.  However dfa life cycle is often dependent on credentials
> which can be freed during interrupt context.  This can in cause the dfa
> to be freed during interrupt context, as well but vfree can not be
> used in interrupt context.
> 
> So for dfas that are allocated with vmalloc delay freeing them to
> a later time by placing them on the kernel shared workqueue.
> 
> Signed-off-by: John Johansen <john.johansen at canonical.com>
> 
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index afc2dd2..a3730e2 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -17,12 +17,26 @@
>  #include <linux/mm.h>
>  #include <linux/slab.h>
>  #include <linux/vmalloc.h>
> +#include <linux/workqueue.h>
>  #include <linux/err.h>
>  #include <linux/kref.h>
>  
>  #include "include/match.h"
>  
>  /**
> + * do_vfree - workqueue routine for freeing vmalloced memory
> + * @work: data to be freed
> + *
> + * The work_struct is overlayed to the data being freed, as at the point
> + * the work is scheduled the data is no longer valid, be its freeing
> + * needs to be delayed until safe.
> + */
> +static void do_vfree(struct work_struct *work)
> +{
> +	vfree(work);
> +}
> +
> +/**
>   * free_table - free a table allocated by unpack table
>   * @table: table to unpack  (MAYBE NULL)
>   */
> @@ -31,9 +45,14 @@ static void free_table(struct table_header *table)
>  	if (!table)
>  		return;
>  
> -	if (is_vmalloc_addr(table))
> -		vfree(table);
> -	else
> +	if (is_vmalloc_addr(table)) {
> +		/* Data is no longer valid so just use the allocated space
> +		 * as the work_struct
> +		 */
> +		struct work_struct *work = (struct work_struct *) table;
> +		INIT_WORK(work, do_vfree);
> +		schedule_work(work);	  
> +	} else
>  		kzfree(table);
>  }
>  
> @@ -75,6 +94,8 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  	/* freed by free_table */
>  	table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN);
>  	if (!table) {
> +		tsize = tsize < sizeof(struct work_struct) ?
> +			sizeof(struct work_struct) : tsize;
>  		unmap_alias = 1;
>  		table = vmalloc(tsize);
>  	}

Ahh, a small comment wouldn't hurt as to _why_ this is there, but that
makes sure.

For a cleanup you could probabally make this more optimal by using an
anonymous union to include a work_struct in the original structure.  But
this makes me happy.

Acked-by: Andy Whitcroft <apw at canonical.com>

-apw