include/linux/cgroup.h:566 suspicious rcu_dereference_check() usage!

DISCUSSION Cristian Rodríguez 3 participants 5 messages Oct 5, 2012 – Oct 10, 2012 Security (LSM)

Share & tools

Paul E. McKenney

2012-10-08 19:49 UTC #1 lore raw

Quoted 40 lines · Cristian Rodríguez

Hi:
I am getting this in the current linus tree.

[    0.408781] ===============================
[    0.408783] [ INFO: suspicious RCU usage. ]
[    0.408786] 3.6.0-canneverbe-07124-g5f3d2f2 #18 Not tainted
[    0.408789] -------------------------------
[    0.408791] include/linux/cgroup.h:566 suspicious
rcu_dereference_check() usage!
[    0.408795]
[    0.408795] other info that might help us debug this:
[    0.408795]
[    0.408799]
[    0.408799] rcu_scheduler_active = 1, debug_locks = 0
[    0.408802] 2 locks held by kdevtmpfs/49:
[    0.408804]  #0:  (sb_writers){.+.+.+}, at: [<ffffffff8119a2ef>]
mnt_want_write+0x1f/0x50
[    0.408814]  #1:  (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at:
[<ffffffff811857cf>] kern_path_create+0x7f/0x170
[    0.408822]
[    0.408822] stack backtrace:
[    0.408825] Pid: 49, comm: kdevtmpfs Not tainted
3.6.0-canneverbe-07124-g5f3d2f2 #18
[    0.408829] Call Trace:
[    0.408834]  [<ffffffff8109fabd>] lockdep_rcu_suspicious+0xfd/0x130
[    0.408838]  [<ffffffff812a876d>] devcgroup_inode_mknod+0x18d/0x230
[    0.408843]  [<ffffffff8126f923>] ? security_capable+0x13/0x20
[    0.408848]  [<ffffffff8104a06f>] ? ns_capable+0x3f/0x80
[    0.408851]  [<ffffffff81186c09>] vfs_mknod+0x79/0x140
[    0.408856]  [<ffffffff813a6df2>] handle_create.isra.2+0x72/0x200
[    0.408860]  [<ffffffff813a702c>] ? devtmpfsd+0xac/0x140
[    0.408863]  [<ffffffff813a7094>] devtmpfsd+0x114/0x140
[    0.408867]  [<ffffffff813a6f80>] ? handle_create.isra.2+0x200/0x200
[    0.408871]  [<ffffffff81064de6>] kthread+0xd6/0xe0
[    0.408877]  [<ffffffff81578cb4>] kernel_thread_helper+0x4/0x10
[    0.408881]  [<ffffffff8156f85c>] ? retint_restore_args+0xe/0xe
[    0.408885]  [<ffffffff81064d10>] ? flush_kthread_work+0x190/0x190
[    0.408889]  [<ffffffff81578cb0>] ? gs_change+0xb/0xb

Quoted 1 line

Hope someone can take a look at it.

It looks to me like commit #ad676077 (device_cgroup: convert device_cgroup internally to policy + exceptions) removed a needed rcu_read_lock(). See below for an untested patch restoring rcu_read_lock().

Thanx, Paul

------------------------------------------------------------------------

device_cgroup: Restore rcu_read_lock() protection to devcgroup_inode_mknod()

Commit ad676077 (device_cgroup: convert device_cgroup internally to policy + exceptions) restructured devcgroup_inode_mknod(), removing rcu_read_lock() in the process. However, RCU read-side protection is required by the call to task_devcgroup(), so this commit restores the rcu_read_lock() and rcu_read_unlock().

Signed-off-by Paul E. McKenney <[email protected]>

security/device_cgroup.c +7 -2

index 44dfc41..c686110 100644
@@ -576,9 +576,12 @@ int __devcgroup_inode_permission(struct inode *inode, int mask)
 
 int devcgroup_inode_mknod(int mode, dev_t dev)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type;
 
+	rcu_read_lock();
+	dev_cgroup = task_devcgroup(current);
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))
 		return 0;

@@ -587,7 +590,9 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 	else
 		type = DEV_CHAR;
 
-	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
+	ret =  __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
 					    MINOR(dev), ACC_MKNOD);
+	rcu_read_unlock();
+	return ret;
 
 }

Sergey Senozhatsky

2012-10-10 01:11 UTC #2 lore raw

Quoted 33 lines · Paul E. McKenney

