[RFC PATCH 0/2] Avoid blocking in selinux inode callbacks on RCU walk

[RFC PATCH 0/2] Avoid blocking in selinux inode callbacks on RCU walk

[Will Deacon]

Hi all,

While debugging a KASAN report in the selinux access vector cache hash
table, I noticed that it looks like we may block in the inode_follow_link()
and inode_permission() callbacks, even when called from the VFS layer as
part of an RCU-protected path walk.

These two patches attempt to fix that, but since I found this by
inspection and I'm not familiar with this code, I'm sending as an RFC in
case I missed something that means this cannot happen.

Comments very welcome,

Will

Cc: Paul Moore <paul@paul-moore.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Jeffrey Vander Stoep <jeffv@google.com>

--->8

Will Deacon (2):
  selinux: Don't call avc_compute_av() from RCU path walk
  selinux: Propagate RCU walk status from 'security_inode_follow_link()'

 security/selinux/avc.c         | 21 +++++++++++++--------
 security/selinux/hooks.c       |  5 +++--
 security/selinux/include/avc.h | 12 ++++++++----
 3 files changed, 24 insertions(+), 14 deletions(-)

-- 
2.24.0.432.g9d3f5f5b63-goog

[RFC PATCH 1/2] selinux: Don't call avc_compute_av() from RCU path walk

[Will Deacon]

'avc_compute_av()' can block, so we carefully exit the RCU read-side
critical section before calling it in 'avc_has_perm_noaudit()'.
Unfortunately, if we're calling from the VFS layer on the RCU path walk
via 'selinux_inode_permission()' then we're still actually in an RCU
read-side critical section and must not block.

'avc_denied()' already handles this by simply returning success and
postponing the auditing until we're called again on the slowpath, so
follow the same approach here and return early if the node lookup fails
on the RCU walk path.

Signed-off-by: Will Deacon <will@kernel.org>
---
 security/selinux/avc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index ecd3829996aa..9c183c899e92 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -1159,16 +1159,19 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
 	rcu_read_lock();
 
 	node = avc_lookup(state->avc, ssid, tsid, tclass);
-	if (unlikely(!node))
+	if (unlikely(!node)) {
+		if (flags & AVC_NONBLOCKING)
+			goto out;
 		node = avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node);
-	else
+	} else {
 		memcpy(avd, &node->ae.avd, sizeof(*avd));
+	}
 
 	denied = requested & ~(avd->allowed);
 	if (unlikely(denied))
 		rc = avc_denied(state, ssid, tsid, tclass, requested, 0, 0,
 				flags, avd);
-
+out:
 	rcu_read_unlock();
 	return rc;
 }
-- 
2.24.0.432.g9d3f5f5b63-goog

[RFC PATCH 2/2] selinux: Propagate RCU walk status from 'security_inode_follow_link()'

[Will Deacon]

'selinux_inode_follow_link()' can be called as part of an RCU path walk,
and is passed a 'bool rcu' parameter to indicate whether or not it is
being called from within an RCU read-side critical section.

Unfortunately, this knowledge is not propagated further and, instead,
'avc_has_perm()' unconditionally passes a flags argument of '0' to both
'avc_has_perm_noaudit()' and 'avc_audit()' which may block.

Introduce 'avc_has_perm_flags()' which can be used safely from within an
RCU read-side critical section.

Signed-off-by: Will Deacon <will@kernel.org>
---
 security/selinux/avc.c         | 12 +++++++-----
 security/selinux/hooks.c       |  5 +++--
 security/selinux/include/avc.h | 12 ++++++++----
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 9c183c899e92..7d99dadd24d0 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -1177,11 +1177,12 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
 }
 
 /**
- * avc_has_perm - Check permissions and perform any appropriate auditing.
+ * avc_has_perm_flags - Check permissions and perform any appropriate auditing.
  * @ssid: source security identifier
  * @tsid: target security identifier
  * @tclass: target security class
  * @requested: requested permissions, interpreted based on @tclass
+ * @flags: AVC_STRICT, AVC_NONBLOCKING, or 0
  * @auditdata: auxiliary audit data
  *
  * Check the AVC to determine whether the @requested permissions are granted
@@ -1192,17 +1193,18 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
  * permissions are granted, -%EACCES if any permissions are denied, or
  * another -errno upon other errors.
  */
