devcgroup: avoid using cgroup_lock

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 <[email protected]>

---
 security/device_cgroup.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

security/device_cgroup.c +10 -5

index 3aacd0f..5fda7df 100644
@@ -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)

security/device_cgroup.c +3 -2

-		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;
 }
 

Signature · 1 line

1.5.4.rc3

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 <[email protected]>

---
 security/device_cgroup.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

security/device_cgroup.c +10 -5

index 3aacd0f..5fda7df 100644
@@ -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)

security/device_cgroup.c +3 -2

-		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;
 }
 

Signature · 1 line

1.5.4.rc3

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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()?

Quoted 9 lines

security/device_cgroup.c +3 -2

-		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;
 }

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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()?

Quoted 9 lines

security/device_cgroup.c +3 -2

-		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;
 }

Quoted 8 lines

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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.

Quoted 8 lines

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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.

Quoted 12 lines · Li Zefan

Quoted 8 lines

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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.

Quoted 3 lines

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.

Quoted 12 lines · Li Zefan

Quoted 8 lines

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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.

Quoted 3 lines

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.

Quoted 14 lines · Serge E. Hallyn

Quoted 13 lines · Li Zefan

Quoted 7 lines

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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.

Quoted 2 lines

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)

Quoted 3 lines

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-r/[email protected]>

-serge

Quoted 14 lines · Serge E. Hallyn

Quoted 13 lines · Li Zefan

Quoted 7 lines

security/device_cgroup.c static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft, · line 429

Quoted 5 lines

security/device_cgroup.c +0 -1

@@ -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.

Quoted 2 lines

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)

Quoted 3 lines

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 <[email protected]>

-serge