| Message ID | 4BC42DE2.9020906@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Andy Whitcroft |
| Headers | show |
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
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
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); }