-int avc_has_perm(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass,
-		 u32 requested, struct common_audit_data *auditdata)
+int avc_has_perm_flags(struct selinux_state *state, u32 ssid, u32 tsid,
+		       u16 tclass, u32 requested, unsigned int flags,
+		       struct common_audit_data *auditdata)
 {
 	struct av_decision avd;
 	int rc, rc2;
 
-	rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, 0,
+	rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, flags,
 				  &avd);
 
 	rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc,
-			auditdata, 0);
+			auditdata, flags);
 	if (rc2)
 		return rc2;
 	return rc;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9625b99e677f..0c09f59a2740 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3008,8 +3008,9 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
 	if (IS_ERR(isec))
 		return PTR_ERR(isec);
 
-	return avc_has_perm(&selinux_state,
-			    sid, isec->sid, isec->sclass, FILE__READ, &ad);
+	return avc_has_perm_flags(&selinux_state, sid, isec->sid, isec->sclass,
+				  rcu ? AVC_NONBLOCKING : 0,
+				  FILE__READ, &ad);
 }
 
 static noinline int audit_inode_permission(struct inode *inode,
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index 7be0e1e90e8b..0450e1b88182 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -149,10 +149,14 @@ int avc_has_perm_noaudit(struct selinux_state *state,
 			 unsigned flags,
 			 struct av_decision *avd);
 
-int avc_has_perm(struct selinux_state *state,
-		 u32 ssid, u32 tsid,
-		 u16 tclass, u32 requested,
-		 struct common_audit_data *auditdata);
+int avc_has_perm_flags(struct selinux_state *state,
+		       u32 ssid, u32 tsid,
+		       u16 tclass, u32 requested,
+		       unsigned flags,
+		       struct common_audit_data *auditdata);
+
+#define avc_has_perm(state, ssid, tsid, tclass, requested, auditdata) \
+	avc_has_perm_flags(state, ssid, tsid, tclass, requested, 0, auditdata)
 
 int avc_has_extended_perms(struct selinux_state *state,
 			   u32 ssid, u32 tsid, u16 tclass, u32 requested,
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [RFC PATCH 2/2] selinux: Propagate RCU walk status from 'security_inode_follow_link()'

[Stephen Smalley]

On 11/19/19 1:40 PM, Will Deacon wrote:
> 'selinux_inode_follow_link()' can be called as part of an RCU path walk,
> and is passed a 'bool rcu' parameter to indicate whether or not it is
> being called from within an RCU read-side critical section.
> 
> Unfortunately, this knowledge is not propagated further and, instead,
> 'avc_has_perm()' unconditionally passes a flags argument of '0' to both
> 'avc_has_perm_noaudit()' and 'avc_audit()' which may block.
> 
> Introduce 'avc_has_perm_flags()' which can be used safely from within an
> RCU read-side critical section.

Please see e46e01eebbbcf2ff6d28ee7cae9f117e9d1572c8 ("selinux: stop 
passing MAY_NOT_BLOCK to the AVC upon follow_link").

> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   security/selinux/avc.c         | 12 +++++++-----
>   security/selinux/hooks.c       |  5 +++--
>   security/selinux/include/avc.h | 12 ++++++++----
>   3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 9c183c899e92..7d99dadd24d0 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -1177,11 +1177,12 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
>   }
>   
>   /**
> - * avc_has_perm - Check permissions and perform any appropriate auditing.
> + * avc_has_perm_flags - Check permissions and perform any appropriate auditing.
>    * @ssid: source security identifier
>    * @tsid: target security identifier
>    * @tclass: target security class
>    * @requested: requested permissions, interpreted based on @tclass
> + * @flags: AVC_STRICT, AVC_NONBLOCKING, or 0
>    * @auditdata: auxiliary audit data
>    *
>    * Check the AVC to determine whether the @requested permissions are granted
> @@ -1192,17 +1193,18 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
>    * permissions are granted, -%EACCES if any permissions are denied, or
>    * another -errno upon other errors.
>    */
> -int avc_has_perm(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass,
> -		 u32 requested, struct common_audit_data *auditdata)
> +int avc_has_perm_flags(struct selinux_state *state, u32 ssid, u32 tsid,
> +		       u16 tclass, u32 requested, unsigned int flags,
> +		       struct common_audit_data *auditdata)
>   {
>   	struct av_decision avd;
>   	int rc, rc2;
>   
> -	rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, 0,
> +	rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, flags,
>   				  &avd);
>   
>   	rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc,
> -			auditdata, 0);
> +			auditdata, flags);
>   	if (rc2)
>   		return rc2;
>   	return rc;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9625b99e677f..0c09f59a2740 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3008,8 +3008,9 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
>   	if (IS_ERR(isec))
>   		return PTR_ERR(isec);
>   
> -	return avc_has_perm(&selinux_state,
> -			    sid, isec->sid, isec->sclass, FILE__READ, &ad);
> +	return avc_has_perm_flags(&selinux_state, sid, isec->sid, isec->sclass,
> +				  rcu ? AVC_NONBLOCKING : 0,
> +				  FILE__READ, &ad);
>   }
>   
>   static noinline int audit_inode_permission(struct inode *inode,
> diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
> index 7be0e1e90e8b..0450e1b88182 100644
> --- a/security/selinux/include/avc.h
> +++ b/security/selinux/include/avc.h
> @@ -149,10 +149,14 @@ int avc_has_perm_noaudit(struct selinux_state *state,
>   			 unsigned flags,
>   			 struct av_decision *avd);
>   
> -int avc_has_perm(struct selinux_state *state,
> -		 u32 ssid, u32 tsid,
> -		 u16 tclass, u32 requested,
> -		 struct common_audit_data *auditdata);
> +int avc_has_perm_flags(struct selinux_state *state,
> +		       u32 ssid, u32 tsid,
> +		       u16 tclass, u32 requested,
> +		       unsigned flags,
> +		       struct common_audit_data *auditdata);
> +
> +#define avc_has_perm(state, ssid, tsid, tclass, requested, auditdata) \
> +	avc_has_perm_flags(state, ssid, tsid, tclass, requested, 0, auditdata)
>   
>   int avc_has_extended_perms(struct selinux_state *state,
>   			   u32 ssid, u32 tsid, u16 tclass, u32 requested,
> 

Re: [RFC PATCH 1/2] selinux: Don't call avc_compute_av() from RCU path walk

[Stephen Smalley]

On 11/19/19 1:40 PM, Will Deacon wrote:
> 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> critical section before calling it in 'avc_has_perm_noaudit()'.
> Unfortunately, if we're calling from the VFS layer on the RCU path walk
> via 'selinux_inode_permission()' then we're still actually in an RCU
> read-side critical section and must not block.

avc_compute_av() should never block AFAIK. The blocking concern was with 
slow_avc_audit(), and even that appears dubious to me. That seems to be 
more about misuse of d_find_alias in dump_common_audit_data() than anything.

> 
> 'avc_denied()' already handles this by simply returning success and
> postponing the auditing until we're called again on the slowpath, so
> follow the same approach here and return early if the node lookup fails
> on the RCU walk path.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   security/selinux/avc.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index ecd3829996aa..9c183c899e92 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -1159,16 +1159,19 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
>   	rcu_read_lock();
>   
>   	node = avc_lookup(state->avc, ssid, tsid, tclass);
> -	if (unlikely(!node))
> +	if (unlikely(!node)) {
> +		if (flags & AVC_NONBLOCKING)
> +			goto out;
>   		node = avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node);
> -	else
> +	} else {
>   		memcpy(avd, &node->ae.avd, sizeof(*avd));
> +	}
>   
>   	denied = requested & ~(avd->allowed);
>   	if (unlikely(denied))
>   		rc = avc_denied(state, ssid, tsid, tclass, requested, 0, 0,
>   				flags, avd);
> -
> +out:
>   	rcu_read_unlock();
>   	return rc;
>   }
> 

