On 9/7/20 12:47 AM, Ondrej Mosnacek wrote:
On Sat, Sep 5, 2020 at 11:33 PM Lakshmi Ramasubramanian
<nramas@xxxxxxxxxxxxxxxxxxx> wrote:
On 8/25/20 8:20 AM, Ondrej Mosnacek wrote:
Hi Ondrej,
I am just trying understand the expected behavior w.r.t the usage of
rcu_dereference_protected() for accessing SELinux policy. Could you
please clarify?
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.
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.
Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
---
security/selinux/ss/services.c | 286 ++++++++++++++++-----------------
security/selinux/ss/services.h | 6 +
2 files changed, 147 insertions(+), 145 deletions(-)
int security_read_policy(struct selinux_state *state,
void **data, size_t *len)
{
struct selinux_policy *policy;
policy = rcu_dereference_protected(
state->policy,
lockdep_is_held(&state->policy_mutex));
if (!policy)
return -EINVAL;
...
}
If "policy_mutex" is not held by the caller of security_read_policy() I
was expecting the above rcu_dereference_protected() call to return NULL,
No, that's not how rcu_dereference_protected() works. You should only
call it when you know for sure that you are in a section that is
mutually exclusive with anything that might change the pointer. The
check argument is only used for sanity checking that this is indeed
true, but other than triggering a warning when RCU debugging is
enabled the result of the check is ignored.
If the returned pointer is NULL, it just means that the pointer hasn't
been assigned yet, i.e. that no policy has been loaded yet (this was
checked using selinux_initialized() before, but when we're under
policy_mutex, checking the pointer for NULL is possible and simpler).
BTW, you should've replied to [1], which is the merged patch that
introduced the code you are referencing :)
Thanks for clarifying Ondrej.
-lakshmi
