Re: [PATCH] device_cgroup: lock assert fails in dev_exception_clean()

Re: [PATCH] device_cgroup: lock assert fails in dev_exception_clean()

[Jerry Snitselaar]

On Thu Jan 17 13, Tejun Heo wrote:
> Hello, Jerry.
> 
> Can you please also cc cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org and Aristeu Rozanski
> <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>?
> 
> 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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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

Re: [PATCH] device_cgroup: lock assert fails in dev_exception_clean()

[Jerry Snitselaar]

On Thu Jan 17 13, Jerry Snitselaar wrote:
> On Thu Jan 17 13, Tejun Heo wrote:
> > Hello, Jerry.
> > 
> > Can you please also cc cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org and Aristeu Rozanski
> > <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>?
> > 
> > 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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  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);
 }

Re: [PATCH] device_cgroup: lock assert fails in dev_exception_clean()

[Aristeu Rozanski]

Hi Jerry,
On Tue, Jan 22, 2013 at 09:31:41PM -0700, Jerry Snitselaar wrote:
> 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?

I think this is what was suggested (not tested):

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index d794abc..1c69e38 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -159,6 +159,16 @@ static void dev_exception_rm(struct dev_cgroup *dev_cgroup,
 	}
 }
 
+static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
+{
+	struct dev_exception_item *ex, *tmp;
+
+	list_for_each_entry_safe(ex, tmp, &dev_cgroup->exceptions, list) {
+		list_del_rcu(&ex->list);
+		kfree_rcu(ex, rcu);
+	}
+}
+
 /**
  * dev_exception_clean - frees all entries of the exception list
  * @dev_cgroup: dev_cgroup with the exception list to be cleaned
@@ -167,14 +177,9 @@ static void dev_exception_rm(struct dev_cgroup *dev_cgroup,
  */
 static void dev_exception_clean(struct dev_cgroup *dev_cgroup)
 {
-	struct dev_exception_item *ex, *tmp;
-
 	lockdep_assert_held(&devcgroup_mutex);
 
-	list_for_each_entry_safe(ex, tmp, &dev_cgroup->exceptions, list) {
-		list_del_rcu(&ex->list);
-		kfree_rcu(ex, rcu);
-	}
+	__dev_exception_clean(dev_cgroup);
 }
 
 /*
@@ -215,9 +220,7 @@ static void devcgroup_css_free(struct cgroup *cgroup)
 	struct dev_cgroup *dev_cgroup;
 
 	dev_cgroup = cgroup_to_devcgroup(cgroup);
-	mutex_lock(&devcgroup_mutex);
-	dev_exception_clean(dev_cgroup);
-	mutex_unlock(&devcgroup_mutex);
+	__dev_exception_clean(dev_cgroup);
 	kfree(dev_cgroup);
 }
 
-- 
Aristeu

Re: [PATCH] device_cgroup: lock assert fails in dev_exception_clean()

[Tejun Heo]

On Wed, Jan 23, 2013 at 09:29:39AM -0500, Aristeu Rozanski wrote:
> Hi Jerry,
> On Tue, Jan 22, 2013 at 09:31:41PM -0700, Jerry Snitselaar wrote:
> > 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?
> 
> I think this is what was suggested (not tested):

Yeap.

Thanks.

-- 
tejun

Re: [PATCH] device_cgroup: lock assert fails in dev_exception_clean()

[Aristeu Rozanski]

Hi Tejun,
On Wed, Jan 23, 2013 at 08:36:40AM -0800, Tejun Heo wrote:
> On Wed, Jan 23, 2013 at 09:29:39AM -0500, Aristeu Rozanski wrote:
> > Hi Jerry,
> > On Tue, Jan 22, 2013 at 09:31:41PM -0700, Jerry Snitselaar wrote:
> > > 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?
> > 
> > I think this is what was suggested (not tested):
> 
> Yeap.

on a slightly subject, hierarchy support is still desired for device
cgroups? I got the patchset ready by the time ebiederman submitted
userns patches and like it was discussed before device_cgroup would be
made obsolete once userns was out.

-- 
Aristeu

Re: [PATCH] device_cgroup: lock assert fails in dev_exception_clean()

[Tejun Heo]

Hello,

On Wed, Jan 23, 2013 at 8:38 AM, Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> on a slightly subject, hierarchy support is still desired for device
> cgroups? I got the patchset ready by the time ebiederman submitted
> userns patches and like it was discussed before device_cgroup would be
> made obsolete once userns was out.

I think having hierarchy support is still a good idea. It's not like
we can disable devcg immediately. Maybe we'll be able to drop it in
say five or ten years if no one is ever using it and breakages in it
go unnoticed for a long time and so on but that's still a long way
out.

Thanks.

-- 
tejun