| Message ID | 20191119184057.14961-2-will@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Avoid blocking in selinux inode callbacks on RCU walk | expand |
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; > } >
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
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.
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
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
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; }
'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(-)