[PATCH] devcgroup: avoid using cgroup_lock

[PATCH] devcgroup: avoid using cgroup_lock

[Li Zefan]

There is nothing special that has to be protected by cgroup_lock,
so introduce devcgroup_mtuex for it's own use.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 security/device_cgroup.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 3aacd0f..5fda7df 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -11,6 +11,7 @@
 #include <linux/uaccess.h>
 #include <linux/seq_file.h>
 #include <linux/rcupdate.h>
+#include <linux/mutex.h>
 
 #define ACC_MKNOD 1
 #define ACC_READ  2
@@ -21,9 +22,11 @@
 #define DEV_CHAR  2
 #define DEV_ALL   4  /* this represents all devices */
 
+static DEFINE_MUTEX(devcgroup_mutex);
+
 /*
  * whitelist locking rules:
- * hold cgroup_lock() for update/read.
+ * hold devcgroup_mutex for update/read.
  * hold rcu_read_lock() for read.
  */
 
@@ -67,7 +70,7 @@ static int devcgroup_can_attach(struct cgroup_subsys *ss,
 }
 
 /*
- * called under cgroup_lock()
+ * called under devcgroup_mutex
  */
 static int dev_whitelist_copy(struct list_head *dest, struct list_head *orig)
 {
@@ -92,7 +95,7 @@ free_and_exit:
 
 /* Stupid prototype - don't bother combining existing entries */
 /*
- * called under cgroup_lock()
+ * called under devcgroup_mutex
  */
 static int dev_whitelist_add(struct dev_cgroup *dev_cgroup,
 			struct dev_whitelist_item *wh)
@@ -130,7 +133,7 @@ static void whitelist_item_free(struct rcu_head *rcu)
 }
 
 /*
- * called under cgroup_lock()
+ * called under devcgroup_mutex
  */
 static void dev_whitelist_rm(struct dev_cgroup *dev_cgroup,
 			struct dev_whitelist_item *wh)
