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

[John Johansen]

On 04/13/2010 02:25 AM, Andy Whitcroft wrote:
> 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.
> 
oh, I like that, will do