------------------------------------------------------------------------
device_cgroup: Restore rcu_read_lock() protection to devcgroup_inode_mknod()
Commit ad676077 (device_cgroup: convert device_cgroup internally to policy + exceptions) restructured devcgroup_inode_mknod(), removing rcu_read_lock() in the process. However, RCU read-side protection is required by the call to task_devcgroup(), so this commit restores the rcu_read_lock() and rcu_read_unlock().
Signed-off-by Paul E. McKenney <[email protected]>
security/device_cgroup.c +4 -1
index 44dfc41..c686110 100644
@@ -576,9 +576,12 @@ int __devcgroup_inode_permission(struct inode *inode, int mask)
 
 int devcgroup_inode_mknod(int mode, dev_t dev)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type;
 
+	rcu_read_lock();
+	dev_cgroup = task_devcgroup(current);
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))
 		return 0;
 

security/device_cgroup.c int devcgroup_inode_mknod(int mode, dev_t dev) · line 598

Quoted 10 lines

security/device_cgroup.c +3 -1

@@ -587,7 +590,9 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 	else
 		type = DEV_CHAR;
 
-	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
+	ret =  __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
 					    MINOR(dev), ACC_MKNOD);
+	rcu_read_unlock();
+	return ret;
 
 }

I believe the same should be done for __devcgroup_inode_permission() as well. And we probably can call task_devcgroup() and rcu_read_lock() after "S_ISBLK(mode) && !S_ISCHR(mode)" checks (I guess we also need to unlock RCU on `return 0').

----------------------------------------------------------------------------

Commit ad676077

 | Author: Aristeu Rozanski <[email protected]>
 | Date:   Thu Oct 4 17:15:17 2012 -0700
 |   device_cgroup: convert device_cgroup internally to policy + exceptions

moved RCU read-side protection from devcgroup_inode_mknod(), which, however is required by task_devcgroup(). Patch also add RCU read-side protection to __devcgroup_inode_permission() function, introduced in commit ad676077.

[    0.946303] include/linux/cgroup.h:566 suspicious rcu_dereference_check() usage!
[    0.946511] 
[    0.946606] 2 locks held by kdevtmpfs/28:
[    0.946684]  #0:  (sb_writers){.+.+.+}, at: [<ffffffff81144bcb>] mnt_want_write+0x24/0x4b
[    0.947083]  #1:  (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at: [<ffffffff81133d04>] kern_path_create+0x83/0x144
[    0.947598] 
[    0.947787] Call Trace:
[    0.947868]  [<ffffffff81089644>] lockdep_rcu_suspicious+0x109/0x112
[    0.947958]  [<ffffffff81258fa0>] devcgroup_inode_mknod+0x9e/0xee
[    0.948043]  [<ffffffff81132ee7>] vfs_mknod+0x8a/0xed
[    0.948129]  [<ffffffff813b98af>] handle_create.isra.2+0x144/0x1b5
[    0.948214]  [<ffffffff813b99bf>] ? devtmpfsd+0x9f/0x138
[    0.948298]  [<ffffffff81295d5c>] ? do_raw_spin_lock+0x67/0xde
[    0.948384]  [<ffffffff81295e92>] ? do_raw_spin_unlock+0x8f/0x98
[    0.948469]  [<ffffffff813b9920>] ? handle_create.isra.2+0x1b5/0x1b5
[    0.948554]  [<ffffffff813b9a04>] devtmpfsd+0xe4/0x138
[    0.948638]  [<ffffffff813b9920>] ? handle_create.isra.2+0x1b5/0x1b5
[    0.948724]  [<ffffffff810582b6>] kthread+0xd5/0xdd
[    0.948814]  [<ffffffff814db664>] kernel_thread_helper+0x4/0x10
[    0.948900]  [<ffffffff814d2973>] ? retint_restore_args+0x13/0x13
[    0.948985]  [<ffffffff810581e1>] ? __init_kthread_worker+0x5a/0x5a
[    0.949069]  [<ffffffff814db660>] ? gs_change+0x13/0x13

devcgroup_inode_mknod() part submitted by Paul E. McKenney.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

 security/device_cgroup.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

security/device_cgroup.c +17 -4

index 44dfc41..043eb00 100644
@@ -558,7 +558,8 @@ static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup,
 
 int __devcgroup_inode_permission(struct inode *inode, int mask)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type, access = 0;
 
 	if (S_ISBLK(inode->i_mode))

@@ -570,13 +571,20 @@ int __devcgroup_inode_permission(struct inode *inode, int mask)
 	if (mask & MAY_READ)
 		access |= ACC_READ;
 
-	return __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
+	rcu_read_lock();
+
+	dev_cgroup = task_devcgroup(current);
+	ret = __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
 					    iminor(inode), access);
+
+	rcu_read_unlock();
+	return ret;
 }
 
 int devcgroup_inode_mknod(int mode, dev_t dev)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type;
 
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))

@@ -587,7 +595,12 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 	else
 		type = DEV_CHAR;
 
-	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
+	rcu_read_lock();
+
+	dev_cgroup = task_devcgroup(current);
+	ret = __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
 					    MINOR(dev), ACC_MKNOD);
 
+	rcu_read_unlock();
+	return ret; 
 }

Paul E. McKenney

2012-10-10 03:25 UTC #3 lore raw

Quoted 42 lines · Sergey Senozhatsky

Quoted 41 lines · Paul E. McKenney
------------------------------------------------------------------------
device_cgroup: Restore rcu_read_lock() protection to devcgroup_inode_mknod()
Commit ad676077 (device_cgroup: convert device_cgroup internally to policy + exceptions) restructured devcgroup_inode_mknod(), removing rcu_read_lock() in the process. However, RCU read-side protection is required by the call to task_devcgroup(), so this commit restores the rcu_read_lock() and rcu_read_unlock().
Signed-off-by Paul E. McKenney <[email protected]>
security/device_cgroup.c +7 -2
index 44dfc41..c686110 100644
@@ -576,9 +576,12 @@ int __devcgroup_inode_permission(struct inode *inode, int mask)
 
 int devcgroup_inode_mknod(int mode, dev_t dev)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type;
 
+	rcu_read_lock();
+	dev_cgroup = task_devcgroup(current);
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))
 		return 0;
 
@@ -587,7 +590,9 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 	else
 		type = DEV_CHAR;
 
-	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
+	ret =  __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
 					    MINOR(dev), ACC_MKNOD);
+	rcu_read_unlock();
+	return ret;
 

Quoted 7 lines

security/device_cgroup.c int devcgroup_inode_mknod(int mode, dev_t dev) · line 598
Quoted 2 lines
security/device_cgroup.c +0 -0
 }
I believe the same should be done for __devcgroup_inode_permission() as well. And we probably can call task_devcgroup() and rcu_read_lock() after "S_ISBLK(mode) && !S_ISCHR(mode)" checks (I guess we also need to unlock RCU on `return 0').

Looks sane to me! Dropping my patch.

Thanx, Paul

Quoted 103 lines

----------------------------------------------------------------------------

Commit ad676077

 | Author: Aristeu Rozanski <[email protected]>
 | Date:   Thu Oct 4 17:15:17 2012 -0700
 |   device_cgroup: convert device_cgroup internally to policy + exceptions

[    0.946303] include/linux/cgroup.h:566 suspicious rcu_dereference_check() usage!
[    0.946511] 
[    0.946606] 2 locks held by kdevtmpfs/28:
[    0.946684]  #0:  (sb_writers){.+.+.+}, at: [<ffffffff81144bcb>] mnt_want_write+0x24/0x4b
[    0.947083]  #1:  (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at: [<ffffffff81133d04>] kern_path_create+0x83/0x144
[    0.947598] 
[    0.947787] Call Trace:
[    0.947868]  [<ffffffff81089644>] lockdep_rcu_suspicious+0x109/0x112
[    0.947958]  [<ffffffff81258fa0>] devcgroup_inode_mknod+0x9e/0xee
[    0.948043]  [<ffffffff81132ee7>] vfs_mknod+0x8a/0xed
[    0.948129]  [<ffffffff813b98af>] handle_create.isra.2+0x144/0x1b5
[    0.948214]  [<ffffffff813b99bf>] ? devtmpfsd+0x9f/0x138
[    0.948298]  [<ffffffff81295d5c>] ? do_raw_spin_lock+0x67/0xde
[    0.948384]  [<ffffffff81295e92>] ? do_raw_spin_unlock+0x8f/0x98
[    0.948469]  [<ffffffff813b9920>] ? handle_create.isra.2+0x1b5/0x1b5
[    0.948554]  [<ffffffff813b9a04>] devtmpfsd+0xe4/0x138
[    0.948638]  [<ffffffff813b9920>] ? handle_create.isra.2+0x1b5/0x1b5
[    0.948724]  [<ffffffff810582b6>] kthread+0xd5/0xdd
[    0.948814]  [<ffffffff814db664>] kernel_thread_helper+0x4/0x10
[    0.948900]  [<ffffffff814d2973>] ? retint_restore_args+0x13/0x13
[    0.948985]  [<ffffffff810581e1>] ? __init_kthread_worker+0x5a/0x5a
[    0.949069]  [<ffffffff814db660>] ? gs_change+0x13/0x13

devcgroup_inode_mknod() part submitted by Paul E. McKenney.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

 security/device_cgroup.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

security/device_cgroup.c +17 -5

index 44dfc41..043eb00 100644
@@ -558,7 +558,8 @@ static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup,
 
 int __devcgroup_inode_permission(struct inode *inode, int mask)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type, access = 0;
 
 	if (S_ISBLK(inode->i_mode))

@@ -570,13 +571,20 @@ int __devcgroup_inode_permission(struct inode *inode, int mask)
 	if (mask & MAY_READ)
 		access |= ACC_READ;
 
-	return __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
+	rcu_read_lock();
+
+	dev_cgroup = task_devcgroup(current);
+	ret = __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
 					    iminor(inode), access);