@@ -185,8 +188,10 @@ static struct cgroup_subsys_state *devcgroup_create(struct cgroup_subsys *ss,
 		list_add(&wh->list, &dev_cgroup->whitelist);
 	} else {
 		parent_dev_cgroup = cgroup_to_devcgroup(parent_cgroup);
+		mutex_lock(&devcgroup_mutex);
 		ret = dev_whitelist_copy(&dev_cgroup->whitelist,
 				&parent_dev_cgroup->whitelist);
+		mutex_unlock(&devcgroup_mutex);
 		if (ret) {
 			kfree(dev_cgroup);
 			return ERR_PTR(ret);
@@ -273,7 +278,7 @@ static int devcgroup_seq_read(struct cgroup *cgroup, struct cftype *cft,
  * does the access granted to dev_cgroup c contain the access
  * requested in whitelist item refwh.
  * return 1 if yes, 0 if no.
- * call with c->lock held
+ * call with devcgroup_mutex held
  */
 static int may_access_whitelist(struct dev_cgroup *c,
 				       struct dev_whitelist_item *refwh)
@@ -426,11 +431,11 @@ static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
 				  const char *buffer)
 {
 	int retval;
-	if (!cgroup_lock_live_group(cgrp))
-		return -ENODEV;
+
+	mutex_lock(&devcgroup_mutex);
 	retval = devcgroup_update_access(cgroup_to_devcgroup(cgrp),
 					 cft->private, buffer);
-	cgroup_unlock();
+	mutex_unlock(&devcgroup_mutex);
 	return retval;
 }
 
-- 
1.5.4.rc3

Re: [PATCH] devcgroup: avoid using cgroup_lock

[Serge E. Hallyn]

Quoting Li Zefan (lizf@cn.fujitsu.com):
> There is nothing special that has to be protected by cgroup_lock,
> so introduce devcgroup_mtuex for it's own use.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  security/device_cgroup.c |   21 +++++++++++++--------
>  1 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index 3aacd0f..5fda7df 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -11,6 +11,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/seq_file.h>
>  #include <linux/rcupdate.h>
> +#include <linux/mutex.h>
> 
>  #define ACC_MKNOD 1
>  #define ACC_READ  2
> @@ -21,9 +22,11 @@
>  #define DEV_CHAR  2
>  #define DEV_ALL   4  /* this represents all devices */
> 
> +static DEFINE_MUTEX(devcgroup_mutex);
> +
>  /*
>   * whitelist locking rules:
> - * hold cgroup_lock() for update/read.
> + * hold devcgroup_mutex for update/read.
>   * hold rcu_read_lock() for read.
>   */
> 
> @@ -67,7 +70,7 @@ static int devcgroup_can_attach(struct cgroup_subsys *ss,
>  }
> 
>  /*
> - * called under cgroup_lock()
> + * called under devcgroup_mutex
>   */
>  static int dev_whitelist_copy(struct list_head *dest, struct list_head *orig)
>  {
> @@ -92,7 +95,7 @@ free_and_exit:
> 
>  /* Stupid prototype - don't bother combining existing entries */
>  /*
> - * called under cgroup_lock()
> + * called under devcgroup_mutex
>   */
>  static int dev_whitelist_add(struct dev_cgroup *dev_cgroup,
>  			struct dev_whitelist_item *wh)
> @@ -130,7 +133,7 @@ static void whitelist_item_free(struct rcu_head *rcu)
>  }
> 
>  /*
> - * called under cgroup_lock()
> + * called under devcgroup_mutex
>   */
>  static void dev_whitelist_rm(struct dev_cgroup *dev_cgroup,
>  			struct dev_whitelist_item *wh)
> @@ -185,8 +188,10 @@ static struct cgroup_subsys_state *devcgroup_create(struct cgroup_subsys *ss,
>  		list_add(&wh->list, &dev_cgroup->whitelist);
>  	} else {
>  		parent_dev_cgroup = cgroup_to_devcgroup(parent_cgroup);
> +		mutex_lock(&devcgroup_mutex);
>  		ret = dev_whitelist_copy(&dev_cgroup->whitelist,
>  				&parent_dev_cgroup->whitelist);
> +		mutex_unlock(&devcgroup_mutex);
>  		if (ret) {
>  			kfree(dev_cgroup);
>  			return ERR_PTR(ret);
> @@ -273,7 +278,7 @@ static int devcgroup_seq_read(struct cgroup *cgroup, struct cftype *cft,
>   * does the access granted to dev_cgroup c contain the access
>   * requested in whitelist item refwh.
>   * return 1 if yes, 0 if no.
> - * call with c->lock held
> + * call with devcgroup_mutex held
>   */
>  static int may_access_whitelist(struct dev_cgroup *c,
>  				       struct dev_whitelist_item *refwh)
> @@ -426,11 +431,11 @@ static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
>  				  const char *buffer)
>  {
>  	int retval;
> -	if (!cgroup_lock_live_group(cgrp))

Does it matter that we no longer check for cgroup_is_removed()?

> -		return -ENODEV;
> +
> +	mutex_lock(&devcgroup_mutex);
>  	retval = devcgroup_update_access(cgroup_to_devcgroup(cgrp),
>  					 cft->private, buffer);
> -	cgroup_unlock();
> +	mutex_unlock(&devcgroup_mutex);
>  	return retval;
>  }
> 
> -- 
> 1.5.4.rc3

Re: [PATCH] devcgroup: avoid using cgroup_lock

[Li Zefan]

>> @@ -426,11 +431,11 @@ static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
>>  				  const char *buffer)
>>  {
>>  	int retval;
>> -	if (!cgroup_lock_live_group(cgrp))
> 
> Does it matter that we no longer check for cgroup_is_removed()?
> 

No, this means in a rare case that the write handler is called when the cgroup
is dead, we still do the update work instead of returning ENODEV.

This is ok, since at that time, accessing cgroup and devcgroup is still valid,
but will have no effect since there is no task in this cgroup and the cgroup
will be destroyed soon.

>> -		return -ENODEV;
>> +
>> +	mutex_lock(&devcgroup_mutex);
>>  	retval = devcgroup_update_access(cgroup_to_devcgroup(cgrp),
>>  					 cft->private, buffer);
>> -	cgroup_unlock();
>> +	mutex_unlock(&devcgroup_mutex);
>>  	return retval;
>>  }

Re: [PATCH] devcgroup: avoid using cgroup_lock

[Serge E. Hallyn]

Quoting Li Zefan (lizf@cn.fujitsu.com):
> >> @@ -426,11 +431,11 @@ static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
> >>  				  const char *buffer)
> >>  {
> >>  	int retval;
> >> -	if (!cgroup_lock_live_group(cgrp))
> > 
> > Does it matter that we no longer check for cgroup_is_removed()?
> > 
> 
> No, this means in a rare case that the write handler is called when the cgroup
> is dead, we still do the update work instead of returning ENODEV.
> 
> This is ok, since at that time, accessing cgroup and devcgroup is still valid,
> but will have no effect since there is no task in this cgroup and the cgroup
> will be destroyed soon.

Ok, just wanted to make sure the devcgroup couldn't be partially torn
down and risking NULL or freed-memory derefs...

BTW is that against linux-next?  (didn't seem to apply cleanly against
my 2.6.29-rc9)  I guess I'd like to do a little test before acking,
though it looks ok based on your answer.

thanks,
-serge

Re: [PATCH] devcgroup: avoid using cgroup_lock

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Li Zefan (lizf@cn.fujitsu.com):
> > >> @@ -426,11 +431,11 @@ static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
> > >>  				  const char *buffer)
> > >>  {
> > >>  	int retval;
> > >> -	if (!cgroup_lock_live_group(cgrp))
> > > 
> > > Does it matter that we no longer check for cgroup_is_removed()?
> > > 
> > 
> > No, this means in a rare case that the write handler is called when the cgroup
> > is dead, we still do the update work instead of returning ENODEV.
> > 
> > This is ok, since at that time, accessing cgroup and devcgroup is still valid,
> > but will have no effect since there is no task in this cgroup and the cgroup
> > will be destroyed soon.
> 
> Ok, just wanted to make sure the devcgroup couldn't be partially torn
> down and risking NULL or freed-memory derefs...

Ok, so the cgroup's files will be deleted first, then on the directory
removal the cgroup's data (each whitelist entry) is deleted.  So we can
let that ordering (by cgroup_clear_directory) ensure that nothing inside
a file write can happen while the destroy handler is called, right?

(That's why I was worried about not using the cgroup_lock: we need some
way of synchronizing those.  But I guess we're fine)

> BTW is that against linux-next?  (didn't seem to apply cleanly against
> my 2.6.29-rc9)  I guess I'd like to do a little test before acking,
> though it looks ok based on your answer.

Acked-by: Serge Hallyn <serue@us.ibm.com>

-serge

Re: [PATCH] devcgroup: avoid using cgroup_lock

[Li Zefan]

Serge E. Hallyn wrote:
> Quoting Serge E. Hallyn (serue@us.ibm.com):
>> Quoting Li Zefan (lizf@cn.fujitsu.com):
>>>>> @@ -426,11 +431,11 @@ static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
>>>>>  				  const char *buffer)
>>>>>  {
>>>>>  	int retval;
>>>>> -	if (!cgroup_lock_live_group(cgrp))
>>>> Does it matter that we no longer check for cgroup_is_removed()?
>>>>
>>> No, this means in a rare case that the write handler is called when the cgroup
>>> is dead, we still do the update work instead of returning ENODEV.
>>>
>>> This is ok, since at that time, accessing cgroup and devcgroup is still valid,
>>> but will have no effect since there is no task in this cgroup and the cgroup
>>> will be destroyed soon.
>> Ok, just wanted to make sure the devcgroup couldn't be partially torn
>> down and risking NULL or freed-memory derefs...
> 
> Ok, so the cgroup's files will be deleted first, then on the directory
> removal the cgroup's data (each whitelist entry) is deleted.  So we can
> let that ordering (by cgroup_clear_directory) ensure that nothing inside
> a file write can happen while the destroy handler is called, right?
> 

When we are in the read/write handler, we have a pin in the dir's dentry
(dentry->d_count > 0), thus cgroup_diput() which destroys the cgroup won't
be called during the read/write.

> (That's why I was worried about not using the cgroup_lock: we need some
> way of synchronizing those.  But I guess we're fine)
> 

Many read/write handlers in other cgroup subsystems don't take cgroup_lock.
:)

>> BTW is that against linux-next?  (didn't seem to apply cleanly against
>> my 2.6.29-rc9)  I guess I'd like to do a little test before acking,
>> though it looks ok based on your answer.
> 
> Acked-by: Serge Hallyn <serue@us.ibm.com>
> 
> -serge
> 
>