| Message ID | 1271142580-26555-12-git-send-email-john.johansen@canonical.com |
|---|---|
| State | Superseded |
| Delegated to: | Andy Whitcroft |
| Headers | show |
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? john.johansen@canonical.com wrote: > From: John Johansen <john.johansen@canonical.com> > > OriginalAuthor: John Johansen <john.johansen@canonical.com> > OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor.git AA2.5-2.6.33 > commit: 2b0c4ab1a947adfef6a09619454b7f8eac46df96 > BugLink: http://bugs.launchpad.net/bugs/562044 > > 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> > --- > security/apparmor/match.c | 25 ++++++++++++++++++++++--- > 1 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/security/apparmor/match.c b/security/apparmor/match.c > index afc2dd2..d2cd554 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); > } >
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, well yes the size is big enough, but we could bound the table size on alloc so it is documented in the code. > but is the struct really never touched again after > calling the worker function? > If it is its a bug. The only way this code gets reached is if all the refcounts on the profile and the dfa have been put. > john.johansen@canonical.com wrote: >> From: John Johansen <john.johansen@canonical.com> >> >> OriginalAuthor: John Johansen <john.johansen@canonical.com> >> OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor.git AA2.5-2.6.33 >> commit: 2b0c4ab1a947adfef6a09619454b7f8eac46df96 >> BugLink: http://bugs.launchpad.net/bugs/562044 >> >> 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> >> --- >> security/apparmor/match.c | 25 ++++++++++++++++++++++--- >> 1 files changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/security/apparmor/match.c b/security/apparmor/match.c >> index afc2dd2..d2cd554 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); >> } >> >
On Tue, Apr 13, 2010 at 12:09:40AM -0700, john.johansen@canonical.com wrote: > From: John Johansen <john.johansen@canonical.com> > > OriginalAuthor: John Johansen <john.johansen@canonical.com> > OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor.git AA2.5-2.6.33 > commit: 2b0c4ab1a947adfef6a09619454b7f8eac46df96 > BugLink: http://bugs.launchpad.net/bugs/562044 > > 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> > --- > security/apparmor/match.c | 25 ++++++++++++++++++++++--- > 1 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/security/apparmor/match.c b/security/apparmor/match.c > index afc2dd2..d2cd554 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); > } This does assume that a dfa cannnot ever be smaller than a struct work_struct ... is that guarenteed? -apw
diff --git a/security/apparmor/match.c b/security/apparmor/match.c index afc2dd2..d2cd554 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); }