On 06/23/2014 08:34 AM, Stephen Smalley wrote:
On 06/20/2014 08:30 PM, Waiman Long wrote:v1->v2: - Add an internal helper to switch on/off lock acquisition instead of modifying the external API.With introduction of fair queued rwlock, recursive read_lock() may hang the offending process if there is a write_lock() somewhere in between. With recursive read_lock checking enabled, the following error was reported: ============================================= [ INFO: possible recursive locking detected ] 3.16.0-rc1 #2 Tainted: G E --------------------------------------------- load_policy/708 is trying to acquire lock: (policy_rwlock){.+.+..}, at: [<ffffffff8125b32a>] security_genfs_sid+0x3a/0x170 but task is already holding lock: (policy_rwlock){.+.+..}, at: [<ffffffff8125b48c>] security_fs_use+0x2c/0x110 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(policy_rwlock); lock(policy_rwlock); This patch fixes the occurrence of recursive read_lock() of policy_rwlock in security_genfs_sid() by adding a helper function which has a 5th argument to indicate if the rwlock has been taken. Signed-off-by: Waiman Long<[email protected]> --- security/selinux/ss/services.c | 36 ++++++++++++++++++++++++++++-------- 1 files changed, 28 insertions(+), 8 deletions(-) diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 4bca494..5f4c1f3 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2277,20 +2277,22 @@ out: } /** - * security_genfs_sid - Obtain a SID for a file in a filesystem + * __security_genfs_sid - Helper to obtain a SID for a file in a filesystem * @fstype: filesystem type * @path: path from root of mount * @sclass: file security class * @sid: SID for path + * @locked: true if policy_rwlock taken * * Obtain a SID to use for a file in a filesystem that * cannot support xattr or use a fixed labeling behavior like * transition SIDs or task SIDs. */ -int security_genfs_sid(const char *fstype, - char *path, - u16 orig_sclass, - u32 *sid) +static inline int __security_genfs_sid(const char *fstype, + char *path, + u16 orig_sclass, + u32 *sid, + int locked) { int len; u16 sclass; @@ -2301,7 +2303,8 @@ int security_genfs_sid(const char *fstype, while (path[0] == '/'&& path[1] == '/') path++; - read_lock(&policy_rwlock); + if (!locked) + read_lock(&policy_rwlock);I believe that this kind of conditional lock-taking is frowned upon in the kernel, although I could be wrong. I think it would be cleaner to instead just unconditionally take and release the lock around the call to this helper in security_genfs_sid(), and not do so around the call to it from security_fs_use().
Thank for the comments. Will send out a new patch with the suggested change. -Longman -- 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/
