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);
sclass = unmap_class(orig_sclass);
*sid = SECINITSID_UNLABELED;
@@ -2336,11 +2339,27 @@ int security_genfs_sid(const char *fstype,
*sid = c->sid[0];
rc = 0;
out:
- read_unlock(&policy_rwlock);
+ if (!locked)
+ read_unlock(&policy_rwlock);
return rc;
}
/**
+ * security_genfs_sid - 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
+ */
+int security_genfs_sid(const char *fstype,
+ char *path,
+ u16 orig_sclass,
+ u32 *sid)
+{
+ return __security_genfs_sid(fstype, path, orig_sclass, sid, false);
+}
+
+/**
* security_fs_use - Determine how to handle labeling for a filesystem.
* @sb: superblock in question
*/
@@ -2370,7 +2389,8 @@ int security_fs_use(struct super_block *sb)
}
sbsec->sid = c->sid[0];
} else {
- rc = security_genfs_sid(fstype, "/", SECCLASS_DIR, &sbsec->sid);
+ rc = __security_genfs_sid(fstype, "/", SECCLASS_DIR,
+ &sbsec->sid, true);
if (rc) {
sbsec->behavior = SECURITY_FS_USE_NONE;
rc = 0;
--
1.7.1
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().
>
> sclass = unmap_class(orig_sclass);
> *sid = SECINITSID_UNLABELED;
> @@ -2336,11 +2339,27 @@ int security_genfs_sid(const char *fstype,
> *sid = c->sid[0];
> rc = 0;
> out:
> - read_unlock(&policy_rwlock);
> + if (!locked)
> + read_unlock(&policy_rwlock);
> return rc;
> }
>
> /**
> + * security_genfs_sid - 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
> + */
> +int security_genfs_sid(const char *fstype,
> + char *path,
> + u16 orig_sclass,
> + u32 *sid)
> +{
> + return __security_genfs_sid(fstype, path, orig_sclass, sid, false);
> +}
> +
> +/**
> * security_fs_use - Determine how to handle labeling for a filesystem.
> * @sb: superblock in question
> */
> @@ -2370,7 +2389,8 @@ int security_fs_use(struct super_block *sb)
> }
> sbsec->sid = c->sid[0];
> } else {
> - rc = security_genfs_sid(fstype, "/", SECCLASS_DIR, &sbsec->sid);
> + rc = __security_genfs_sid(fstype, "/", SECCLASS_DIR,
> + &sbsec->sid, true);
> if (rc) {
> sbsec->behavior = SECURITY_FS_USE_NONE;
> rc = 0;
>
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