| Message ID | e9c1967d-170f-86f0-2762-7ca36ea08e40@sony.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RFC] selinux: Use call_rcu for policydb and booleans | expand |
On 8/19/20 4:32 AM, peter enderborg wrote: > From 8184ea3648b18718fdb460a30dfc7f848b7bc6a2 Mon Sep 17 00:00:00 2001 > From: Peter Enderborg <peter.enderborg@sony.com> > Date: Wed, 19 Aug 2020 10:20:28 +0200 > Subject: [RFC 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. If you look at the first version of my patch, I used call_rcu() but in a manner that avoided the need to remove cond_resched() or kvfree() calls from the freeing code by having the rcu callback just schedule_work() to free it later. That follows the pattern used for freeing user namespaces, for example. However, in re-reading the RCU documentation, my understanding is that one should use synchronize_rcu() followed by direct freeing whenever possible and this is possible from both the policy load and setting booleans. Neither of them are very frequent operations nor so performance-critical that the cost of synchronize_rcu() would be considered unacceptable IMHO. Thus, I don't believe we need to do this.
This will might even compile! :-) From f2d5b2a33c97fef896758becfe62e79aed96352d Mon Sep 17 00:00:00 2001 From: Peter Enderborg <peter.enderborg@sony.com> 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. Signed-off-by: Peter Enderborg <peter.enderborg@sony.com> --- 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);
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@sony.com> > 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@sony.com> > --- > 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);
On Wed, Aug 19, 2020 at 08:06:25AM -0400, Stephen Smalley wrote: > On 8/19/20 4:32 AM, peter enderborg wrote: > > > From 8184ea3648b18718fdb460a30dfc7f848b7bc6a2 Mon Sep 17 00:00:00 2001 > > From: Peter Enderborg <peter.enderborg@sony.com> > > Date: Wed, 19 Aug 2020 10:20:28 +0200 > > Subject: [RFC 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. > > If you look at the first version of my patch, I used call_rcu() but in a > manner that avoided the need to remove cond_resched() or kvfree() calls from > the freeing code by having the rcu callback just schedule_work() to free it > later. That follows the pattern used for freeing user namespaces, for > example. However, in re-reading the RCU documentation, my understanding is > that one should use synchronize_rcu() followed by direct freeing whenever > possible and this is possible from both the policy load and setting > booleans. Neither of them are very frequent operations nor so > performance-critical that the cost of synchronize_rcu() would be considered > unacceptable IMHO. Thus, I don't believe we need to do this. Indeed, synchronize_rcu() avoids things like callback flooding. There are nevertheless cases where synchronize_rcu() can be a problem, for example due to its relatively long latency (milliseconds at best). But if this is an infrequent operation, that should not be an issue. If there nevertheless is a reason to avoid synchronize_rcu(), then queue_rcu_work() gets you directly to a sleepable workqueue context where cond_resched() can be used. Thnax, Paul
On 8/19/20 2:06 PM, Stephen Smalley wrote: > On 8/19/20 4:32 AM, peter enderborg wrote: > >> From 8184ea3648b18718fdb460a30dfc7f848b7bc6a2 Mon Sep 17 00:00:00 2001 >> From: Peter Enderborg <peter.enderborg@sony.com> >> Date: Wed, 19 Aug 2020 10:20:28 +0200 >> Subject: [RFC 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. > > If you look at the first version of my patch, I used call_rcu() but in a manner that avoided the need to remove cond_resched() or kvfree() calls from the freeing code by having the rcu callback just schedule_work() to free it later. That follows the pattern used for freeing user namespaces, for example. However, in re-reading the RCU documentation, my understanding is that one should use synchronize_rcu() followed by direct freeing whenever possible and this is possible from both the policy load and setting booleans. Neither of them are very frequent operations nor so performance-critical that the cost of synchronize_rcu() would be considered unacceptable IMHO. Thus, I don't believe we need to do this. > > Loading policydb should be very rare, and it takes for ever anyway. Booleans I have no idea. It seems to something that turns on-off quickly and there are many of them so it will be hard to say. However I did a test. Before rcu a boolean cycle is (on my test rig) 15ms, with call_rcu 14ms and with synchronize_rcu 10ms. Not what I expected. And there are rcu versions for the kvfree if needed.
On 8/19/20 3:15 PM, Paul E. McKenney wrote: > On Wed, Aug 19, 2020 at 08:06:25AM -0400, Stephen Smalley wrote: >> On 8/19/20 4:32 AM, peter enderborg wrote: >> >>> From 8184ea3648b18718fdb460a30dfc7f848b7bc6a2 Mon Sep 17 00:00:00 2001 >>> From: Peter Enderborg <peter.enderborg@sony.com> >>> Date: Wed, 19 Aug 2020 10:20:28 +0200 >>> Subject: [RFC 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. >> If you look at the first version of my patch, I used call_rcu() but in a >> manner that avoided the need to remove cond_resched() or kvfree() calls from >> the freeing code by having the rcu callback just schedule_work() to free it >> later. That follows the pattern used for freeing user namespaces, for >> example. However, in re-reading the RCU documentation, my understanding is >> that one should use synchronize_rcu() followed by direct freeing whenever >> possible and this is possible from both the policy load and setting >> booleans. Neither of them are very frequent operations nor so >> performance-critical that the cost of synchronize_rcu() would be considered >> unacceptable IMHO. Thus, I don't believe we need to do this. > Indeed, synchronize_rcu() avoids things like callback flooding. > There are nevertheless cases where synchronize_rcu() can be a problem, > for example due to its relatively long latency (milliseconds at best). > But if this is an infrequent operation, that should not be an issue. > > If there nevertheless is a reason to avoid synchronize_rcu(), then > queue_rcu_work() gets you directly to a sleepable workqueue context > where cond_resched() can be used. > > Thnax, Paul From what I have seen in usage of booleans the usage is switch-state, do-something, switch-back. So if there is anything security related you want switch time to be fast. But when I measured it was faster with synchronize_rcu than call_rcu. booleans work are smaller than a total policy load.
On Wed, Aug 19, 2020 at 05:05:28PM +0200, peter enderborg wrote: > On 8/19/20 3:15 PM, Paul E. McKenney wrote: > > On Wed, Aug 19, 2020 at 08:06:25AM -0400, Stephen Smalley wrote: > >> On 8/19/20 4:32 AM, peter enderborg wrote: > >> > >>> From 8184ea3648b18718fdb460a30dfc7f848b7bc6a2 Mon Sep 17 00:00:00 2001 > >>> From: Peter Enderborg <peter.enderborg@sony.com> > >>> Date: Wed, 19 Aug 2020 10:20:28 +0200 > >>> Subject: [RFC 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. > >> If you look at the first version of my patch, I used call_rcu() but in a > >> manner that avoided the need to remove cond_resched() or kvfree() calls from > >> the freeing code by having the rcu callback just schedule_work() to free it > >> later. That follows the pattern used for freeing user namespaces, for > >> example. However, in re-reading the RCU documentation, my understanding is > >> that one should use synchronize_rcu() followed by direct freeing whenever > >> possible and this is possible from both the policy load and setting > >> booleans. Neither of them are very frequent operations nor so > >> performance-critical that the cost of synchronize_rcu() would be considered > >> unacceptable IMHO. Thus, I don't believe we need to do this. > > Indeed, synchronize_rcu() avoids things like callback flooding. > > There are nevertheless cases where synchronize_rcu() can be a problem, > > for example due to its relatively long latency (milliseconds at best). > > But if this is an infrequent operation, that should not be an issue. > > > > If there nevertheless is a reason to avoid synchronize_rcu(), then > > queue_rcu_work() gets you directly to a sleepable workqueue context > > where cond_resched() can be used. > > From what I have seen in usage of booleans the usage is > switch-state, do-something, switch-back. So if there is anything > security related you want switch time to be fast. But when I measured > it was faster with synchronize_rcu than call_rcu. booleans work are > smaller than a total policy load. That sounds to me like a good argument for synchronize_rcu(), but I at the end of the day, I must defer to you guys given your knowledge of this code. Thanx, Paul
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..11eff3a98ef8 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 dep_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);