Re: [RFC PATCH 1/2] selinux: Don't call avc_compute_av() from RCU path walk
From: Stephen Smalley
Date: Wed Nov 20 2019 - 10:29:09 EST
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.