On Thu, Aug 20, 2020 at 10:27 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > 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. You and Stephen have obviously spent more time looking at this than I have, but looking it over this morning it seems reasonable. > As for rcu_dereference_check(), I'd prefer to pass the fsi->mutex in > there for better clarity. I still can't say I'm really loving the write lock side of this, but short of breaking the boundary between the security server and the Linux/LSM hook side of the code I'm not sure there is a really good solution. I think simply commenting it as Stephen has done is about the best we can do. It is my opinion that passing an arbitrary mutex into the security server code doesn't really accomplish much. Sure, it gives the caller some indication that exclusivity is required, but a bare mutex doesn't really provide any guarantee that the *right* mutex is being used. If we wanted to fix that we would need to look at passing down the selinux_fs_info pointer itself, but then we are back to breaking the abstraction between the security server and the Linux hook layer. The only thing saving us here is that this code is *deep* inside SELinux and it is *extremely* unlikely that we are going to have any cross-subsystem abuse which touches this code. The comment based approach isn't perfect, but I think it is our best option at this point in time. > > 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. I'll differ :) I realize multiple assignments look pretty cool in that hey-I-can-combine-three-lines-of-code-into-one sorta way, but I hate having to read code like that, especially when you get to that point where you end up reading more code than you write. Simple code is easier to read, easier to review, easier to debug, and easier to maintain. ... and just to be clear, combining multiple assignments on a single line in combination with a ternary statement is not my idea of "simple" ;) > 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> Merged into selinux/next, thanks everyone. -- paul moore www.paul-moore.com
