Re: [RFC PATCH] selinux: Use call_rcu for policydb and booleans

[Stephen Smalley <stephen.smalley.work@xxxxxxxxx>]

On 8/19/20 8:06 AM, peter enderborg wrote:

> This will might even compile! :-)
>
>  From f2d5b2a33c97fef896758becfe62e79aed96352d Mon Sep 17 00:00:00 2001
> From: Peter Enderborg <peter.enderborg@xxxxxxxx>
> Date: Wed, 19 Aug 2020 10:20:28 +0200
> Subject: [PATCH] selinux: Use call_rcu for policydb and booleans
>
> This patch adds call_rcu that moves sycronize out
>
> out call path. In the callback we can no call
> cond_resched so they have to be remvoed.
I don't see why this is necessary.  My v1 patch used call_rcu() without 
needing these changes, but I didn't think using call_rcu() was justified 
and switched to synchronize_rcu().  What is lacking in my v2 patch that 
you are trying to fix?  If it is a performance concern around calling 
synchronize_rcu() during security_load_policy() and security_set_bool(), 
I don't think that is significant - those are infrequent operations.
> Signed-off-by: Peter Enderborg <peter.enderborg@xxxxxxxx>
> ---
>   security/selinux/ss/policydb.c |  6 -----
>   security/selinux/ss/services.c | 43 ++++++++++++++++++++++++++++++----
>   2 files changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 9fccf417006b..bcf49da4d7b2 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -341,7 +341,6 @@ static int filenametr_destroy(void *key, void *datum, void *p)
>                  kfree(d);
>                  d = next;
>          } while (unlikely(d));
> -       cond_resched();
>          return 0;
>   }
>   
> @@ -353,7 +352,6 @@ static int range_tr_destroy(void *key, void *datum, void *p)
>          ebitmap_destroy(&rt->level[0].cat);
>          ebitmap_destroy(&rt->level[1].cat);
>          kfree(datum);
> -       cond_resched();
>          return 0;
>   }
>   
> @@ -791,7 +789,6 @@ void policydb_destroy(struct policydb *p)
>          struct role_allow *ra, *lra = NULL;
>   
>          for (i = 0; i < SYM_NUM; i++) {
> -               cond_resched();
>                  hashtab_map(&p->symtab[i].table, destroy_f[i], NULL);
>                  hashtab_destroy(&p->symtab[i].table);
>          }
> @@ -807,7 +804,6 @@ void policydb_destroy(struct policydb *p)
>          avtab_destroy(&p->te_avtab);
>   
>          for (i = 0; i < OCON_NUM; i++) {
> -               cond_resched();
>                  c = p->ocontexts[i];
>                  while (c) {
>                          ctmp = c;
> @@ -819,7 +815,6 @@ void policydb_destroy(struct policydb *p)
>   
>          g = p->genfs;
>          while (g) {
> -               cond_resched();
>                  kfree(g->fstype);
>                  c = g->head;
>                  while (c) {
> @@ -839,7 +834,6 @@ void policydb_destroy(struct policydb *p)
>          hashtab_destroy(&p->role_tr);
>   
>          for (ra = p->role_allow; ra; ra = ra->next) {
> -               cond_resched();
>                  kfree(lra);
>                  lra = ra;
>          }
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index ba9347517e5b..61e8296908df 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2184,11 +2184,29 @@ static void selinux_notify_policy_change(struct selinux_state *state,
>          selinux_xfrm_notify_policyload();
>   }
>   
> +struct deprecated_policy {
> +       struct selinux_policy *policy;
> +       int partial;
> +       struct rcu_head rcu;
> +};
> +
> +void policy_reclaim(struct rcu_head *rp)
> +{
> +       struct deprecated_policy *dep = container_of(rp, struct deprecated_policy, rcu);
> +
> +       if (dep->partial)
> +               selinux_policy_cond_free(dep->policy);
> +       else
> +               selinux_policy_free(dep->policy);
> +       kfree(dep);
> +}
> +
>   void selinux_policy_commit(struct selinux_state *state,
>                          struct selinux_policy *newpolicy)
>   {
>          struct selinux_policy *oldpolicy;
>          u32 seqno;
> +       struct deprecated_policy *dep;
>   
>          /*
>           * NOTE: We do not need to take the rcu read lock
> @@ -2231,8 +2249,16 @@ void selinux_policy_commit(struct selinux_state *state,
>          }
>   
>          /* Free the old policy */
> -       synchronize_rcu();
> -       selinux_policy_free(oldpolicy);
> +       /* if cant alloc we need to it the slow way */
> +       dep  = kzalloc(sizeof(struct deprecated_policy), GFP_KERNEL);
> +       if (dep) {
> +               dep->policy = oldpolicy;
> +               dep->partial = 0;
> +               call_rcu(&dep->rcu, policy_reclaim);
> +       } else {
> +               synchronize_rcu();
> +               selinux_policy_free(oldpolicy);
> +       }
>   
>          /* Notify others of the policy change */
>          selinux_notify_policy_change(state, seqno);
> @@ -2956,6 +2982,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values)
>          struct selinux_policy *newpolicy, *oldpolicy;
>          int rc;
>          u32 i, seqno = 0;
> +       struct deprecated_policy *dep;
>   
>          if (!selinux_initialized(state))
>                  return -EINVAL;
> @@ -3020,8 +3047,16 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values)
>           * that were copied for the new policy, and the oldpolicy
>           * structure itself but not what it references.
>           */
> -       synchronize_rcu();
> -       selinux_policy_cond_free(oldpolicy);
> +       /* if we can not alloc do it the slow way */
> +       dep  = kzalloc(sizeof(struct deprecated_policy), GFP_KERNEL);
> +       if (dep) {
> +               dep->policy = oldpolicy;
> +               dep->partial = 1;
> +               call_rcu(&dep->rcu, policy_reclaim);
> +       } else {
> +               synchronize_rcu();
> +               selinux_policy_cond_free(oldpolicy);
> +       }
>   
>          /* Notify others of the policy change */
>          selinux_notify_policy_change(state, seqno);