On Thu Jan 17 13, Jerry Snitselaar wrote:
> On Thu Jan 17 13, Tejun Heo wrote:
> > Hello, Jerry.
> >
> > Can you please also cc cgroups@xxxxxxxxxxxxxxx and Aristeu Rozanski
> > <aris@xxxxxxxxxx>?
> >
> > On Thu, Jan 17, 2013 at 03:00:18PM -0700, Jerry Snitselaar wrote:
> > > devcgroup_css_free() calls dev_exception_clean() without the
> > > devcgroup_mutex being held.
> > >
> > > Shutting down a kvm virt was giving me the following trace:
> >
> > Hmm...
> >
> > > Signed-off-by: Jerry Snitselaar <jerry.snitselaar@xxxxxxxxxx>
> > > ---
> > > security/device_cgroup.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> > > index 19ecc8d..d794abc 100644
> > > --- a/security/device_cgroup.c
> > > +++ b/security/device_cgroup.c
> > > @@ -215,7 +215,9 @@ static void devcgroup_css_free(struct cgroup *cgroup)
> > > struct dev_cgroup *dev_cgroup;
> > >
> > > dev_cgroup = cgroup_to_devcgroup(cgroup);
> > > + mutex_lock(&devcgroup_mutex);
> >
> > You can't grab mutex from rcu callback. It seems like the lockdep
> > assertion isn't necessary in this case as the free callback should be
> > the last remaining user of the dev_cgroup. Maybe factor out
> > __devcgroup_css_free() which doesn't have lockdep assertion?
> >
>
> Ok. I was wondering if it was really needed since it was freeing
> dev_cgroup right after that. I probably would have never noticed the
> trace if hadn't been checking to see if a fix for ttm stopped it from
> spamming me with them for trying to sleep in an invalid context.
>
> Jerry
>
> > > dev_exception_clean(dev_cgroup);
> > > + mutex_unlock(&devcgroup_mutex);
> > > kfree(dev_cgroup);
> >
> > Thanks.
> >
> > --
> > tejun
Hi Tejun and Aritsteu,
The original patch I sent to James and lkml got pulled in to linus. Is
the correct thing to do here put the list cleanup code back in
devcgroup_css_free and drop the call to dev_exception_clean along with
change my patch made like this?
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index d794abc..68237e9 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -213,11 +213,13 @@ static struct cgroup_subsys_state *devcgroup_css_alloc(struct cgroup *cgroup)
static void devcgroup_css_free(struct cgroup *cgroup)
{
struct dev_cgroup *dev_cgroup;
+ struct dev_exception_item *ex, *tmp;
dev_cgroup = cgroup_to_devcgroup(cgroup);
- mutex_lock(&devcgroup_mutex);
- dev_exception_clean(dev_cgroup);
- mutex_unlock(&devcgroup_mutex);
+ list_for_each_entry_safe(ex, tmp, &dev_cgroup->exceptions, list) {
+ list_del_rcu(&ex->list);
+ kfree_rcu(ex, rcu);
+ }
kfree(dev_cgroup);
}
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
