On Wed, Aug 19, 2020 at 9:45 PM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > Convert the policy read-write lock to RCU. This is significantly > simplified by the earlier work to encapsulate the policy data > structures and refactor the policy load and boolean setting logic. > Move the latest_granting sequence number into the selinux_policy > structure so that it can be updated atomically with the policy. > Since removing the policy rwlock and moving latest_granting reduces > the selinux_ss structure to nothing more than a wrapper around the > selinux_policy pointer, get rid of the extra layer of indirection. > > At present this change merely passes a hardcoded 1 to > rcu_dereference_check() in the cases where we know we do not need to > take rcu_read_lock(), with the preceding comment explaining why. > Alternatively we could pass fsi->mutex down from selinuxfs and > apply a lockdep check on it instead. > > Based in part on earlier attempts to convert the policy rwlock > to RCU by Kaigai Kohei [1] and by Peter Enderborg [2]. > > [1] https://lore.kernel.org/selinux/6e2f9128-e191-ebb3-0e87-74bfccb0767f@xxxxxxxxxxxxx/ > [2] https://lore.kernel.org/selinux/20180530141104.28569-1-peter.enderborg@xxxxxxxx/ > > Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> > --- (from v3 comment section:) > Remaining open questions include whether I should change selinuxfs > to pass down fsi->mutex so that we can use it in a lockdep check > for rcu_dereference_check() and whether the sidtab live convert is > safe after this change. FTR, I spent some time pondering on whether there is any bad interaction with the sidtab live convert and I couldn't find anything. The tricky part was splitting the policy load into load/commit/cancel and that seems to have been done correctly. As for rcu_dereference_check(), I'd prefer to pass the fsi->mutex in there for better clarity. For example, if someone were to call one of the functions containing it, they may not realize that some mutual exclusion is expected around them. If the functions take the mutex as an argument, they at least have to stop and think what to pass in, which should eventually lead them down the right path. > v4 fixes the __rcu annotation so that sparse doesn't complain about > services.c and removes a few extraneous empty lines. > > security/selinux/hooks.c | 1 - > security/selinux/include/security.h | 5 +- > security/selinux/ss/services.c | 487 ++++++++++++++++------------ > security/selinux/ss/services.h | 5 - > 4 files changed, 280 insertions(+), 218 deletions(-) > [...] > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index a48fc1b337ba..838161462756 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c [...] > @@ -2174,14 +2208,18 @@ void selinux_policy_commit(struct selinux_state *state, > pr_info("SELinux: Enabling MLS support...\n"); > } > > + /* Set latest granting seqno for new policy. */ > + if (oldpolicy) > + newpolicy->latest_granting = oldpolicy->latest_granting + 1; > + else > + newpolicy->latest_granting = 1; > + seqno = newpolicy->latest_granting; This could be written as: seqno = newpolicy->latest_granting = oldpolicy ? oldpolicy->latest_granting + 1 : 1; ...which is a bit easier to read to me, but others may differ. > + > /* Install the new policy. */ > - write_lock_irq(&state->ss->policy_rwlock); > - state->ss->policy = newpolicy; > - seqno = ++state->ss->latest_granting; > - write_unlock_irq(&state->ss->policy_rwlock); > + rcu_assign_pointer(state->policy, newpolicy); > > /* Load the policycaps from the new policy */ > - security_load_policycaps(state); > + security_load_policycaps(state, newpolicy); > > if (!selinux_initialized(state)) { > /* [...] > @@ -2997,20 +3049,20 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values) > /* Re-evaluate the conditional rules in the copy */ > evaluate_cond_nodes(&newpolicy->policydb); > > + /* Set latest granting seqno for new policy */ > + newpolicy->latest_granting = oldpolicy->latest_granting + 1; > + seqno = newpolicy->latest_granting; These could also be merged into one line. > + > /* Install the new policy */ > - write_lock_irq(&state->ss->policy_rwlock); > - state->ss->policy = newpolicy; > - seqno = ++state->ss->latest_granting; > - write_unlock_irq(&state->ss->policy_rwlock); > + rcu_assign_pointer(state->policy, newpolicy); > [...] Other than the minor things above, I didn't find any logical issue in this patch. Since the mutex passing is going to be discussed in a separate patch and the only other comments I had are minor nits (and which may not align with your and/or Paul's preference), here you go: Reviewed-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