+
+	rcu_read_unlock();
+	return ret;
 }
 
 int devcgroup_inode_mknod(int mode, dev_t dev)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type;
 
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))

@@ -587,7 +595,12 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 	else
 		type = DEV_CHAR;
 
-	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
+	rcu_read_lock();
+
+	dev_cgroup = task_devcgroup(current);
+	ret = __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
 					    MINOR(dev), ACC_MKNOD);
 
+	rcu_read_unlock();
+	return ret; 
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Sergey Senozhatsky

2012-10-10 23:23 UTC #4 lore raw

Quoted 5 lines · Paul E. McKenney

Looks sane to me! Dropping my patch.
Thanx, Paul

Thanks a lot, Paul. I will resend with proper subject, so it'll be more visible in the list.

-ss

Quoted 104 lines

Quoted 103 lines

----------------------------------------------------------------------------

Commit ad676077

 | Author: Aristeu Rozanski <[email protected]>
 | Date:   Thu Oct 4 17:15:17 2012 -0700
 |   device_cgroup: convert device_cgroup internally to policy + exceptions

[    0.946303] include/linux/cgroup.h:566 suspicious rcu_dereference_check() usage!
[    0.946511] 
[    0.946606] 2 locks held by kdevtmpfs/28:
[    0.946684]  #0:  (sb_writers){.+.+.+}, at: [<ffffffff81144bcb>] mnt_want_write+0x24/0x4b
[    0.947083]  #1:  (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at: [<ffffffff81133d04>] kern_path_create+0x83/0x144
[    0.947598] 
[    0.947787] Call Trace:
[    0.947868]  [<ffffffff81089644>] lockdep_rcu_suspicious+0x109/0x112
[    0.947958]  [<ffffffff81258fa0>] devcgroup_inode_mknod+0x9e/0xee
[    0.948043]  [<ffffffff81132ee7>] vfs_mknod+0x8a/0xed
[    0.948129]  [<ffffffff813b98af>] handle_create.isra.2+0x144/0x1b5
[    0.948214]  [<ffffffff813b99bf>] ? devtmpfsd+0x9f/0x138
[    0.948298]  [<ffffffff81295d5c>] ? do_raw_spin_lock+0x67/0xde
[    0.948384]  [<ffffffff81295e92>] ? do_raw_spin_unlock+0x8f/0x98
[    0.948469]  [<ffffffff813b9920>] ? handle_create.isra.2+0x1b5/0x1b5
[    0.948554]  [<ffffffff813b9a04>] devtmpfsd+0xe4/0x138
[    0.948638]  [<ffffffff813b9920>] ? handle_create.isra.2+0x1b5/0x1b5
[    0.948724]  [<ffffffff810582b6>] kthread+0xd5/0xdd
[    0.948814]  [<ffffffff814db664>] kernel_thread_helper+0x4/0x10
[    0.948900]  [<ffffffff814d2973>] ? retint_restore_args+0x13/0x13
[    0.948985]  [<ffffffff810581e1>] ? __init_kthread_worker+0x5a/0x5a
[    0.949069]  [<ffffffff814db660>] ? gs_change+0x13/0x13

