On Tue, Aug 25, 2020 at 6:47 PM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > On Tue, Aug 25, 2020 at 11:20 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > > Instead of holding the RCU read lock the whole time while accessing the > > policy, add a simple refcount mechanism to track its lifetime. After > > this, the RCU read lock is held only for a brief time when fetching the > > policy pointer and incrementing the refcount. The policy struct is then > > guaranteed to stay alive until the refcount is decremented. > > > > Freeing of the policy remains the responsibility of the task that does > > the policy reload. In case the refcount drops to zero in a different > > task, the policy load task is notified via a completion. > > That's an interesting pattern. Is this approach used anywhere else in > the kernel? I didn't see any examples of it in the RCU documentation. If you mean RCU + reference counting, that's actually mentioned in RCU documentation in quite a few places as an option, e.g. [1] or [2]. As for the completion, I'm not aware if it's been used like this yet, but it seems to fit the purpose nicely. At least I hope there are no hidden gotchas, but I couldn't think of any. I know it from the crypto subsystem, where it's often used to wait for the result of an asynchronous operation. [1] https://www.kernel.org/doc/html/latest/RCU/whatisRCU.html#rcu-read-lock [2] https://www.kernel.org/doc/html/latest/core-api/kernel-api.html?highlight=long_lived#c.rcu_pointer_handoff > > > The advantage of this change is that the operations that access the > > policy can now do sleeping allocations, since they don't need to hold > > the RCU read lock anymore. This patch so far only leverages this in > > security_read_policy() for the vmalloc_user() allocation (although this > > function is always called under fsi->mutex and could just access the > > policy pointer directly). The conversion of affected GFP_ATOMIC > > allocations to GFP_KERNEL is left for a later patch, since auditing > > which code paths may still need GFP_ATOMIC is not very easy. > > Technically we don't need this patch for that purpose because > rcu_read_lock() isn't actually needed at all in > security_read_policy(), so I think we're better off just getting rid > of it there and letting it use rcu_dereference_check(..., 1) or > rcu_dereference_protected() instead. Yes, I'll address that in the next revision. -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