Re: [RFC PATCH 1/2] selinux: Don't call avc_compute_av() from RCU path walk

[Will Deacon]

Hi Stephen,

Thanks for the quick reply.

On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
> On 11/19/19 1:40 PM, Will Deacon wrote:
> > 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> > critical section before calling it in 'avc_has_perm_noaudit()'.
> > Unfortunately, if we're calling from the VFS layer on the RCU path walk
> > via 'selinux_inode_permission()' then we're still actually in an RCU
> > read-side critical section and must not block.
> 
> avc_compute_av() should never block AFAIK. The blocking concern was with
> slow_avc_audit(), and even that appears dubious to me. That seems to be more
> about misuse of d_find_alias in dump_common_audit_data() than anything.

Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
think it was propagated down to all of the potential allocations and
string functions. Having looked at it again, I can't see where it blocks.

Might be worth a comment in avc_compute_av(), because the temporary
dropping of rcu_read_lock() looks really dodgy when we could be running
on the RCU path walk path anyway.

Will

Re: [RFC PATCH 2/2] selinux: Propagate RCU walk status from 'security_inode_follow_link()'

[Will Deacon]

Hi Stephen,

Thanks for the quick review.

On Tue, Nov 19, 2019 at 01:46:37PM -0500, Stephen Smalley wrote:
> On 11/19/19 1:40 PM, Will Deacon wrote:
> > 'selinux_inode_follow_link()' can be called as part of an RCU path walk,
> > and is passed a 'bool rcu' parameter to indicate whether or not it is
> > being called from within an RCU read-side critical section.
> > 
> > Unfortunately, this knowledge is not propagated further and, instead,
> > 'avc_has_perm()' unconditionally passes a flags argument of '0' to both
> > 'avc_has_perm_noaudit()' and 'avc_audit()' which may block.
> > 
> > Introduce 'avc_has_perm_flags()' which can be used safely from within an
> > RCU read-side critical section.
> 
> Please see e46e01eebbbcf2ff6d28ee7cae9f117e9d1572c8 ("selinux: stop passing
> MAY_NOT_BLOCK to the AVC upon follow_link").

Ha, not sure how I missed that -- my patch is almost a direct revert,
including the name 'avs_has_perm_flags()'! My only concern is that the
commit message for e46e01eebbbc asserts that the only use of MAY_NOT_BLOCK
is in slow_avc_audit(), but AVC_NONBLOCKING is used more widely than that.

For example:

	selinux_inode_follow_link()
	  -> avc_has_perm()
	    -> avc_has_perm_noaudit()
	      -> avc_denied()
	        -> avc_update_node()

where we return early if AVC_NONBLOCKING is set, except flags are always
zero on this path.

Will

Re: [RFC PATCH 2/2] selinux: Propagate RCU walk status from 'security_inode_follow_link()'

[Stephen Smalley]

On 11/20/19 8:13 AM, Will Deacon wrote:
> Hi Stephen,
> 
> Thanks for the quick review.
> 
> On Tue, Nov 19, 2019 at 01:46:37PM -0500, Stephen Smalley wrote:
>> On 11/19/19 1:40 PM, Will Deacon wrote:
>>> 'selinux_inode_follow_link()' can be called as part of an RCU path walk,
>>> and is passed a 'bool rcu' parameter to indicate whether or not it is
>>> being called from within an RCU read-side critical section.
>>>
>>> Unfortunately, this knowledge is not propagated further and, instead,
>>> 'avc_has_perm()' unconditionally passes a flags argument of '0' to both
>>> 'avc_has_perm_noaudit()' and 'avc_audit()' which may block.
>>>
>>> Introduce 'avc_has_perm_flags()' which can be used safely from within an
>>> RCU read-side critical section.
>>
>> Please see e46e01eebbbcf2ff6d28ee7cae9f117e9d1572c8 ("selinux: stop passing
>> MAY_NOT_BLOCK to the AVC upon follow_link").
> 
> Ha, not sure how I missed that -- my patch is almost a direct revert,
> including the name 'avs_has_perm_flags()'! My only concern is that the
> commit message for e46e01eebbbc asserts that the only use of MAY_NOT_BLOCK
> is in slow_avc_audit(), but AVC_NONBLOCKING is used more widely than that.
> 
> For example:
> 
> 	selinux_inode_follow_link()
> 	  -> avc_has_perm()
> 	    -> avc_has_perm_noaudit()
> 	      -> avc_denied()
> 	        -> avc_update_node()
> 
> where we return early if AVC_NONBLOCKING is set, except flags are always
> zero on this path.

That was introduced by 3a28cff3bd4bf43f02be0c4e7933aebf3dc8197e 
("selinux: avoid silent denials in permissive mode under RCU walk") and 
is only needed if we have to pass MAY_NOT_BLOCK to slow_avc_audit(), 
which is only presently needed in the selinux_inode_permission() case 
AFAICT.  Both AVC_NONBLOCKING and MAY_NOT_BLOCK are misnomers wrt the 
AVC since it should never block regardless; the issue IIUC was rather 
the inability to safely collect the dentry name in an audit message 
during RCU walk per commit 0dc1ba24f7fff659725eecbba2c9ad679a0954cd (" 
SELINUX: Make selinux cache VFS RCU walks safe").

I'm not 100% certain about this; it is possible that the test in 
slow_avc_audit() is wrong and we ought to be doing this for any of 
LSM_AUDIT_DATA_PATH, _DENTRY, or _INODE (these were split out of 
LSM_AUDIT_DATA_FS).  In that case, we should revert my earlier commit 
for follow_link and fix the test inside of slow_avc_audit() instead.

I cc'd some additional folks who may have insight.  Al, tell us if we 
got it wrong please!

Re: [RFC PATCH 1/2] selinux: Don't call avc_compute_av() from RCU path walk

[Stephen Smalley]

On 11/20/19 8:12 AM, Will Deacon wrote:
> Hi Stephen,
> 
> Thanks for the quick reply.
> 
> On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
>> On 11/19/19 1:40 PM, Will Deacon wrote:
>>> 'avc_compute_av()' can block, so we carefully exit the RCU read-side
>>> critical section before calling it in 'avc_has_perm_noaudit()'.
>>> Unfortunately, if we're calling from the VFS layer on the RCU path walk
>>> via 'selinux_inode_permission()' then we're still actually in an RCU
>>> read-side critical section and must not block.
>>
>> avc_compute_av() should never block AFAIK. The blocking concern was with
>> slow_avc_audit(), and even that appears dubious to me. That seems to be more
>> about misuse of d_find_alias in dump_common_audit_data() than anything.
> 
> Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
> think it was propagated down to all of the potential allocations and
> string functions. Having looked at it again, I can't see where it blocks.
> 
> Might be worth a comment in avc_compute_av(), because the temporary
> dropping of rcu_read_lock() looks really dodgy when we could be running
> on the RCU path walk path anyway.

I don't think that's a problem but I'll defer to the fsdevel and rcu 
folks.  The use of RCU within the SELinux AVC long predates the 
introduction of RCU path walk, and the rcu_read_lock()/unlock() pairs 
inside the AVC are not related in any way to RCU path walk.  Hopefully 
they don't break it.  The SELinux security server (i.e. 
security_compute_av() and the rest of security/selinux/ss/*) internally 
has its own locking for its data structures, primarily the policy 
rwlock.  There was also a patch long ago to convert use of that policy 
rwlock to RCU but it didn't seem justified at the time.  We are 
interested in revisiting that however.  That would introduce its own set 
of rcu_read_lock/unlock pairs inside of security_compute_av() as well.

Re: [RFC PATCH 1/2] selinux: Don't call avc_compute_av() from RCU path walk

[Paul E. McKenney]

On Wed, Nov 20, 2019 at 10:28:31AM -0500, Stephen Smalley wrote:
> On 11/20/19 8:12 AM, Will Deacon wrote:
> > Hi Stephen,
> > 
> > Thanks for the quick reply.
> > 
> > On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
> > > On 11/19/19 1:40 PM, Will Deacon wrote:
> > > > 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> > > > critical section before calling it in 'avc_has_perm_noaudit()'.
> > > > Unfortunately, if we're calling from the VFS layer on the RCU path walk
> > > > via 'selinux_inode_permission()' then we're still actually in an RCU
> > > > read-side critical section and must not block.
> > > 
> > > avc_compute_av() should never block AFAIK. The blocking concern was with
> > > slow_avc_audit(), and even that appears dubious to me. That seems to be more
> > > about misuse of d_find_alias in dump_common_audit_data() than anything.
> > 
> > Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
> > think it was propagated down to all of the potential allocations and
> > string functions. Having looked at it again, I can't see where it blocks.
> > 
> > Might be worth a comment in avc_compute_av(), because the temporary
> > dropping of rcu_read_lock() looks really dodgy when we could be running
> > on the RCU path walk path anyway.
> 
> I don't think that's a problem but I'll defer to the fsdevel and rcu folks.
> The use of RCU within the SELinux AVC long predates the introduction of RCU
> path walk, and the rcu_read_lock()/unlock() pairs inside the AVC are not
> related in any way to RCU path walk.  Hopefully they don't break it.  The
> SELinux security server (i.e. security_compute_av() and the rest of
> security/selinux/ss/*) internally has its own locking for its data
> structures, primarily the policy rwlock.  There was also a patch long ago to
> convert use of that policy rwlock to RCU but it didn't seem justified at the
> time.  We are interested in revisiting that however.  That would introduce
> its own set of rcu_read_lock/unlock pairs inside of security_compute_av() as
> well.

RCU readers nest, so it should be fine.  (Famous last words...)

							Thanx, Paul

Re: [RFC PATCH 1/2] selinux: Don't call avc_compute_av() from RCU path walk

[Will Deacon]

On Wed, Nov 20, 2019 at 11:07:43AM -0800, Paul E. McKenney wrote:
> On Wed, Nov 20, 2019 at 10:28:31AM -0500, Stephen Smalley wrote:
> > On 11/20/19 8:12 AM, Will Deacon wrote:
> > > Hi Stephen,
> > > 
> > > Thanks for the quick reply.
> > > 
> > > On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
> > > > On 11/19/19 1:40 PM, Will Deacon wrote:
> > > > > 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> > > > > critical section before calling it in 'avc_has_perm_noaudit()'.
> > > > > Unfortunately, if we're calling from the VFS layer on the RCU path walk
> > > > > via 'selinux_inode_permission()' then we're still actually in an RCU
> > > > > read-side critical section and must not block.
> > > > 
> > > > avc_compute_av() should never block AFAIK. The blocking concern was with
> > > > slow_avc_audit(), and even that appears dubious to me. That seems to be more
> > > > about misuse of d_find_alias in dump_common_audit_data() than anything.
> > > 
> > > Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
> > > think it was propagated down to all of the potential allocations and
> > > string functions. Having looked at it again, I can't see where it blocks.
> > > 
> > > Might be worth a comment in avc_compute_av(), because the temporary
> > > dropping of rcu_read_lock() looks really dodgy when we could be running
> > > on the RCU path walk path anyway.
> > 
> > I don't think that's a problem but I'll defer to the fsdevel and rcu folks.
> > The use of RCU within the SELinux AVC long predates the introduction of RCU
> > path walk, and the rcu_read_lock()/unlock() pairs inside the AVC are not
> > related in any way to RCU path walk.  Hopefully they don't break it.  The
> > SELinux security server (i.e. security_compute_av() and the rest of
> > security/selinux/ss/*) internally has its own locking for its data
> > structures, primarily the policy rwlock.  There was also a patch long ago to
> > convert use of that policy rwlock to RCU but it didn't seem justified at the
> > time.  We are interested in revisiting that however.  That would introduce
> > its own set of rcu_read_lock/unlock pairs inside of security_compute_av() as
> > well.
> 
> RCU readers nest, so it should be fine.  (Famous last words...)

Agreed. It was blocking that worried me, and it turns out that doesn't
happen for this code.

Will