| Message ID | 20200116120439.303034-2-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | selinux: Assorted simplifications and cleanups | expand |
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); >
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.
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); >
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.
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);
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(-)