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

login
register
mail settings
Submitter John Johansen
Date April 13, 2010, 8:40 a.m.
Message ID <4BC42DE2.9020906@canonical.com>
Download mbox | patch
Permalink /patch/50050/
State Accepted
Delegated to: Andy Whitcroft
Headers show

Comments

John Johansen - April 13, 2010, 8:40 a.m.
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
Andy Whitcroft - April 13, 2010, 9:25 a.m.
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@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@canonical.com>

-apw
John Johansen - April 13, 2010, 9:26 a.m.
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@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

Patch

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@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);
 	}