devcgroup_inode_mknod() part submitted by Paul E. McKenney.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

 security/device_cgroup.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

security/device_cgroup.c +17 -5

index 44dfc41..043eb00 100644
@@ -558,7 +558,8 @@ static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup,
 
 int __devcgroup_inode_permission(struct inode *inode, int mask)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type, access = 0;
 
 	if (S_ISBLK(inode->i_mode))

@@ -570,13 +571,20 @@ int __devcgroup_inode_permission(struct inode *inode, int mask)
 	if (mask & MAY_READ)
 		access |= ACC_READ;
 
-	return __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
+	rcu_read_lock();
+
+	dev_cgroup = task_devcgroup(current);
+	ret = __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
 					    iminor(inode), access);
+
+	rcu_read_unlock();
+	return ret;
 }
 
 int devcgroup_inode_mknod(int mode, dev_t dev)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
+	struct dev_cgroup *dev_cgroup;
+	int ret;
 	short type;
 
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))

@@ -587,7 +595,12 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 	else
 		type = DEV_CHAR;
 
-	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
+	rcu_read_lock();
+
+	dev_cgroup = task_devcgroup(current);
+	ret = __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
 					    MINOR(dev), ACC_MKNOD);
 
+	rcu_read_unlock();
+	return ret; 
 }
 
--