| Message ID | 20200826135906.1912186-3-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: RCU conversion follow-ups | expand |
On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > The policycap array in struct selinux_state is redundant and can be > substituted by calling security_policycap_supported(). > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 7cc2f7486c18f..e82a2cfe171f3 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2113,30 +2113,6 @@ bad: > return 0; > } > > -static void security_load_policycaps(struct selinux_state *state, > - struct selinux_policy *policy) > -{ > - struct policydb *p; > - unsigned int i; > - struct ebitmap_node *node; > - > - p = &policy->policydb; > - > - for (i = 0; i < ARRAY_SIZE(state->policycap); i++) > - state->policycap[i] = ebitmap_get_bit(&p->policycaps, i); > - > - for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++) > - pr_info("SELinux: policy capability %s=%d\n", > - selinux_policycap_names[i], > - ebitmap_get_bit(&p->policycaps, i)); > - > - ebitmap_for_each_positive_bit(&p->policycaps, node, i) { > - if (i >= ARRAY_SIZE(selinux_policycap_names)) > - pr_info("SELinux: unknown policy capability %u\n", > - i); > - } > -} > - Two requests: 1. Can you do a little benchmarking to confirm that calling security_policycap_supported() each time doesn't cause any significant overheads? Networking benchmark might be of interest. 2. Can you retain the logging of the policy capability values? Just drop the first part of the function and rename it e.g. security_log_policycaps().
On Wed, Aug 26, 2020 at 4:12 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > The policycap array in struct selinux_state is redundant and can be > > substituted by calling security_policycap_supported(). > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index 7cc2f7486c18f..e82a2cfe171f3 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -2113,30 +2113,6 @@ bad: > > return 0; > > } > > > > -static void security_load_policycaps(struct selinux_state *state, > > - struct selinux_policy *policy) > > -{ > > - struct policydb *p; > > - unsigned int i; > > - struct ebitmap_node *node; > > - > > - p = &policy->policydb; > > - > > - for (i = 0; i < ARRAY_SIZE(state->policycap); i++) > > - state->policycap[i] = ebitmap_get_bit(&p->policycaps, i); > > - > > - for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++) > > - pr_info("SELinux: policy capability %s=%d\n", > > - selinux_policycap_names[i], > > - ebitmap_get_bit(&p->policycaps, i)); > > - > > - ebitmap_for_each_positive_bit(&p->policycaps, node, i) { > > - if (i >= ARRAY_SIZE(selinux_policycap_names)) > > - pr_info("SELinux: unknown policy capability %u\n", > > - i); > > - } > > -} > > - > > Two requests: > 1. Can you do a little benchmarking to confirm that calling > security_policycap_supported() each time doesn't cause any significant > overheads? Networking benchmark might be of interest. I tried to sample a simple `ping -f -i 0 -c 5000000 127.0.0.1` with perf and indeed security_policycap_supported() now makes up about half the time spent in some hooks (selinux_socket_sock_rcv_skb(), selinux_ip_postroute()), mainly because of ebitmap_get_bit() it seems. I'll try moving the array to policydb and using it in security_policycap_supported() instead of the bitmap. > > 2. Can you retain the logging of the policy capability values? Just > drop the first part of the function and rename it e.g. > security_log_policycaps(). Sure, somehow I failed to notice those prints...
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 839774929a10d..9ab8f8da47812 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -96,7 +96,6 @@ struct selinux_state { #endif bool checkreqprot; bool initialized; - bool policycap[__POLICYDB_CAPABILITY_MAX]; struct page *status_page; struct mutex status_lock; @@ -159,53 +158,49 @@ static inline bool selinux_disabled(struct selinux_state *state) } #endif +int security_policycap_supported(struct selinux_state *state, + unsigned int req_cap); + static inline bool selinux_policycap_netpeer(void) { - struct selinux_state *state = &selinux_state; - - return state->policycap[POLICYDB_CAPABILITY_NETPEER]; + return security_policycap_supported(&selinux_state, + POLICYDB_CAPABILITY_NETPEER); } static inline bool selinux_policycap_openperm(void) { - struct selinux_state *state = &selinux_state; - - return state->policycap[POLICYDB_CAPABILITY_OPENPERM]; + return security_policycap_supported(&selinux_state, + POLICYDB_CAPABILITY_OPENPERM); } static inline bool selinux_policycap_extsockclass(void) { - struct selinux_state *state = &selinux_state; - - return state->policycap[POLICYDB_CAPABILITY_EXTSOCKCLASS]; + return security_policycap_supported(&selinux_state, + POLICYDB_CAPABILITY_EXTSOCKCLASS); } static inline bool selinux_policycap_alwaysnetwork(void) { - struct selinux_state *state = &selinux_state; - - return state->policycap[POLICYDB_CAPABILITY_ALWAYSNETWORK]; + return security_policycap_supported(&selinux_state, + POLICYDB_CAPABILITY_ALWAYSNETWORK); } static inline bool selinux_policycap_cgroupseclabel(void) { - struct selinux_state *state = &selinux_state; - - return state->policycap[POLICYDB_CAPABILITY_CGROUPSECLABEL]; + return security_policycap_supported(&selinux_state, + POLICYDB_CAPABILITY_CGROUPSECLABEL); } static inline bool selinux_policycap_nnp_nosuid_transition(void) { - struct selinux_state *state = &selinux_state; - - return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION]; + return security_policycap_supported(&selinux_state, + POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION); } static inline bool selinux_policycap_genfs_seclabel_symlinks(void) { - struct selinux_state *state = &selinux_state; - - return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]; + return security_policycap_supported(&selinux_state, + POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS); } int security_mls_enabled(struct selinux_state *state); @@ -219,9 +214,6 @@ void selinux_policy_cancel(struct selinux_state *state, int security_read_policy(struct selinux_state *state, void **data, size_t *len); -int security_policycap_supported(struct selinux_state *state, - unsigned int req_cap); - #define SEL_VEC_MAX 32 struct av_decision { u32 allowed; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 7cc2f7486c18f..e82a2cfe171f3 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2113,30 +2113,6 @@ bad: return 0; } -static void security_load_policycaps(struct selinux_state *state, - struct selinux_policy *policy) -{ - struct policydb *p; - unsigned int i; - struct ebitmap_node *node; - - p = &policy->policydb; - - for (i = 0; i < ARRAY_SIZE(state->policycap); i++) - state->policycap[i] = ebitmap_get_bit(&p->policycaps, i); - - for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++) - pr_info("SELinux: policy capability %s=%d\n", - selinux_policycap_names[i], - ebitmap_get_bit(&p->policycaps, i)); - - ebitmap_for_each_positive_bit(&p->policycaps, node, i) { - if (i >= ARRAY_SIZE(selinux_policycap_names)) - pr_info("SELinux: unknown policy capability %u\n", - i); - } -} - static int security_preserve_bools(struct selinux_policy *oldpolicy, struct selinux_policy *newpolicy); @@ -2218,9 +2194,6 @@ void selinux_policy_commit(struct selinux_state *state, /* Install the new policy. */ rcu_assign_pointer(state->policy, newpolicy); - /* Load the policycaps from the new policy */ - security_load_policycaps(state, newpolicy); - if (!selinux_initialized(state)) { /* * After first policy load, the security server is
The policycap array in struct selinux_state is redundant and can be substituted by calling security_policycap_supported(). Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/include/security.h | 42 ++++++++++++----------------- security/selinux/ss/services.c | 27 ------------------- 2 files changed, 17 insertions(+), 52 deletions(-)