| Message ID | 20230307204524.214983-1-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC] selinux: uninline unlikely parts of avc_has_perm_noaudit() | expand |
On Tue, Mar 7, 2023 at 3:45 PM Paul Moore <paul@paul-moore.com> wrote: > > This is based on earlier patch posted to the list by Linus, his > commit description read: > > "avc_has_perm_noaudit()is one of those hot functions that end up > being used by almost all filesystem operations (through > "avc_has_perm()") and it's intended to be cheap enough to inline. > > However, it turns out that the unlikely parts of it (where it > doesn't find an existing avc node) need a fair amount of stack > space for the automatic replacement node, so if it were to be > inlined (at least clang does not) it would just use stack space > unnecessarily. > > So split the unlikely part out of it, and mark that part noinline. > That improves the actual likely part." > > The basic idea behind the patch was reasonable, but there were minor > nits (double indenting, etc.) and the RCU read lock unlock/re-lock in > avc_compute_av() began to look even more ugly. This patch builds on > Linus' first effort by cleaning things up a bit and removing the RCU > unlock/lock dance in avc_compute_av(). > > Removing the RCU lock dance in avc_compute_av() is safe as there are > currently two callers of avc_compute_av(): avc_has_perm_noaudit() and > avc_has_extended_perms(). The first caller in avc_has_perm_noaudit() > does not require a RCU lock as there is no avc_node to protect so the > RCU lock can be dropped before calling avc_compute_av(). The second > caller, avc_has_extended_perms(), is similar in that there is no > avc_node that requires RCU protection, but the code is simplified by > holding the RCU look around the avc_compute_av() call, and given that > we enter a RCU critical section in security_compute_av() (called from > av_compute_av()) the impact will likely be unnoticeable. It is also > worth noting that avc_has_extended_perms() is only called from the > SELinux ioctl() access control hook at the moment. > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/avc.c | 85 ++++++++++++++++++++++++++++-------------- > 1 file changed, 57 insertions(+), 28 deletions(-) FYI, marked as a RFC as I haven't tested this yet but I felt this was worth posting early as Linus' original patch was sent as an attachment and some of you may have missed it as a result. https://lore.kernel.org/selinux/CAHk-=whR4KSGqfEodXMwOMdQBt+V2HHMyz6+CobiydnZE+Vq9Q@mail.gmail.com/
On Tue, Mar 7, 2023 at 12:45 PM Paul Moore <paul@paul-moore.com> wrote: > > This is based on earlier patch posted to the list by Linus Ack, looks fine to me. I didn't apply it and look at code generation, but I don't see any reason why it would be bad, and I agree with your RCU lock cleanup being probably the better option (rather than having the comment about the odd rcu lock behavior). Linus
On Tue, Mar 7, 2023 at 4:00 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Mar 7, 2023 at 12:45 PM Paul Moore <paul@paul-moore.com> wrote: > > > > This is based on earlier patch posted to the list by Linus > > Ack, looks fine to me. > > I didn't apply it and look at code generation, but I don't see any > reason why it would be bad, and I agree with your RCU lock cleanup > being probably the better option (rather than having the comment about > the odd rcu lock behavior). It seems to be passing all our tests so I've just merged it into selinux/next; assuming nothing strange pops up in the next several weeks you'll see this during the next merge window.
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 9a43af0ebd7d..5ce3ad451665 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -988,25 +988,26 @@ int avc_ss_reset(struct selinux_avc *avc, u32 seqno) return rc; } -/* - * Slow-path helper function for avc_has_perm_noaudit, - * when the avc_node lookup fails. We get called with - * the RCU read lock held, and need to return with it - * still held, but drop if for the security compute. +/** + * avc_compute_av - Add an entry to the AVC based on the security policy + * @state: SELinux state pointer + * @ssid: subject + * @tsid: object/target + * @tclass: object class + * @avd: access vector decision + * @xp_node: AVC extended permissions node * - * Don't inline this, since it's the slow-path and just - * results in a bigger stack frame. + * Slow-path helper function for avc_has_perm_noaudit, when the avc_node lookup + * fails. Don't inline this, since it's the slow-path and just results in a + * bigger stack frame. */ -static noinline -struct avc_node *avc_compute_av(struct selinux_state *state, - u32 ssid, u32 tsid, - u16 tclass, struct av_decision *avd, - struct avc_xperms_node *xp_node) +static noinline struct avc_node *avc_compute_av(struct selinux_state *state, + u32 ssid, u32 tsid, u16 tclass, + struct av_decision *avd, + struct avc_xperms_node *xp_node) { - rcu_read_unlock(); INIT_LIST_HEAD(&xp_node->xpd_head); security_compute_av(state, ssid, tsid, tclass, avd, &xp_node->xp); - rcu_read_lock(); return avc_insert(state->avc, ssid, tsid, tclass, avd, xp_node); } @@ -1112,6 +1113,36 @@ int avc_has_extended_perms(struct selinux_state *state, return rc; } +/** + * avc_perm_nonode - Add an entry to the AVC + * @state: SELinux state pointer + * @ssid: subject + * @tsid: object/target + * @tclass: object class + * @requested: requested permissions + * @flags: AVC flags + * @avd: access vector decision + * + * This is the "we have no node" part of avc_has_perm_noaudit(), which is + * unlikely and needs extra stack space for the new node that we generate, so + * don't inline it. + */ +static noinline int avc_perm_nonode(struct selinux_state *state, + u32 ssid, u32 tsid, u16 tclass, + u32 requested, unsigned int flags, + struct av_decision *avd) +{ + u32 denied; + struct avc_xperms_node xp_node; + + avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node); + denied = requested & ~(avd->allowed); + if (unlikely(denied)) + return avc_denied(state, ssid, tsid, tclass, requested, 0, 0, + flags, avd); + return 0; +} + /** * avc_has_perm_noaudit - Check permissions but perform no auditing. * @state: SELinux state @@ -1139,29 +1170,27 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, unsigned int flags, struct av_decision *avd) { - struct avc_node *node; - struct avc_xperms_node xp_node; - int rc = 0; u32 denied; + struct avc_node *node; if (WARN_ON(!requested)) return -EACCES; rcu_read_lock(); - node = avc_lookup(state->avc, ssid, tsid, tclass); - if (unlikely(!node)) - avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node); - else - memcpy(avd, &node->ae.avd, sizeof(*avd)); + if (unlikely(!node)) { + rcu_read_unlock(); + return avc_perm_nonode(state, ssid, tsid, tclass, requested, + flags, avd); + } + denied = requested & ~node->ae.avd.allowed; + memcpy(avd, &node->ae.avd, sizeof(*avd)); + rcu_read_unlock(); - denied = requested & ~(avd->allowed); if (unlikely(denied)) - rc = avc_denied(state, ssid, tsid, tclass, requested, 0, 0, - flags, avd); - - rcu_read_unlock(); - return rc; + return avc_denied(state, ssid, tsid, tclass, requested, 0, 0, + flags, avd); + return 0; } /**
This is based on earlier patch posted to the list by Linus, his commit description read: "avc_has_perm_noaudit()is one of those hot functions that end up being used by almost all filesystem operations (through "avc_has_perm()") and it's intended to be cheap enough to inline. However, it turns out that the unlikely parts of it (where it doesn't find an existing avc node) need a fair amount of stack space for the automatic replacement node, so if it were to be inlined (at least clang does not) it would just use stack space unnecessarily. So split the unlikely part out of it, and mark that part noinline. That improves the actual likely part." The basic idea behind the patch was reasonable, but there were minor nits (double indenting, etc.) and the RCU read lock unlock/re-lock in avc_compute_av() began to look even more ugly. This patch builds on Linus' first effort by cleaning things up a bit and removing the RCU unlock/lock dance in avc_compute_av(). Removing the RCU lock dance in avc_compute_av() is safe as there are currently two callers of avc_compute_av(): avc_has_perm_noaudit() and avc_has_extended_perms(). The first caller in avc_has_perm_noaudit() does not require a RCU lock as there is no avc_node to protect so the RCU lock can be dropped before calling avc_compute_av(). The second caller, avc_has_extended_perms(), is similar in that there is no avc_node that requires RCU protection, but the code is simplified by holding the RCU look around the avc_compute_av() call, and given that we enter a RCU critical section in security_compute_av() (called from av_compute_av()) the impact will likely be unnoticeable. It is also worth noting that avc_has_extended_perms() is only called from the SELinux ioctl() access control hook at the moment. Cc: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/avc.c | 85 ++++++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 28 deletions(-)