* [PATCH 1/6] selinux: do not allocate ancillary buffer on first load
2020-01-16 12:04 [PATCH 0/6] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
@ 2020-01-16 12:04 ` Ondrej Mosnacek
2020-01-16 16:02 ` Stephen Smalley
2020-01-16 16:34 ` Stephen Smalley
2020-01-16 12:04 ` [PATCH 2/6] selinux: simplify security_preserve_bools() Ondrej Mosnacek
` (5 subsequent siblings)
6 siblings, 2 replies; 21+ messages in thread
From: Ondrej Mosnacek @ 2020-01-16 12:04 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
In security_load_policy(), we can defer allocating the newpolicydb
ancillary array to after checking state->initialized, thereby avoiding
the pointless allocation when loading policy the first time.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/services.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 55cf42945cba..42ca9f6dbbf4 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2183,26 +2183,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
int rc = 0;
struct policy_file file = { data, len }, *fp = &file;
- oldpolicydb = kcalloc(2, sizeof(*oldpolicydb), GFP_KERNEL);
- if (!oldpolicydb) {
- rc = -ENOMEM;
- goto out;
- }
- newpolicydb = oldpolicydb + 1;
-
policydb = &state->ss->policydb;
newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL);
- if (!newsidtab) {
- rc = -ENOMEM;
- goto out;
- }
+ if (!newsidtab)
+ return -ENOMEM;
if (!state->initialized) {
rc = policydb_read(policydb, fp);
if (rc) {
kfree(newsidtab);
- goto out;
+ return rc;
}
policydb->len = len;
@@ -2211,14 +2202,14 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
if (rc) {
kfree(newsidtab);
policydb_destroy(policydb);
- goto out;
+ return rc;
}
rc = policydb_load_isids(policydb, newsidtab);
if (rc) {
kfree(newsidtab);
policydb_destroy(policydb);
- goto out;
+ return rc;
}
state->ss->sidtab = newsidtab;
@@ -2231,9 +2222,16 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
selinux_status_update_policyload(state, seqno);
selinux_netlbl_cache_invalidate();
selinux_xfrm_notify_policyload();
- goto out;
+ return 0;
}
+ oldpolicydb = kcalloc(2, sizeof(*oldpolicydb), GFP_KERNEL);
+ if (!oldpolicydb) {
+ kfree(newsidtab);
+ return -ENOMEM;
+ }
+ newpolicydb = oldpolicydb + 1;
+
rc = policydb_read(newpolicydb, fp);
if (rc) {
kfree(newsidtab);
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/6] selinux: do not allocate ancillary buffer on first load
2020-01-16 12:04 ` [PATCH 1/6] selinux: do not allocate ancillary buffer on first load Ondrej Mosnacek
@ 2020-01-16 16:02 ` Stephen Smalley
2020-01-16 16:18 ` Ondrej Mosnacek
2020-01-16 16:34 ` Stephen Smalley
1 sibling, 1 reply; 21+ messages in thread
From: Stephen Smalley @ 2020-01-16 16:02 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> In security_load_policy(), we can defer allocating the newpolicydb
> ancillary array to after checking state->initialized, thereby avoiding
> the pointless allocation when loading policy the first time.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
What are these relative to, because they don't apply for me on
selinux/next? In particular they conflict with your 'treat atomic flags
more carefully' patch.
> ---
> security/selinux/ss/services.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 55cf42945cba..42ca9f6dbbf4 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2183,26 +2183,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> int rc = 0;
> struct policy_file file = { data, len }, *fp = &file;
>
> - oldpolicydb = kcalloc(2, sizeof(*oldpolicydb), GFP_KERNEL);
> - if (!oldpolicydb) {
> - rc = -ENOMEM;
> - goto out;
> - }
> - newpolicydb = oldpolicydb + 1;
> -
> policydb = &state->ss->policydb;
>
> newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL);
> - if (!newsidtab) {
> - rc = -ENOMEM;
> - goto out;
> - }
> + if (!newsidtab)
> + return -ENOMEM;
>
> if (!state->initialized) {
> rc = policydb_read(policydb, fp);
> if (rc) {
> kfree(newsidtab);
> - goto out;
> + return rc;
> }
>
> policydb->len = len;
> @@ -2211,14 +2202,14 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> if (rc) {
> kfree(newsidtab);
> policydb_destroy(policydb);
> - goto out;
> + return rc;
> }
>
> rc = policydb_load_isids(policydb, newsidtab);
> if (rc) {
> kfree(newsidtab);
> policydb_destroy(policydb);
> - goto out;
> + return rc;
> }
>
> state->ss->sidtab = newsidtab;
> @@ -2231,9 +2222,16 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> selinux_status_update_policyload(state, seqno);
> selinux_netlbl_cache_invalidate();
> selinux_xfrm_notify_policyload();
> - goto out;
> + return 0;
> }
>
> + oldpolicydb = kcalloc(2, sizeof(*oldpolicydb), GFP_KERNEL);
> + if (!oldpolicydb) {
> + kfree(newsidtab);
> + return -ENOMEM;
> + }
> + newpolicydb = oldpolicydb + 1;
> +
> rc = policydb_read(newpolicydb, fp);
> if (rc) {
> kfree(newsidtab);
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/6] selinux: do not allocate ancillary buffer on first load
2020-01-16 16:02 ` Stephen Smalley
@ 2020-01-16 16:18 ` Ondrej Mosnacek
2020-01-16 21:57 ` Paul Moore
0 siblings, 1 reply; 21+ messages in thread
From: Ondrej Mosnacek @ 2020-01-16 16:18 UTC (permalink / raw)
To: Stephen Smalley; +Cc: SElinux list, Paul Moore
On Thu, Jan 16, 2020 at 5:02 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> > In security_load_policy(), we can defer allocating the newpolicydb
> > ancillary array to after checking state->initialized, thereby avoiding
> > the pointless allocation when loading policy the first time.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> What are these relative to, because they don't apply for me on
> selinux/next? In particular they conflict with your 'treat atomic flags
> more carefully' patch.
Ah, I forgot to pull latest selinux/next before posting... They should
apply cleanly on top of d41415eb5eda ("Documentation,selinux: fix
references to old selinuxfs mount point"), but they auto-merged
cleanly when git-rebased on top of current selinux/next.
Paul, should I repost the patches or is it OK for you to apply on top
of d41415eb5eda and rebase?
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 1/6] selinux: do not allocate ancillary buffer on first load
2020-01-16 16:18 ` Ondrej Mosnacek
@ 2020-01-16 21:57 ` Paul Moore
0 siblings, 0 replies; 21+ messages in thread
From: Paul Moore @ 2020-01-16 21:57 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: Stephen Smalley, SElinux list
On Thu, Jan 16, 2020 at 11:18 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Jan 16, 2020 at 5:02 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> > > In security_load_policy(), we can defer allocating the newpolicydb
> > > ancillary array to after checking state->initialized, thereby avoiding
> > > the pointless allocation when loading policy the first time.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >
> > What are these relative to, because they don't apply for me on
> > selinux/next? In particular they conflict with your 'treat atomic flags
> > more carefully' patch.
>
> Ah, I forgot to pull latest selinux/next before posting... They should
> apply cleanly on top of d41415eb5eda ("Documentation,selinux: fix
> references to old selinuxfs mount point"), but they auto-merged
> cleanly when git-rebased on top of current selinux/next.
>
> Paul, should I repost the patches or is it OK for you to apply on top
> of d41415eb5eda and rebase?
I went ahead and applied 1/6 into selinux/next, but I want to look at
patch 2/6 a bit closer before applying.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] selinux: do not allocate ancillary buffer on first load
2020-01-16 12:04 ` [PATCH 1/6] selinux: do not allocate ancillary buffer on first load Ondrej Mosnacek
2020-01-16 16:02 ` Stephen Smalley
@ 2020-01-16 16:34 ` Stephen Smalley
1 sibling, 0 replies; 21+ messages in thread
From: Stephen Smalley @ 2020-01-16 16:34 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> In security_load_policy(), we can defer allocating the newpolicydb
> ancillary array to after checking state->initialized, thereby avoiding
> the pointless allocation when loading policy the first time.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/services.c | 28 +++++++++++++---------------
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 55cf42945cba..42ca9f6dbbf4 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2183,26 +2183,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> int rc = 0;
> struct policy_file file = { data, len }, *fp = &file;
>
> - oldpolicydb = kcalloc(2, sizeof(*oldpolicydb), GFP_KERNEL);
> - if (!oldpolicydb) {
> - rc = -ENOMEM;
> - goto out;
> - }
> - newpolicydb = oldpolicydb + 1;
> -
> policydb = &state->ss->policydb;
>
> newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL);
> - if (!newsidtab) {
> - rc = -ENOMEM;
> - goto out;
> - }
> + if (!newsidtab)
> + return -ENOMEM;
>
> if (!state->initialized) {
> rc = policydb_read(policydb, fp);
> if (rc) {
> kfree(newsidtab);
> - goto out;
> + return rc;
> }
>
> policydb->len = len;
> @@ -2211,14 +2202,14 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> if (rc) {
> kfree(newsidtab);
> policydb_destroy(policydb);
> - goto out;
> + return rc;
> }
>
> rc = policydb_load_isids(policydb, newsidtab);
> if (rc) {
> kfree(newsidtab);
> policydb_destroy(policydb);
> - goto out;
> + return rc;
> }
>
> state->ss->sidtab = newsidtab;
> @@ -2231,9 +2222,16 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> selinux_status_update_policyload(state, seqno);
> selinux_netlbl_cache_invalidate();
> selinux_xfrm_notify_policyload();
> - goto out;
> + return 0;
> }
>
> + oldpolicydb = kcalloc(2, sizeof(*oldpolicydb), GFP_KERNEL);
> + if (!oldpolicydb) {
> + kfree(newsidtab);
> + return -ENOMEM;
> + }
> + newpolicydb = oldpolicydb + 1;
> +
> rc = policydb_read(newpolicydb, fp);
> if (rc) {
> kfree(newsidtab);
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/6] selinux: simplify security_preserve_bools()
2020-01-16 12:04 [PATCH 0/6] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
2020-01-16 12:04 ` [PATCH 1/6] selinux: do not allocate ancillary buffer on first load Ondrej Mosnacek
@ 2020-01-16 12:04 ` Ondrej Mosnacek
2020-01-16 16:42 ` Stephen Smalley
2020-01-16 12:04 ` [PATCH 3/6] selinux: convert cond_list to array Ondrej Mosnacek
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Ondrej Mosnacek @ 2020-01-16 12:04 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
First, evaluate_cond_node() never returns an error. Make it just return
void.
Second, drop the use of security_get_bools() from
security_preserve_bools() and read from the old policydb directly. This
saves some useless allocations and together with the first change makes
security_preserve_bools() no longer possibly return an error. Again the
return type is changed to void.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/conditional.c | 3 +-
security/selinux/ss/conditional.h | 2 +-
security/selinux/ss/services.c | 52 ++++++++++---------------------
3 files changed, 18 insertions(+), 39 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 70c378ee1a2f..04593062008d 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -85,7 +85,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
* list appropriately. If the result of the expression is undefined
* all of the rules are disabled for safety.
*/
-int evaluate_cond_node(struct policydb *p, struct cond_node *node)
+void evaluate_cond_node(struct policydb *p, struct cond_node *node)
{
int new_state;
struct cond_av_list *cur;
@@ -111,7 +111,6 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node)
cur->node->key.specified |= AVTAB_ENABLED;
}
}
- return 0;
}
int cond_policydb_init(struct policydb *p)
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index ec846e45904c..d86ef286ca84 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -75,6 +75,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
struct av_decision *avd, struct extended_perms *xperms);
void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key,
struct extended_perms_decision *xpermd);
-int evaluate_cond_node(struct policydb *p, struct cond_node *node);
+void evaluate_cond_node(struct policydb *p, struct cond_node *node);
#endif /* _CONDITIONAL_H_ */
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 42ca9f6dbbf4..b9eda7d89e22 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2157,8 +2157,8 @@ static void security_load_policycaps(struct selinux_state *state)
}
}
-static int security_preserve_bools(struct selinux_state *state,
- struct policydb *newpolicydb);
+static void security_preserve_bools(struct policydb *oldpolicydb,
+ struct policydb *newpolicydb);
/**
* security_load_policy - Load a security policy configuration.
@@ -2257,11 +2257,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
if (rc)
goto err;
- rc = security_preserve_bools(state, newpolicydb);
- if (rc) {
- pr_err("SELinux: unable to preserve booleans\n");
- goto err;
- }
+ security_preserve_bools(policydb, newpolicydb);
oldsidtab = state->ss->sidtab;
@@ -2958,11 +2954,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
policydb->bool_val_to_struct[i]->state = 0;
}
- for (cur = policydb->cond_list; cur; cur = cur->next) {
- rc = evaluate_cond_node(policydb, cur);
- if (rc)
- goto out;
- }
+ for (cur = policydb->cond_list; cur; cur = cur->next)
+ evaluate_cond_node(policydb, cur);
seqno = ++state->ss->latest_granting;
rc = 0;
@@ -2999,36 +2992,23 @@ out:
return rc;
}
-static int security_preserve_bools(struct selinux_state *state,
- struct policydb *policydb)
+static void security_preserve_bools(struct policydb *oldpolicydb,
+ struct policydb *newpolicydb)
{
- int rc, nbools = 0, *bvalues = NULL, i;
- char **bnames = NULL;
struct cond_bool_datum *booldatum;
struct cond_node *cur;
+ int i;
- rc = security_get_bools(state, &nbools, &bnames, &bvalues);
- if (rc)
- goto out;
- for (i = 0; i < nbools; i++) {
- booldatum = hashtab_search(policydb->p_bools.table, bnames[i]);
- if (booldatum)
- booldatum->state = bvalues[i];
- }
- for (cur = policydb->cond_list; cur; cur = cur->next) {
- rc = evaluate_cond_node(policydb, cur);
- if (rc)
- goto out;
- }
+ for (i = 0; i < oldpolicydb->p_bools.nprim; i++) {
+ const char *name = sym_name(oldpolicydb, SYM_BOOLS, i);
+ int value = oldpolicydb->bool_val_to_struct[i]->state;
-out:
- if (bnames) {
- for (i = 0; i < nbools; i++)
- kfree(bnames[i]);
+ booldatum = hashtab_search(newpolicydb->p_bools.table, name);
+ if (booldatum)
+ booldatum->state = value;
}
- kfree(bnames);
- kfree(bvalues);
- return rc;
+ for (cur = newpolicydb->cond_list; cur; cur = cur->next)
+ evaluate_cond_node(newpolicydb, cur);
}
/*
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] selinux: simplify security_preserve_bools()
2020-01-16 12:04 ` [PATCH 2/6] selinux: simplify security_preserve_bools() Ondrej Mosnacek
@ 2020-01-16 16:42 ` Stephen Smalley
2020-01-16 22:28 ` Paul Moore
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Smalley @ 2020-01-16 16:42 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> First, evaluate_cond_node() never returns an error. Make it just return
> void.
>
> Second, drop the use of security_get_bools() from
> security_preserve_bools() and read from the old policydb directly. This
> saves some useless allocations and together with the first change makes
> security_preserve_bools() no longer possibly return an error. Again the
> return type is changed to void.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Dropping use of security_get_bools() means we are no longer reading the
boolean values with the policy read-lock held so they could in theory
change underneath us. However, this is presently prevented via the
fsi->mutex taken by selinuxfs so I believe this is safe.
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/conditional.c | 3 +-
> security/selinux/ss/conditional.h | 2 +-
> security/selinux/ss/services.c | 52 ++++++++++---------------------
> 3 files changed, 18 insertions(+), 39 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 70c378ee1a2f..04593062008d 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -85,7 +85,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
> * list appropriately. If the result of the expression is undefined
> * all of the rules are disabled for safety.
> */
> -int evaluate_cond_node(struct policydb *p, struct cond_node *node)
> +void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> {
> int new_state;
> struct cond_av_list *cur;
> @@ -111,7 +111,6 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node)
> cur->node->key.specified |= AVTAB_ENABLED;
> }
> }
> - return 0;
> }
>
> int cond_policydb_init(struct policydb *p)
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index ec846e45904c..d86ef286ca84 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -75,6 +75,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
> struct av_decision *avd, struct extended_perms *xperms);
> void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key,
> struct extended_perms_decision *xpermd);
> -int evaluate_cond_node(struct policydb *p, struct cond_node *node);
> +void evaluate_cond_node(struct policydb *p, struct cond_node *node);
>
> #endif /* _CONDITIONAL_H_ */
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 42ca9f6dbbf4..b9eda7d89e22 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2157,8 +2157,8 @@ static void security_load_policycaps(struct selinux_state *state)
> }
> }
>
> -static int security_preserve_bools(struct selinux_state *state,
> - struct policydb *newpolicydb);
> +static void security_preserve_bools(struct policydb *oldpolicydb,
> + struct policydb *newpolicydb);
>
> /**
> * security_load_policy - Load a security policy configuration.
> @@ -2257,11 +2257,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> if (rc)
> goto err;
>
> - rc = security_preserve_bools(state, newpolicydb);
> - if (rc) {
> - pr_err("SELinux: unable to preserve booleans\n");
> - goto err;
> - }
> + security_preserve_bools(policydb, newpolicydb);
>
> oldsidtab = state->ss->sidtab;
>
> @@ -2958,11 +2954,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
> policydb->bool_val_to_struct[i]->state = 0;
> }
>
> - for (cur = policydb->cond_list; cur; cur = cur->next) {
> - rc = evaluate_cond_node(policydb, cur);
> - if (rc)
> - goto out;
> - }
> + for (cur = policydb->cond_list; cur; cur = cur->next)
> + evaluate_cond_node(policydb, cur);
>
> seqno = ++state->ss->latest_granting;
> rc = 0;
> @@ -2999,36 +2992,23 @@ out:
> return rc;
> }
>
> -static int security_preserve_bools(struct selinux_state *state,
> - struct policydb *policydb)
> +static void security_preserve_bools(struct policydb *oldpolicydb,
> + struct policydb *newpolicydb)
> {
> - int rc, nbools = 0, *bvalues = NULL, i;
> - char **bnames = NULL;
> struct cond_bool_datum *booldatum;
> struct cond_node *cur;
> + int i;
>
> - rc = security_get_bools(state, &nbools, &bnames, &bvalues);
> - if (rc)
> - goto out;
> - for (i = 0; i < nbools; i++) {
> - booldatum = hashtab_search(policydb->p_bools.table, bnames[i]);
> - if (booldatum)
> - booldatum->state = bvalues[i];
> - }
> - for (cur = policydb->cond_list; cur; cur = cur->next) {
> - rc = evaluate_cond_node(policydb, cur);
> - if (rc)
> - goto out;
> - }
> + for (i = 0; i < oldpolicydb->p_bools.nprim; i++) {
> + const char *name = sym_name(oldpolicydb, SYM_BOOLS, i);
> + int value = oldpolicydb->bool_val_to_struct[i]->state;
>
> -out:
> - if (bnames) {
> - for (i = 0; i < nbools; i++)
> - kfree(bnames[i]);
> + booldatum = hashtab_search(newpolicydb->p_bools.table, name);
> + if (booldatum)
> + booldatum->state = value;
> }
> - kfree(bnames);
> - kfree(bvalues);
> - return rc;
> + for (cur = newpolicydb->cond_list; cur; cur = cur->next)
> + evaluate_cond_node(newpolicydb, cur);
> }
>
> /*
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] selinux: simplify security_preserve_bools()
2020-01-16 16:42 ` Stephen Smalley
@ 2020-01-16 22:28 ` Paul Moore
0 siblings, 0 replies; 21+ messages in thread
From: Paul Moore @ 2020-01-16 22:28 UTC (permalink / raw)
To: Stephen Smalley, Ondrej Mosnacek; +Cc: selinux
On Thu, Jan 16, 2020 at 11:41 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> > First, evaluate_cond_node() never returns an error. Make it just return
> > void.
> >
> > Second, drop the use of security_get_bools() from
> > security_preserve_bools() and read from the old policydb directly. This
> > saves some useless allocations and together with the first change makes
> > security_preserve_bools() no longer possibly return an error. Again the
> > return type is changed to void.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Dropping use of security_get_bools() means we are no longer reading the
> boolean values with the policy read-lock held so they could in theory
> change underneath us. However, this is presently prevented via the
> fsi->mutex taken by selinuxfs so I believe this is safe.
Since this code shouldn't be run very often, I think I would prefer
the added abstraction and safety of preserving the call to
security_get_bools().
In an effort to make sure expectations are set correctly, patches 2
through 6 are something that should probably wait until after the
upcoming merge window, so no rush on a respin.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/6] selinux: convert cond_list to array
2020-01-16 12:04 [PATCH 0/6] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
2020-01-16 12:04 ` [PATCH 1/6] selinux: do not allocate ancillary buffer on first load Ondrej Mosnacek
2020-01-16 12:04 ` [PATCH 2/6] selinux: simplify security_preserve_bools() Ondrej Mosnacek
@ 2020-01-16 12:04 ` Ondrej Mosnacek
2020-01-16 17:07 ` Stephen Smalley
2020-01-16 12:04 ` [PATCH 4/6] selinux: convert cond_av_list " Ondrej Mosnacek
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Ondrej Mosnacek @ 2020-01-16 12:04 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
While there, also fix signedness of some related variables/parameters.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/include/conditional.h | 6 +--
security/selinux/selinuxfs.c | 4 +-
security/selinux/ss/conditional.c | 54 ++++++++++----------------
security/selinux/ss/conditional.h | 3 +-
security/selinux/ss/policydb.c | 2 +-
security/selinux/ss/policydb.h | 3 +-
security/selinux/ss/services.c | 27 +++++++------
7 files changed, 42 insertions(+), 57 deletions(-)
diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
index 0ab316f61da0..ffb9a33341f8 100644
--- a/security/selinux/include/conditional.h
+++ b/security/selinux/include/conditional.h
@@ -14,12 +14,12 @@
#include "security.h"
int security_get_bools(struct selinux_state *state,
- int *len, char ***names, int **values);
+ u32 *len, char ***names, int **values);
int security_set_bools(struct selinux_state *state,
- int len, int *values);
+ u32 len, int *values);
int security_get_bool_value(struct selinux_state *state,
- int index);
+ u32 index);
#endif
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 79c710911a3c..296ce86e8b1f 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1327,14 +1327,14 @@ static void sel_remove_entries(struct dentry *de)
static int sel_make_bools(struct selinux_fs_info *fsi)
{
- int i, ret;
+ int ret;
ssize_t len;
struct dentry *dentry = NULL;
struct dentry *dir = fsi->bool_dir;
struct inode *inode = NULL;
struct inode_security_struct *isec;
char **names = NULL, *page;
- int num;
+ u32 i, num;
int *values = NULL;
u32 sid;
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 04593062008d..c8a02c9b23ee 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p)
p->bool_val_to_struct = NULL;
p->cond_list = NULL;
+ p->cond_list_len = 0;
rc = avtab_init(&p->te_cond_avtab);
if (rc)
@@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node)
}
cond_av_list_destroy(node->true_list);
cond_av_list_destroy(node->false_list);
- kfree(node);
}
-static void cond_list_destroy(struct cond_node *list)
+static void cond_list_destroy(struct policydb *p)
{
- struct cond_node *next, *cur;
+ u32 i;
- if (list == NULL)
- return;
-
- for (cur = list; cur; cur = next) {
- next = cur->next;
- cond_node_destroy(cur);
- }
+ for (i = 0; i < p->cond_list_len; i++)
+ cond_node_destroy(&p->cond_list[i]);
+ kfree(p->cond_list);
}
void cond_policydb_destroy(struct policydb *p)
{
kfree(p->bool_val_to_struct);
avtab_destroy(&p->te_cond_avtab);
- cond_list_destroy(p->cond_list);
+ cond_list_destroy(p);
}
int cond_init_bool_indexes(struct policydb *p)
@@ -447,7 +443,6 @@ err:
int cond_read_list(struct policydb *p, void *fp)
{
- struct cond_node *node, *last = NULL;
__le32 buf[1];
u32 i, len;
int rc;
@@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp)
len = le32_to_cpu(buf[0]);
+ p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
+ if (!p->cond_list)
+ return rc;
+
rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
if (rc)
goto err;
for (i = 0; i < len; i++) {
- rc = -ENOMEM;
- node = kzalloc(sizeof(*node), GFP_KERNEL);
- if (!node)
- goto err;
-
- rc = cond_read_node(p, node, fp);
+ rc = cond_read_node(p, &p->cond_list[i], fp);
if (rc)
goto err;
-
- if (i == 0)
- p->cond_list = node;
- else
- last->next = node;
- last = node;
}
+
+ p->cond_list_len = len;
return 0;
err:
- cond_list_destroy(p->cond_list);
+ cond_list_destroy(p);
p->cond_list = NULL;
return rc;
}
@@ -585,23 +575,19 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
return 0;
}
-int cond_write_list(struct policydb *p, struct cond_node *list, void *fp)
+int cond_write_list(struct policydb *p, void *fp)
{
- struct cond_node *cur;
- u32 len;
+ u32 i;
__le32 buf[1];
int rc;
- len = 0;
- for (cur = list; cur != NULL; cur = cur->next)
- len++;
- buf[0] = cpu_to_le32(len);
+ buf[0] = cpu_to_le32(p->cond_list_len);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- for (cur = list; cur != NULL; cur = cur->next) {
- rc = cond_write_node(p, cur, fp);
+ for (i = 0; i < p->cond_list_len; i++) {
+ rc = cond_write_node(p, &p->cond_list[i], fp);
if (rc)
return rc;
}
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index d86ef286ca84..e474bdd3a0ed 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -55,7 +55,6 @@ struct cond_node {
struct cond_expr *expr;
struct cond_av_list *true_list;
struct cond_av_list *false_list;
- struct cond_node *next;
};
int cond_policydb_init(struct policydb *p);
@@ -69,7 +68,7 @@ int cond_index_bool(void *key, void *datum, void *datap);
int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp);
int cond_read_list(struct policydb *p, void *fp);
int cond_write_bool(void *key, void *datum, void *ptr);
-int cond_write_list(struct policydb *p, struct cond_node *list, void *fp);
+int cond_write_list(struct policydb *p, void *fp);
void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
struct av_decision *avd, struct extended_perms *xperms);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index e369b0092cdf..ef1718394dee 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -3483,7 +3483,7 @@ int policydb_write(struct policydb *p, void *fp)
if (rc)
return rc;
- rc = cond_write_list(p, p->cond_list, fp);
+ rc = cond_write_list(p, fp);
if (rc)
return rc;
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 69b24191fa38..6459616f8487 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -272,8 +272,9 @@ struct policydb {
struct cond_bool_datum **bool_val_to_struct;
/* type enforcement conditional access vectors and transitions */
struct avtab te_cond_avtab;
- /* linked list indexing te_cond_avtab by conditional */
+ /* array indexing te_cond_avtab by conditional */
struct cond_node *cond_list;
+ u32 cond_list_len;
/* role allows */
struct role_allow *role_allow;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b9eda7d89e22..b0f71afcf4b8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2864,10 +2864,11 @@ out:
}
int security_get_bools(struct selinux_state *state,
- int *len, char ***names, int **values)
+ u32 *len, char ***names, int **values)
{
struct policydb *policydb;
- int i, rc;
+ u32 i;
+ int rc;
if (!state->initialized) {
*len = 0;
@@ -2921,12 +2922,11 @@ err:
}
-int security_set_bools(struct selinux_state *state, int len, int *values)
+int security_set_bools(struct selinux_state *state, u32 len, int *values)
{
struct policydb *policydb;
- int i, rc;
- int lenp, seqno = 0;
- struct cond_node *cur;
+ int rc;
+ u32 i, lenp, seqno = 0;
write_lock_irq(&state->ss->policy_rwlock);
@@ -2954,8 +2954,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
policydb->bool_val_to_struct[i]->state = 0;
}
- for (cur = policydb->cond_list; cur; cur = cur->next)
- evaluate_cond_node(policydb, cur);
+ for (i = 0; i < policydb->cond_list_len; i++)
+ evaluate_cond_node(policydb, &policydb->cond_list[i]);
seqno = ++state->ss->latest_granting;
rc = 0;
@@ -2971,11 +2971,11 @@ out:
}
int security_get_bool_value(struct selinux_state *state,
- int index)
+ u32 index)
{
struct policydb *policydb;
int rc;
- int len;
+ u32 len;
read_lock(&state->ss->policy_rwlock);
@@ -2996,8 +2996,7 @@ static void security_preserve_bools(struct policydb *oldpolicydb,
struct policydb *newpolicydb)
{
struct cond_bool_datum *booldatum;
- struct cond_node *cur;
- int i;
+ u32 i;
for (i = 0; i < oldpolicydb->p_bools.nprim; i++) {
const char *name = sym_name(oldpolicydb, SYM_BOOLS, i);
@@ -3007,8 +3006,8 @@ static void security_preserve_bools(struct policydb *oldpolicydb,
if (booldatum)
booldatum->state = value;
}
- for (cur = newpolicydb->cond_list; cur; cur = cur->next)
- evaluate_cond_node(newpolicydb, cur);
+ for (i = 0; i < newpolicydb->cond_list_len; i++)
+ evaluate_cond_node(newpolicydb, &newpolicydb->cond_list[i]);
}
/*
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/6] selinux: convert cond_list to array
2020-01-16 12:04 ` [PATCH 3/6] selinux: convert cond_list to array Ondrej Mosnacek
@ 2020-01-16 17:07 ` Stephen Smalley
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Smalley @ 2020-01-16 17:07 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> Since it is fixed-size after allocation and we know the size beforehand,
> using a plain old array is simpler and more efficient.
>
> While there, also fix signedness of some related variables/parameters.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/include/conditional.h | 6 +--
> security/selinux/selinuxfs.c | 4 +-
> security/selinux/ss/conditional.c | 54 ++++++++++----------------
> security/selinux/ss/conditional.h | 3 +-
> security/selinux/ss/policydb.c | 2 +-
> security/selinux/ss/policydb.h | 3 +-
> security/selinux/ss/services.c | 27 +++++++------
> 7 files changed, 42 insertions(+), 57 deletions(-)
>
> diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
> index 0ab316f61da0..ffb9a33341f8 100644
> --- a/security/selinux/include/conditional.h
> +++ b/security/selinux/include/conditional.h
> @@ -14,12 +14,12 @@
> #include "security.h"
>
> int security_get_bools(struct selinux_state *state,
> - int *len, char ***names, int **values);
> + u32 *len, char ***names, int **values);
>
> int security_set_bools(struct selinux_state *state,
> - int len, int *values);
> + u32 len, int *values);
>
> int security_get_bool_value(struct selinux_state *state,
> - int index);
> + u32 index);
>
> #endif
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 79c710911a3c..296ce86e8b1f 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1327,14 +1327,14 @@ static void sel_remove_entries(struct dentry *de)
>
> static int sel_make_bools(struct selinux_fs_info *fsi)
> {
> - int i, ret;
> + int ret;
> ssize_t len;
> struct dentry *dentry = NULL;
> struct dentry *dir = fsi->bool_dir;
> struct inode *inode = NULL;
> struct inode_security_struct *isec;
> char **names = NULL, *page;
> - int num;
> + u32 i, num;
> int *values = NULL;
> u32 sid;
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 04593062008d..c8a02c9b23ee 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p)
>
> p->bool_val_to_struct = NULL;
> p->cond_list = NULL;
> + p->cond_list_len = 0;
>
> rc = avtab_init(&p->te_cond_avtab);
> if (rc)
> @@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node)
> }
> cond_av_list_destroy(node->true_list);
> cond_av_list_destroy(node->false_list);
> - kfree(node);
> }
>
> -static void cond_list_destroy(struct cond_node *list)
> +static void cond_list_destroy(struct policydb *p)
> {
> - struct cond_node *next, *cur;
> + u32 i;
>
> - if (list == NULL)
> - return;
> -
> - for (cur = list; cur; cur = next) {
> - next = cur->next;
> - cond_node_destroy(cur);
> - }
> + for (i = 0; i < p->cond_list_len; i++)
> + cond_node_destroy(&p->cond_list[i]);
> + kfree(p->cond_list);
> }
>
> void cond_policydb_destroy(struct policydb *p)
> {
> kfree(p->bool_val_to_struct);
> avtab_destroy(&p->te_cond_avtab);
> - cond_list_destroy(p->cond_list);
> + cond_list_destroy(p);
> }
>
> int cond_init_bool_indexes(struct policydb *p)
> @@ -447,7 +443,6 @@ err:
>
> int cond_read_list(struct policydb *p, void *fp)
> {
> - struct cond_node *node, *last = NULL;
> __le32 buf[1];
> u32 i, len;
> int rc;
> @@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp)
>
> len = le32_to_cpu(buf[0]);
>
> + p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
> + if (!p->cond_list)
> + return rc;
> +
> rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
> if (rc)
> goto err;
>
> for (i = 0; i < len; i++) {
> - rc = -ENOMEM;
> - node = kzalloc(sizeof(*node), GFP_KERNEL);
> - if (!node)
> - goto err;
> -
> - rc = cond_read_node(p, node, fp);
> + rc = cond_read_node(p, &p->cond_list[i], fp);
> if (rc)
> goto err;
> -
> - if (i == 0)
> - p->cond_list = node;
> - else
> - last->next = node;
> - last = node;
> }
> +
> + p->cond_list_len = len;
> return 0;
> err:
> - cond_list_destroy(p->cond_list);
> + cond_list_destroy(p);
> p->cond_list = NULL;
> return rc;
> }
> @@ -585,23 +575,19 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
> return 0;
> }
>
> -int cond_write_list(struct policydb *p, struct cond_node *list, void *fp)
> +int cond_write_list(struct policydb *p, void *fp)
> {
> - struct cond_node *cur;
> - u32 len;
> + u32 i;
> __le32 buf[1];
> int rc;
>
> - len = 0;
> - for (cur = list; cur != NULL; cur = cur->next)
> - len++;
> - buf[0] = cpu_to_le32(len);
> + buf[0] = cpu_to_le32(p->cond_list_len);
> rc = put_entry(buf, sizeof(u32), 1, fp);
> if (rc)
> return rc;
>
> - for (cur = list; cur != NULL; cur = cur->next) {
> - rc = cond_write_node(p, cur, fp);
> + for (i = 0; i < p->cond_list_len; i++) {
> + rc = cond_write_node(p, &p->cond_list[i], fp);
> if (rc)
> return rc;
> }
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index d86ef286ca84..e474bdd3a0ed 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -55,7 +55,6 @@ struct cond_node {
> struct cond_expr *expr;
> struct cond_av_list *true_list;
> struct cond_av_list *false_list;
> - struct cond_node *next;
> };
>
> int cond_policydb_init(struct policydb *p);
> @@ -69,7 +68,7 @@ int cond_index_bool(void *key, void *datum, void *datap);
> int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp);
> int cond_read_list(struct policydb *p, void *fp);
> int cond_write_bool(void *key, void *datum, void *ptr);
> -int cond_write_list(struct policydb *p, struct cond_node *list, void *fp);
> +int cond_write_list(struct policydb *p, void *fp);
>
> void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
> struct av_decision *avd, struct extended_perms *xperms);
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index e369b0092cdf..ef1718394dee 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -3483,7 +3483,7 @@ int policydb_write(struct policydb *p, void *fp)
> if (rc)
> return rc;
>
> - rc = cond_write_list(p, p->cond_list, fp);
> + rc = cond_write_list(p, fp);
> if (rc)
> return rc;
>
> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 69b24191fa38..6459616f8487 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -272,8 +272,9 @@ struct policydb {
> struct cond_bool_datum **bool_val_to_struct;
> /* type enforcement conditional access vectors and transitions */
> struct avtab te_cond_avtab;
> - /* linked list indexing te_cond_avtab by conditional */
> + /* array indexing te_cond_avtab by conditional */
> struct cond_node *cond_list;
> + u32 cond_list_len;
>
> /* role allows */
> struct role_allow *role_allow;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index b9eda7d89e22..b0f71afcf4b8 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2864,10 +2864,11 @@ out:
> }
>
> int security_get_bools(struct selinux_state *state,
> - int *len, char ***names, int **values)
> + u32 *len, char ***names, int **values)
> {
> struct policydb *policydb;
> - int i, rc;
> + u32 i;
> + int rc;
>
> if (!state->initialized) {
> *len = 0;
> @@ -2921,12 +2922,11 @@ err:
> }
>
>
> -int security_set_bools(struct selinux_state *state, int len, int *values)
> +int security_set_bools(struct selinux_state *state, u32 len, int *values)
> {
> struct policydb *policydb;
> - int i, rc;
> - int lenp, seqno = 0;
> - struct cond_node *cur;
> + int rc;
> + u32 i, lenp, seqno = 0;
>
> write_lock_irq(&state->ss->policy_rwlock);
>
> @@ -2954,8 +2954,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values)
> policydb->bool_val_to_struct[i]->state = 0;
> }
>
> - for (cur = policydb->cond_list; cur; cur = cur->next)
> - evaluate_cond_node(policydb, cur);
> + for (i = 0; i < policydb->cond_list_len; i++)
> + evaluate_cond_node(policydb, &policydb->cond_list[i]);
>
> seqno = ++state->ss->latest_granting;
> rc = 0;
> @@ -2971,11 +2971,11 @@ out:
> }
>
> int security_get_bool_value(struct selinux_state *state,
> - int index)
> + u32 index)
> {
> struct policydb *policydb;
> int rc;
> - int len;
> + u32 len;
>
> read_lock(&state->ss->policy_rwlock);
>
> @@ -2996,8 +2996,7 @@ static void security_preserve_bools(struct policydb *oldpolicydb,
> struct policydb *newpolicydb)
> {
> struct cond_bool_datum *booldatum;
> - struct cond_node *cur;
> - int i;
> + u32 i;
>
> for (i = 0; i < oldpolicydb->p_bools.nprim; i++) {
> const char *name = sym_name(oldpolicydb, SYM_BOOLS, i);
> @@ -3007,8 +3006,8 @@ static void security_preserve_bools(struct policydb *oldpolicydb,
> if (booldatum)
> booldatum->state = value;
> }
> - for (cur = newpolicydb->cond_list; cur; cur = cur->next)
> - evaluate_cond_node(newpolicydb, cur);
> + for (i = 0; i < newpolicydb->cond_list_len; i++)
> + evaluate_cond_node(newpolicydb, &newpolicydb->cond_list[i]);
> }
>
> /*
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/6] selinux: convert cond_av_list to array
2020-01-16 12:04 [PATCH 0/6] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (2 preceding siblings ...)
2020-01-16 12:04 ` [PATCH 3/6] selinux: convert cond_list to array Ondrej Mosnacek
@ 2020-01-16 12:04 ` Ondrej Mosnacek
2020-01-16 17:13 ` Stephen Smalley
2020-01-16 12:04 ` [PATCH 5/6] selinux: convert cond_expr " Ondrej Mosnacek
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Ondrej Mosnacek @ 2020-01-16 12:04 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/conditional.c | 125 ++++++++++++------------------
security/selinux/ss/conditional.h | 8 +-
2 files changed, 53 insertions(+), 80 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index c8a02c9b23ee..b847fd2a6a51 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -87,8 +87,9 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
*/
void evaluate_cond_node(struct policydb *p, struct cond_node *node)
{
+ struct avtab_node *avnode;
int new_state;
- struct cond_av_list *cur;
+ u32 i;
new_state = cond_evaluate_expr(p, node->expr);
if (new_state != node->cur_state) {
@@ -96,19 +97,21 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
if (new_state == -1)
pr_err("SELinux: expression result was undefined - disabling all rules.\n");
/* turn the rules on or off */
- for (cur = node->true_list; cur; cur = cur->next) {
+ for (i = 0; i < node->true_list.len; i++) {
+ avnode = node->true_list.nodes[i];
if (new_state <= 0)
- cur->node->key.specified &= ~AVTAB_ENABLED;
+ avnode->key.specified &= ~AVTAB_ENABLED;
else
- cur->node->key.specified |= AVTAB_ENABLED;
+ avnode->key.specified |= AVTAB_ENABLED;
}
- for (cur = node->false_list; cur; cur = cur->next) {
+ for (i = 0; i < node->false_list.len; i++) {
+ avnode = node->false_list.nodes[i];
/* -1 or 1 */
if (new_state)
- cur->node->key.specified &= ~AVTAB_ENABLED;
+ avnode->key.specified &= ~AVTAB_ENABLED;
else
- cur->node->key.specified |= AVTAB_ENABLED;
+ avnode->key.specified |= AVTAB_ENABLED;
}
}
}
@@ -128,16 +131,6 @@ int cond_policydb_init(struct policydb *p)
return 0;
}
-static void cond_av_list_destroy(struct cond_av_list *list)
-{
- struct cond_av_list *cur, *next;
- for (cur = list; cur; cur = next) {
- next = cur->next;
- /* the avtab_ptr_t node is destroy by the avtab */
- kfree(cur);
- }
-}
-
static void cond_node_destroy(struct cond_node *node)
{
struct cond_expr *cur_expr, *next_expr;
@@ -146,8 +139,9 @@ static void cond_node_destroy(struct cond_node *node)
next_expr = cur_expr->next;
kfree(cur_expr);
}
- cond_av_list_destroy(node->true_list);
- cond_av_list_destroy(node->false_list);
+ /* the avtab_ptr_t nodes are destroyed by the avtab */
+ kfree(node->true_list.nodes);
+ kfree(node->false_list.nodes);
}
static void cond_list_destroy(struct policydb *p)
@@ -255,19 +249,17 @@ err:
struct cond_insertf_data {
struct policydb *p;
+ struct avtab_node **dst;
struct cond_av_list *other;
- struct cond_av_list *head;
- struct cond_av_list *tail;
};
static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum *d, void *ptr)
{
struct cond_insertf_data *data = ptr;
struct policydb *p = data->p;
- struct cond_av_list *other = data->other, *list, *cur;
struct avtab_node *node_ptr;
- u8 found;
- int rc = -EINVAL;
+ u32 i;
+ bool found;
/*
* For type rules we have to make certain there aren't any
@@ -277,7 +269,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
if (k->specified & AVTAB_TYPE) {
if (avtab_search(&p->te_avtab, k)) {
pr_err("SELinux: type rule already exists outside of a conditional.\n");
- goto err;
+ return -EINVAL;
}
/*
* If we are reading the false list other will be a pointer to
@@ -287,29 +279,29 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
* If we are reading the true list (other == NULL) there shouldn't
* be any other entries.
*/
- if (other) {
+ if (data->other) {
node_ptr = avtab_search_node(&p->te_cond_avtab, k);
if (node_ptr) {
if (avtab_search_node_next(node_ptr, k->specified)) {
pr_err("SELinux: too many conflicting type rules.\n");
- goto err;
+ return -EINVAL;
}
- found = 0;
- for (cur = other; cur; cur = cur->next) {
- if (cur->node == node_ptr) {
- found = 1;
+ found = false;
+ for (i = 0; i < data->other->len; i++) {
+ if (data->other->nodes[i] == node_ptr) {
+ found = true;
break;
}
}
if (!found) {
pr_err("SELinux: conflicting type rules.\n");
- goto err;
+ return -EINVAL;
}
}
} else {
if (avtab_search(&p->te_cond_avtab, k)) {
pr_err("SELinux: conflicting type rules when adding type rule for true.\n");
- goto err;
+ return -EINVAL;
}
}
}
@@ -317,39 +309,22 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
node_ptr = avtab_insert_nonunique(&p->te_cond_avtab, k, d);
if (!node_ptr) {
pr_err("SELinux: could not insert rule.\n");
- rc = -ENOMEM;
- goto err;
- }
-
- list = kzalloc(sizeof(*list), GFP_KERNEL);
- if (!list) {
- rc = -ENOMEM;
- goto err;
+ return -ENOMEM;
}
- list->node = node_ptr;
- if (!data->head)
- data->head = list;
- else
- data->tail->next = list;
- data->tail = list;
+ *data->dst = node_ptr;
return 0;
-
-err:
- cond_av_list_destroy(data->head);
- data->head = NULL;
- return rc;
}
-static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list **ret_list, struct cond_av_list *other)
+static int cond_read_av_list(struct policydb *p, void *fp,
+ struct cond_av_list *list,
+ struct cond_av_list *other)
{
- int i, rc;
+ int rc;
__le32 buf[1];
- u32 len;
+ u32 i, len;
struct cond_insertf_data data;
- *ret_list = NULL;
-
rc = next_entry(buf, fp, sizeof(u32));
if (rc)
return rc;
@@ -358,18 +333,24 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list *
if (len == 0)
return 0;
+ list->nodes = kcalloc(len, sizeof(*list->nodes), GFP_KERNEL);
+ if (!list->nodes)
+ return -ENOMEM;
+
data.p = p;
data.other = other;
- data.head = NULL;
- data.tail = NULL;
for (i = 0; i < len; i++) {
+ data.dst = &list->nodes[i];
rc = avtab_read_item(&p->te_cond_avtab, fp, p, cond_insertf,
&data);
- if (rc)
+ if (rc) {
+ kfree(list->nodes);
+ list->nodes = NULL;
return rc;
+ }
}
- *ret_list = data.head;
+ list->len = len;
return 0;
}
@@ -432,7 +413,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
rc = cond_read_av_list(p, fp, &node->true_list, NULL);
if (rc)
goto err;
- rc = cond_read_av_list(p, fp, &node->false_list, node->true_list);
+ rc = cond_read_av_list(p, fp, &node->false_list, &node->true_list);
if (rc)
goto err;
return 0;
@@ -511,24 +492,16 @@ static int cond_write_av_list(struct policydb *p,
struct cond_av_list *list, struct policy_file *fp)
{
__le32 buf[1];
- struct cond_av_list *cur_list;
- u32 len;
+ u32 i;
int rc;
- len = 0;
- for (cur_list = list; cur_list != NULL; cur_list = cur_list->next)
- len++;
-
- buf[0] = cpu_to_le32(len);
+ buf[0] = cpu_to_le32(list->len);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- if (len == 0)
- return 0;
-
- for (cur_list = list; cur_list != NULL; cur_list = cur_list->next) {
- rc = avtab_write_item(p, cur_list->node, fp);
+ for (i = 0; i < list->len; i++) {
+ rc = avtab_write_item(p, list->nodes[i], fp);
if (rc)
return rc;
}
@@ -565,10 +538,10 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
return rc;
}
- rc = cond_write_av_list(p, node->true_list, fp);
+ rc = cond_write_av_list(p, &node->true_list, fp);
if (rc)
return rc;
- rc = cond_write_av_list(p, node->false_list, fp);
+ rc = cond_write_av_list(p, &node->false_list, fp);
if (rc)
return rc;
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index e474bdd3a0ed..5f97f678440e 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -39,8 +39,8 @@ struct cond_expr {
* struct is for that list.
*/
struct cond_av_list {
- struct avtab_node *node;
- struct cond_av_list *next;
+ struct avtab_node **nodes;
+ u32 len;
};
/*
@@ -53,8 +53,8 @@ struct cond_av_list {
struct cond_node {
int cur_state;
struct cond_expr *expr;
- struct cond_av_list *true_list;
- struct cond_av_list *false_list;
+ struct cond_av_list true_list;
+ struct cond_av_list false_list;
};
int cond_policydb_init(struct policydb *p);
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] selinux: convert cond_av_list to array
2020-01-16 12:04 ` [PATCH 4/6] selinux: convert cond_av_list " Ondrej Mosnacek
@ 2020-01-16 17:13 ` Stephen Smalley
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Smalley @ 2020-01-16 17:13 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> Since it is fixed-size after allocation and we know the size beforehand,
> using a plain old array is simpler and more efficient.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/conditional.c | 125 ++++++++++++------------------
> security/selinux/ss/conditional.h | 8 +-
> 2 files changed, 53 insertions(+), 80 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index c8a02c9b23ee..b847fd2a6a51 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -87,8 +87,9 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
> */
> void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> {
> + struct avtab_node *avnode;
> int new_state;
> - struct cond_av_list *cur;
> + u32 i;
>
> new_state = cond_evaluate_expr(p, node->expr);
> if (new_state != node->cur_state) {
> @@ -96,19 +97,21 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> if (new_state == -1)
> pr_err("SELinux: expression result was undefined - disabling all rules.\n");
> /* turn the rules on or off */
> - for (cur = node->true_list; cur; cur = cur->next) {
> + for (i = 0; i < node->true_list.len; i++) {
> + avnode = node->true_list.nodes[i];
> if (new_state <= 0)
> - cur->node->key.specified &= ~AVTAB_ENABLED;
> + avnode->key.specified &= ~AVTAB_ENABLED;
> else
> - cur->node->key.specified |= AVTAB_ENABLED;
> + avnode->key.specified |= AVTAB_ENABLED;
> }
>
> - for (cur = node->false_list; cur; cur = cur->next) {
> + for (i = 0; i < node->false_list.len; i++) {
> + avnode = node->false_list.nodes[i];
> /* -1 or 1 */
> if (new_state)
> - cur->node->key.specified &= ~AVTAB_ENABLED;
> + avnode->key.specified &= ~AVTAB_ENABLED;
> else
> - cur->node->key.specified |= AVTAB_ENABLED;
> + avnode->key.specified |= AVTAB_ENABLED;
> }
> }
> }
> @@ -128,16 +131,6 @@ int cond_policydb_init(struct policydb *p)
> return 0;
> }
>
> -static void cond_av_list_destroy(struct cond_av_list *list)
> -{
> - struct cond_av_list *cur, *next;
> - for (cur = list; cur; cur = next) {
> - next = cur->next;
> - /* the avtab_ptr_t node is destroy by the avtab */
> - kfree(cur);
> - }
> -}
> -
> static void cond_node_destroy(struct cond_node *node)
> {
> struct cond_expr *cur_expr, *next_expr;
> @@ -146,8 +139,9 @@ static void cond_node_destroy(struct cond_node *node)
> next_expr = cur_expr->next;
> kfree(cur_expr);
> }
> - cond_av_list_destroy(node->true_list);
> - cond_av_list_destroy(node->false_list);
> + /* the avtab_ptr_t nodes are destroyed by the avtab */
> + kfree(node->true_list.nodes);
> + kfree(node->false_list.nodes);
> }
>
> static void cond_list_destroy(struct policydb *p)
> @@ -255,19 +249,17 @@ err:
>
> struct cond_insertf_data {
> struct policydb *p;
> + struct avtab_node **dst;
> struct cond_av_list *other;
> - struct cond_av_list *head;
> - struct cond_av_list *tail;
> };
>
> static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum *d, void *ptr)
> {
> struct cond_insertf_data *data = ptr;
> struct policydb *p = data->p;
> - struct cond_av_list *other = data->other, *list, *cur;
> struct avtab_node *node_ptr;
> - u8 found;
> - int rc = -EINVAL;
> + u32 i;
> + bool found;
>
> /*
> * For type rules we have to make certain there aren't any
> @@ -277,7 +269,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
> if (k->specified & AVTAB_TYPE) {
> if (avtab_search(&p->te_avtab, k)) {
> pr_err("SELinux: type rule already exists outside of a conditional.\n");
> - goto err;
> + return -EINVAL;
> }
> /*
> * If we are reading the false list other will be a pointer to
> @@ -287,29 +279,29 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
> * If we are reading the true list (other == NULL) there shouldn't
> * be any other entries.
> */
> - if (other) {
> + if (data->other) {
> node_ptr = avtab_search_node(&p->te_cond_avtab, k);
> if (node_ptr) {
> if (avtab_search_node_next(node_ptr, k->specified)) {
> pr_err("SELinux: too many conflicting type rules.\n");
> - goto err;
> + return -EINVAL;
> }
> - found = 0;
> - for (cur = other; cur; cur = cur->next) {
> - if (cur->node == node_ptr) {
> - found = 1;
> + found = false;
> + for (i = 0; i < data->other->len; i++) {
> + if (data->other->nodes[i] == node_ptr) {
> + found = true;
> break;
> }
> }
> if (!found) {
> pr_err("SELinux: conflicting type rules.\n");
> - goto err;
> + return -EINVAL;
> }
> }
> } else {
> if (avtab_search(&p->te_cond_avtab, k)) {
> pr_err("SELinux: conflicting type rules when adding type rule for true.\n");
> - goto err;
> + return -EINVAL;
> }
> }
> }
> @@ -317,39 +309,22 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
> node_ptr = avtab_insert_nonunique(&p->te_cond_avtab, k, d);
> if (!node_ptr) {
> pr_err("SELinux: could not insert rule.\n");
> - rc = -ENOMEM;
> - goto err;
> - }
> -
> - list = kzalloc(sizeof(*list), GFP_KERNEL);
> - if (!list) {
> - rc = -ENOMEM;
> - goto err;
> + return -ENOMEM;
> }
>
> - list->node = node_ptr;
> - if (!data->head)
> - data->head = list;
> - else
> - data->tail->next = list;
> - data->tail = list;
> + *data->dst = node_ptr;
> return 0;
> -
> -err:
> - cond_av_list_destroy(data->head);
> - data->head = NULL;
> - return rc;
> }
>
> -static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list **ret_list, struct cond_av_list *other)
> +static int cond_read_av_list(struct policydb *p, void *fp,
> + struct cond_av_list *list,
> + struct cond_av_list *other)
> {
> - int i, rc;
> + int rc;
> __le32 buf[1];
> - u32 len;
> + u32 i, len;
> struct cond_insertf_data data;
>
> - *ret_list = NULL;
> -
> rc = next_entry(buf, fp, sizeof(u32));
> if (rc)
> return rc;
> @@ -358,18 +333,24 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list *
> if (len == 0)
> return 0;
>
> + list->nodes = kcalloc(len, sizeof(*list->nodes), GFP_KERNEL);
> + if (!list->nodes)
> + return -ENOMEM;
> +
> data.p = p;
> data.other = other;
> - data.head = NULL;
> - data.tail = NULL;
> for (i = 0; i < len; i++) {
> + data.dst = &list->nodes[i];
> rc = avtab_read_item(&p->te_cond_avtab, fp, p, cond_insertf,
> &data);
> - if (rc)
> + if (rc) {
> + kfree(list->nodes);
> + list->nodes = NULL;
> return rc;
> + }
> }
>
> - *ret_list = data.head;
> + list->len = len;
> return 0;
> }
>
> @@ -432,7 +413,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
> rc = cond_read_av_list(p, fp, &node->true_list, NULL);
> if (rc)
> goto err;
> - rc = cond_read_av_list(p, fp, &node->false_list, node->true_list);
> + rc = cond_read_av_list(p, fp, &node->false_list, &node->true_list);
> if (rc)
> goto err;
> return 0;
> @@ -511,24 +492,16 @@ static int cond_write_av_list(struct policydb *p,
> struct cond_av_list *list, struct policy_file *fp)
> {
> __le32 buf[1];
> - struct cond_av_list *cur_list;
> - u32 len;
> + u32 i;
> int rc;
>
> - len = 0;
> - for (cur_list = list; cur_list != NULL; cur_list = cur_list->next)
> - len++;
> -
> - buf[0] = cpu_to_le32(len);
> + buf[0] = cpu_to_le32(list->len);
> rc = put_entry(buf, sizeof(u32), 1, fp);
> if (rc)
> return rc;
>
> - if (len == 0)
> - return 0;
> -
> - for (cur_list = list; cur_list != NULL; cur_list = cur_list->next) {
> - rc = avtab_write_item(p, cur_list->node, fp);
> + for (i = 0; i < list->len; i++) {
> + rc = avtab_write_item(p, list->nodes[i], fp);
> if (rc)
> return rc;
> }
> @@ -565,10 +538,10 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
> return rc;
> }
>
> - rc = cond_write_av_list(p, node->true_list, fp);
> + rc = cond_write_av_list(p, &node->true_list, fp);
> if (rc)
> return rc;
> - rc = cond_write_av_list(p, node->false_list, fp);
> + rc = cond_write_av_list(p, &node->false_list, fp);
> if (rc)
> return rc;
>
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index e474bdd3a0ed..5f97f678440e 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -39,8 +39,8 @@ struct cond_expr {
> * struct is for that list.
> */
> struct cond_av_list {
> - struct avtab_node *node;
> - struct cond_av_list *next;
> + struct avtab_node **nodes;
> + u32 len;
> };
>
> /*
> @@ -53,8 +53,8 @@ struct cond_av_list {
> struct cond_node {
> int cur_state;
> struct cond_expr *expr;
> - struct cond_av_list *true_list;
> - struct cond_av_list *false_list;
> + struct cond_av_list true_list;
> + struct cond_av_list false_list;
> };
>
> int cond_policydb_init(struct policydb *p);
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/6] selinux: convert cond_expr to array
2020-01-16 12:04 [PATCH 0/6] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (3 preceding siblings ...)
2020-01-16 12:04 ` [PATCH 4/6] selinux: convert cond_av_list " Ondrej Mosnacek
@ 2020-01-16 12:04 ` Ondrej Mosnacek
2020-01-16 17:17 ` Stephen Smalley
2020-01-16 12:04 ` [PATCH 6/6] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
2020-01-16 23:09 ` [PATCH 0/6] selinux: Assorted simplifications and cleanups Casey Schaufler
6 siblings, 1 reply; 21+ messages in thread
From: Ondrej Mosnacek @ 2020-01-16 12:04 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/conditional.c | 62 ++++++++++++-------------------
security/selinux/ss/conditional.h | 14 ++++---
2 files changed, 33 insertions(+), 43 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index b847fd2a6a51..8f9f2f3c86a0 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -23,18 +23,19 @@
*/
static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
{
-
- struct cond_expr *cur;
+ u32 i;
int s[COND_EXPR_MAXDEPTH];
int sp = -1;
- for (cur = expr; cur; cur = cur->next) {
- switch (cur->expr_type) {
+ for (i = 0; i < expr->len; i++) {
+ struct cond_expr_node *node = &expr->nodes[i];
+
+ switch (node->expr_type) {
case COND_BOOL:
if (sp == (COND_EXPR_MAXDEPTH - 1))
return -1;
sp++;
- s[sp] = p->bool_val_to_struct[cur->bool - 1]->state;
+ s[sp] = p->bool_val_to_struct[node->bool - 1]->state;
break;
case COND_NOT:
if (sp < 0)
@@ -91,7 +92,7 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
int new_state;
u32 i;
- new_state = cond_evaluate_expr(p, node->expr);
+ new_state = cond_evaluate_expr(p, &node->expr);
if (new_state != node->cur_state) {
node->cur_state = new_state;
if (new_state == -1)
@@ -133,12 +134,7 @@ int cond_policydb_init(struct policydb *p)
static void cond_node_destroy(struct cond_node *node)
{
- struct cond_expr *cur_expr, *next_expr;
-
- for (cur_expr = node->expr; cur_expr; cur_expr = next_expr) {
- next_expr = cur_expr->next;
- kfree(cur_expr);
- }
+ kfree(node->expr.nodes);
/* the avtab_ptr_t nodes are destroyed by the avtab */
kfree(node->true_list.nodes);
kfree(node->false_list.nodes);
@@ -354,7 +350,7 @@ static int cond_read_av_list(struct policydb *p, void *fp,
return 0;
}
-static int expr_isvalid(struct policydb *p, struct cond_expr *expr)
+static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr)
{
if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) {
pr_err("SELinux: conditional expressions uses unknown operator.\n");
@@ -371,43 +367,37 @@ static int expr_isvalid(struct policydb *p, struct cond_expr *expr)
static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
{
__le32 buf[2];
- u32 len, i;
+ u32 i, len;
int rc;
- struct cond_expr *expr = NULL, *last = NULL;
rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
- goto err;
+ return rc;
node->cur_state = le32_to_cpu(buf[0]);
/* expr */
len = le32_to_cpu(buf[1]);
+ node->expr.nodes = kcalloc(len, sizeof(*node->expr.nodes), GFP_KERNEL);
+ if (!node->expr.nodes)
+ return -ENOMEM;
+
+ node->expr.len = len;
for (i = 0; i < len; i++) {
+ struct cond_expr_node *expr = &node->expr.nodes[i];
+
rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
goto err;
- rc = -ENOMEM;
- expr = kzalloc(sizeof(*expr), GFP_KERNEL);
- if (!expr)
- goto err;
-
expr->expr_type = le32_to_cpu(buf[0]);
expr->bool = le32_to_cpu(buf[1]);
- if (!expr_isvalid(p, expr)) {
+ if (!expr_node_isvalid(p, expr)) {
rc = -EINVAL;
- kfree(expr);
goto err;
}
-
- if (i == 0)
- node->expr = expr;
- else
- last->next = expr;
- last = expr;
}
rc = cond_read_av_list(p, fp, &node->true_list, NULL);
@@ -512,27 +502,23 @@ static int cond_write_av_list(struct policydb *p,
static int cond_write_node(struct policydb *p, struct cond_node *node,
struct policy_file *fp)
{
- struct cond_expr *cur_expr;
__le32 buf[2];
int rc;
- u32 len = 0;
+ u32 i;
buf[0] = cpu_to_le32(node->cur_state);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next)
- len++;
-
- buf[0] = cpu_to_le32(len);
+ buf[0] = cpu_to_le32(node->expr.len);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next) {
- buf[0] = cpu_to_le32(cur_expr->expr_type);
- buf[1] = cpu_to_le32(cur_expr->bool);
+ for (i = 0; i < node->expr.len; i++) {
+ buf[0] = cpu_to_le32(node->expr.nodes[i].expr_type);
+ buf[1] = cpu_to_le32(node->expr.nodes[i].bool);
rc = put_entry(buf, sizeof(u32), 2, fp);
if (rc)
return rc;
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index 5f97f678440e..4677c6ff7450 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -19,7 +19,7 @@
* A conditional expression is a list of operators and operands
* in reverse polish notation.
*/
-struct cond_expr {
+struct cond_expr_node {
#define COND_BOOL 1 /* plain bool */
#define COND_NOT 2 /* !bool */
#define COND_OR 3 /* bool || bool */
@@ -28,9 +28,13 @@ struct cond_expr {
#define COND_EQ 6 /* bool == bool */
#define COND_NEQ 7 /* bool != bool */
#define COND_LAST COND_NEQ
- __u32 expr_type;
- __u32 bool;
- struct cond_expr *next;
+ u32 expr_type;
+ u32 bool;
+};
+
+struct cond_expr {
+ struct cond_expr_node *nodes;
+ u32 len;
};
/*
@@ -52,7 +56,7 @@ struct cond_av_list {
*/
struct cond_node {
int cur_state;
- struct cond_expr *expr;
+ struct cond_expr expr;
struct cond_av_list true_list;
struct cond_av_list false_list;
};
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/6] selinux: convert cond_expr to array
2020-01-16 12:04 ` [PATCH 5/6] selinux: convert cond_expr " Ondrej Mosnacek
@ 2020-01-16 17:17 ` Stephen Smalley
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Smalley @ 2020-01-16 17:17 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> Since it is fixed-size after allocation and we know the size beforehand,
> using a plain old array is simpler and more efficient.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/conditional.c | 62 ++++++++++++-------------------
> security/selinux/ss/conditional.h | 14 ++++---
> 2 files changed, 33 insertions(+), 43 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index b847fd2a6a51..8f9f2f3c86a0 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -23,18 +23,19 @@
> */
> static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
> {
> -
> - struct cond_expr *cur;
> + u32 i;
> int s[COND_EXPR_MAXDEPTH];
> int sp = -1;
>
> - for (cur = expr; cur; cur = cur->next) {
> - switch (cur->expr_type) {
> + for (i = 0; i < expr->len; i++) {
> + struct cond_expr_node *node = &expr->nodes[i];
> +
> + switch (node->expr_type) {
> case COND_BOOL:
> if (sp == (COND_EXPR_MAXDEPTH - 1))
> return -1;
> sp++;
> - s[sp] = p->bool_val_to_struct[cur->bool - 1]->state;
> + s[sp] = p->bool_val_to_struct[node->bool - 1]->state;
> break;
> case COND_NOT:
> if (sp < 0)
> @@ -91,7 +92,7 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> int new_state;
> u32 i;
>
> - new_state = cond_evaluate_expr(p, node->expr);
> + new_state = cond_evaluate_expr(p, &node->expr);
> if (new_state != node->cur_state) {
> node->cur_state = new_state;
> if (new_state == -1)
> @@ -133,12 +134,7 @@ int cond_policydb_init(struct policydb *p)
>
> static void cond_node_destroy(struct cond_node *node)
> {
> - struct cond_expr *cur_expr, *next_expr;
> -
> - for (cur_expr = node->expr; cur_expr; cur_expr = next_expr) {
> - next_expr = cur_expr->next;
> - kfree(cur_expr);
> - }
> + kfree(node->expr.nodes);
> /* the avtab_ptr_t nodes are destroyed by the avtab */
> kfree(node->true_list.nodes);
> kfree(node->false_list.nodes);
> @@ -354,7 +350,7 @@ static int cond_read_av_list(struct policydb *p, void *fp,
> return 0;
> }
>
> -static int expr_isvalid(struct policydb *p, struct cond_expr *expr)
> +static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr)
> {
> if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) {
> pr_err("SELinux: conditional expressions uses unknown operator.\n");
> @@ -371,43 +367,37 @@ static int expr_isvalid(struct policydb *p, struct cond_expr *expr)
> static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
> {
> __le32 buf[2];
> - u32 len, i;
> + u32 i, len;
> int rc;
> - struct cond_expr *expr = NULL, *last = NULL;
>
> rc = next_entry(buf, fp, sizeof(u32) * 2);
> if (rc)
> - goto err;
> + return rc;
>
> node->cur_state = le32_to_cpu(buf[0]);
>
> /* expr */
> len = le32_to_cpu(buf[1]);
> + node->expr.nodes = kcalloc(len, sizeof(*node->expr.nodes), GFP_KERNEL);
> + if (!node->expr.nodes)
> + return -ENOMEM;
> +
> + node->expr.len = len;
>
> for (i = 0; i < len; i++) {
> + struct cond_expr_node *expr = &node->expr.nodes[i];
> +
> rc = next_entry(buf, fp, sizeof(u32) * 2);
> if (rc)
> goto err;
>
> - rc = -ENOMEM;
> - expr = kzalloc(sizeof(*expr), GFP_KERNEL);
> - if (!expr)
> - goto err;
> -
> expr->expr_type = le32_to_cpu(buf[0]);
> expr->bool = le32_to_cpu(buf[1]);
>
> - if (!expr_isvalid(p, expr)) {
> + if (!expr_node_isvalid(p, expr)) {
> rc = -EINVAL;
> - kfree(expr);
> goto err;
> }
> -
> - if (i == 0)
> - node->expr = expr;
> - else
> - last->next = expr;
> - last = expr;
> }
>
> rc = cond_read_av_list(p, fp, &node->true_list, NULL);
> @@ -512,27 +502,23 @@ static int cond_write_av_list(struct policydb *p,
> static int cond_write_node(struct policydb *p, struct cond_node *node,
> struct policy_file *fp)
> {
> - struct cond_expr *cur_expr;
> __le32 buf[2];
> int rc;
> - u32 len = 0;
> + u32 i;
>
> buf[0] = cpu_to_le32(node->cur_state);
> rc = put_entry(buf, sizeof(u32), 1, fp);
> if (rc)
> return rc;
>
> - for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next)
> - len++;
> -
> - buf[0] = cpu_to_le32(len);
> + buf[0] = cpu_to_le32(node->expr.len);
> rc = put_entry(buf, sizeof(u32), 1, fp);
> if (rc)
> return rc;
>
> - for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next) {
> - buf[0] = cpu_to_le32(cur_expr->expr_type);
> - buf[1] = cpu_to_le32(cur_expr->bool);
> + for (i = 0; i < node->expr.len; i++) {
> + buf[0] = cpu_to_le32(node->expr.nodes[i].expr_type);
> + buf[1] = cpu_to_le32(node->expr.nodes[i].bool);
> rc = put_entry(buf, sizeof(u32), 2, fp);
> if (rc)
> return rc;
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index 5f97f678440e..4677c6ff7450 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -19,7 +19,7 @@
> * A conditional expression is a list of operators and operands
> * in reverse polish notation.
> */
> -struct cond_expr {
> +struct cond_expr_node {
> #define COND_BOOL 1 /* plain bool */
> #define COND_NOT 2 /* !bool */
> #define COND_OR 3 /* bool || bool */
> @@ -28,9 +28,13 @@ struct cond_expr {
> #define COND_EQ 6 /* bool == bool */
> #define COND_NEQ 7 /* bool != bool */
> #define COND_LAST COND_NEQ
> - __u32 expr_type;
> - __u32 bool;
> - struct cond_expr *next;
> + u32 expr_type;
> + u32 bool;
> +};
> +
> +struct cond_expr {
> + struct cond_expr_node *nodes;
> + u32 len;
> };
>
> /*
> @@ -52,7 +56,7 @@ struct cond_av_list {
> */
> struct cond_node {
> int cur_state;
> - struct cond_expr *expr;
> + struct cond_expr expr;
> struct cond_av_list true_list;
> struct cond_av_list false_list;
> };
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] selinux: generalize evaluate_cond_node()
2020-01-16 12:04 [PATCH 0/6] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (4 preceding siblings ...)
2020-01-16 12:04 ` [PATCH 5/6] selinux: convert cond_expr " Ondrej Mosnacek
@ 2020-01-16 12:04 ` Ondrej Mosnacek
2020-01-16 17:18 ` Stephen Smalley
2020-01-16 23:09 ` [PATCH 0/6] selinux: Assorted simplifications and cleanups Casey Schaufler
6 siblings, 1 reply; 21+ messages in thread
From: Ondrej Mosnacek @ 2020-01-16 12:04 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Both callers iterate the cond_list and call it for each node - turn it
into evaluate_cond_nodes(), which does the iteration for them.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/conditional.c | 10 +++++++++-
security/selinux/ss/conditional.h | 2 +-
security/selinux/ss/services.c | 6 ++----
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 8f9f2f3c86a0..ad709ccea036 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -86,7 +86,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
* list appropriately. If the result of the expression is undefined
* all of the rules are disabled for safety.
*/
-void evaluate_cond_node(struct policydb *p, struct cond_node *node)
+static void evaluate_cond_node(struct policydb *p, struct cond_node *node)
{
struct avtab_node *avnode;
int new_state;
@@ -117,6 +117,14 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
}
}
+void evaluate_cond_nodes(struct policydb *p)
+{
+ u32 i;
+
+ for (i = 0; i < p->cond_list_len; i++)
+ evaluate_cond_node(p, &p->cond_list[i]);
+}
+
int cond_policydb_init(struct policydb *p)
{
int rc;
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index 4677c6ff7450..b9eb888ffa76 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -78,6 +78,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
struct av_decision *avd, struct extended_perms *xperms);
void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key,
struct extended_perms_decision *xpermd);
-void evaluate_cond_node(struct policydb *p, struct cond_node *node);
+void evaluate_cond_nodes(struct policydb *p);
#endif /* _CONDITIONAL_H_ */
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b0f71afcf4b8..887331a0cc3c 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2954,8 +2954,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values)
policydb->bool_val_to_struct[i]->state = 0;
}
- for (i = 0; i < policydb->cond_list_len; i++)
- evaluate_cond_node(policydb, &policydb->cond_list[i]);
+ evaluate_cond_nodes(policydb);
seqno = ++state->ss->latest_granting;
rc = 0;
@@ -3006,8 +3005,7 @@ static void security_preserve_bools(struct policydb *oldpolicydb,
if (booldatum)
booldatum->state = value;
}
- for (i = 0; i < newpolicydb->cond_list_len; i++)
- evaluate_cond_node(newpolicydb, &newpolicydb->cond_list[i]);
+ evaluate_cond_nodes(newpolicydb);
}
/*
--
2.24.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 6/6] selinux: generalize evaluate_cond_node()
2020-01-16 12:04 ` [PATCH 6/6] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
@ 2020-01-16 17:18 ` Stephen Smalley
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Smalley @ 2020-01-16 17:18 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote:
> Both callers iterate the cond_list and call it for each node - turn it
> into evaluate_cond_nodes(), which does the iteration for them.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/conditional.c | 10 +++++++++-
> security/selinux/ss/conditional.h | 2 +-
> security/selinux/ss/services.c | 6 ++----
> 3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 8f9f2f3c86a0..ad709ccea036 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -86,7 +86,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
> * list appropriately. If the result of the expression is undefined
> * all of the rules are disabled for safety.
> */
> -void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> +static void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> {
> struct avtab_node *avnode;
> int new_state;
> @@ -117,6 +117,14 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> }
> }
>
> +void evaluate_cond_nodes(struct policydb *p)
> +{
> + u32 i;
> +
> + for (i = 0; i < p->cond_list_len; i++)
> + evaluate_cond_node(p, &p->cond_list[i]);
> +}
> +
> int cond_policydb_init(struct policydb *p)
> {
> int rc;
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index 4677c6ff7450..b9eb888ffa76 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -78,6 +78,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
> struct av_decision *avd, struct extended_perms *xperms);
> void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key,
> struct extended_perms_decision *xpermd);
> -void evaluate_cond_node(struct policydb *p, struct cond_node *node);
> +void evaluate_cond_nodes(struct policydb *p);
>
> #endif /* _CONDITIONAL_H_ */
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index b0f71afcf4b8..887331a0cc3c 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2954,8 +2954,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values)
> policydb->bool_val_to_struct[i]->state = 0;
> }
>
> - for (i = 0; i < policydb->cond_list_len; i++)
> - evaluate_cond_node(policydb, &policydb->cond_list[i]);
> + evaluate_cond_nodes(policydb);
>
> seqno = ++state->ss->latest_granting;
> rc = 0;
> @@ -3006,8 +3005,7 @@ static void security_preserve_bools(struct policydb *oldpolicydb,
> if (booldatum)
> booldatum->state = value;
> }
> - for (i = 0; i < newpolicydb->cond_list_len; i++)
> - evaluate_cond_node(newpolicydb, &newpolicydb->cond_list[i]);
> + evaluate_cond_nodes(newpolicydb);
> }
>
> /*
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] selinux: Assorted simplifications and cleanups
2020-01-16 12:04 [PATCH 0/6] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (5 preceding siblings ...)
2020-01-16 12:04 ` [PATCH 6/6] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
@ 2020-01-16 23:09 ` Casey Schaufler
2020-01-16 23:59 ` Paul Moore
6 siblings, 1 reply; 21+ messages in thread
From: Casey Schaufler @ 2020-01-16 23:09 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore; +Cc: Stephen Smalley
On 1/16/2020 4:04 AM, Ondrej Mosnacek wrote:
> This series contains some simplifications that I discovered while
> working on another patch. I believe they also save some run time
> (although not in any perf-critical paths) and some memory overhead.
>
> The first patch is a cleanup in security_load_policy()
It's a real nuisance that the security server code uses the
prefix "security_". If you're making significant changes in
the security server it would be really nice to clean up the
namespace collision.
> that avoids a
> pointless allocation during initial policy load. The rest are
> cleanups/simplifications of the booleans-related code - mostly
> converting linked lists to arrays.
>
> Ondrej Mosnacek (6):
> selinux: do not allocate ancillary buffer on first load
> selinux: simplify security_preserve_bools()
> selinux: convert cond_list to array
> selinux: convert cond_av_list to array
> selinux: convert cond_expr to array
> selinux: generalize evaluate_cond_node()
>
> security/selinux/include/conditional.h | 6 +-
> security/selinux/selinuxfs.c | 4 +-
> security/selinux/ss/conditional.c | 252 ++++++++++---------------
> security/selinux/ss/conditional.h | 27 +--
> security/selinux/ss/policydb.c | 2 +-
> security/selinux/ss/policydb.h | 3 +-
> security/selinux/ss/services.c | 95 ++++------
> 7 files changed, 160 insertions(+), 229 deletions(-)
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 0/6] selinux: Assorted simplifications and cleanups
2020-01-16 23:09 ` [PATCH 0/6] selinux: Assorted simplifications and cleanups Casey Schaufler
@ 2020-01-16 23:59 ` Paul Moore
2020-01-17 0:49 ` Casey Schaufler
0 siblings, 1 reply; 21+ messages in thread
From: Paul Moore @ 2020-01-16 23:59 UTC (permalink / raw)
To: Casey Schaufler; +Cc: Ondrej Mosnacek, selinux, Stephen Smalley
On Thu, Jan 16, 2020 at 6:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/16/2020 4:04 AM, Ondrej Mosnacek wrote:
> > This series contains some simplifications that I discovered while
> > working on another patch. I believe they also save some run time
> > (although not in any perf-critical paths) and some memory overhead.
> >
> > The first patch is a cleanup in security_load_policy()
>
> It's a real nuisance that the security server code uses the
> prefix "security_". If you're making significant changes in
> the security server it would be really nice to clean up the
> namespace collision.
For all the people lurking on the mailing list reading Casey's
response, *please* do not do this (without discussion). That change
has the potential to wreck a development cycle.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] selinux: Assorted simplifications and cleanups
2020-01-16 23:59 ` Paul Moore
@ 2020-01-17 0:49 ` Casey Schaufler
2020-01-17 0:56 ` Paul Moore
0 siblings, 1 reply; 21+ messages in thread
From: Casey Schaufler @ 2020-01-17 0:49 UTC (permalink / raw)
To: Paul Moore; +Cc: Ondrej Mosnacek, selinux, Stephen Smalley, Casey Schaufler
On 1/16/2020 3:59 PM, Paul Moore wrote:
> On Thu, Jan 16, 2020 at 6:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/16/2020 4:04 AM, Ondrej Mosnacek wrote:
>>> This series contains some simplifications that I discovered while
>>> working on another patch. I believe they also save some run time
>>> (although not in any perf-critical paths) and some memory overhead.
>>>
>>> The first patch is a cleanup in security_load_policy()
>> It's a real nuisance that the security server code uses the
>> prefix "security_". If you're making significant changes in
>> the security server it would be really nice to clean up the
>> namespace collision.
> For all the people lurking on the mailing list reading Casey's
> response, *please* do not do this (without discussion). That change
> has the potential to wreck a development cycle.
Of course discussion is critical, and breaking a development cycle
would be a Bad Thing. I only suggested this because I'm seeing a bit
of clean-up I would consider to be in the same vein. I was not
advocating disruption. Carry on.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] selinux: Assorted simplifications and cleanups
2020-01-17 0:49 ` Casey Schaufler
@ 2020-01-17 0:56 ` Paul Moore
0 siblings, 0 replies; 21+ messages in thread
From: Paul Moore @ 2020-01-17 0:56 UTC (permalink / raw)
To: Casey Schaufler; +Cc: Ondrej Mosnacek, selinux, Stephen Smalley
On Thu, Jan 16, 2020 at 7:49 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/16/2020 3:59 PM, Paul Moore wrote:
> > On Thu, Jan 16, 2020 at 6:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 1/16/2020 4:04 AM, Ondrej Mosnacek wrote:
> >>> This series contains some simplifications that I discovered while
> >>> working on another patch. I believe they also save some run time
> >>> (although not in any perf-critical paths) and some memory overhead.
> >>>
> >>> The first patch is a cleanup in security_load_policy()
> >> It's a real nuisance that the security server code uses the
> >> prefix "security_". If you're making significant changes in
> >> the security server it would be really nice to clean up the
> >> namespace collision.
> > For all the people lurking on the mailing list reading Casey's
> > response, *please* do not do this (without discussion). That change
> > has the potential to wreck a development cycle.
>
> Of course discussion is critical, and breaking a development cycle
> would be a Bad Thing. I only suggested this because I'm seeing a bit
> of clean-up I would consider to be in the same vein. I was not
> advocating disruption. Carry on.
FWIW, the cleanup you've seen lately has been mostly removing empty
wrapper functions and changing how we allocate/manage things; what you
are proposing is mostly a bulk rename which is quite different in my
mind.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread