| Message ID | 20200825065953.1566718-2-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: Clean up GFP flag usage | expand |
On Tue, Aug 25, 2020 at 3:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > There seems to be no reason to use GFP_ATOMIC in these cases. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/hooks.c | 6 +++--- > security/selinux/ss/policydb.c | 10 +++++----- > security/selinux/ss/services.c | 4 ++-- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 89d3753b7bd5d..4de962daffbde 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6854,7 +6854,7 @@ static int selinux_lockdown(enum lockdown_reason what) > > if (WARN(invalid_reason, "Invalid lockdown reason")) { > audit_log(audit_context(), > - GFP_ATOMIC, AUDIT_SELINUX_ERR, > + GFP_KERNEL, AUDIT_SELINUX_ERR, > "lockdown_reason=invalid"); > return -EINVAL; > } Have you audited all callers of security_locked_down() to ensure that they are never holding any locks around the call? That's the only one I saw that might be a problem now or in the future. > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index a48fc1b337ba9..fa61a54bc1440 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -109,7 +109,7 @@ static int selinux_set_mapping(struct policydb *pol, > @@ -2982,7 +2982,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values) > int old_state = newpolicy->policydb.bool_val_to_struct[i]->state; > > if (new_state != old_state) { > - audit_log(audit_context(), GFP_ATOMIC, > + audit_log(audit_context(), GFP_KERNEL, > AUDIT_MAC_CONFIG_CHANGE, > "bool=%s val=%d old_val=%d auid=%u ses=%u", > sym_name(&newpolicy->policydb, SYM_BOOLS, i), Note that this one shouldn't be back-ported prior to my refactoring patches because previously this was called while holding the policy rwlock.
On Tue, Aug 25, 2020 at 5:01 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Tue, Aug 25, 2020 at 3:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > There seems to be no reason to use GFP_ATOMIC in these cases. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > security/selinux/hooks.c | 6 +++--- > > security/selinux/ss/policydb.c | 10 +++++----- > > security/selinux/ss/services.c | 4 ++-- > > 3 files changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 89d3753b7bd5d..4de962daffbde 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -6854,7 +6854,7 @@ static int selinux_lockdown(enum lockdown_reason what) > > > > if (WARN(invalid_reason, "Invalid lockdown reason")) { > > audit_log(audit_context(), > > - GFP_ATOMIC, AUDIT_SELINUX_ERR, > > + GFP_KERNEL, AUDIT_SELINUX_ERR, > > "lockdown_reason=invalid"); > > return -EINVAL; > > } > > Have you audited all callers of security_locked_down() to ensure that > they are never holding any locks around the call? That's the only one > I saw that might be a problem now or in the future. Hm... didn't realize there were so many :) I'll try to go over them. As a side note, it would be nice if we had the may (not) sleep requirements documented somehow for each hook at the LSM level, ideally with a might_sleep() check at the beginning of each secuity_*() function that is expected to be called only from a non-atomic context... Some hooks already have arguments that specify this (inode_permission, inode_follow_link), but for others it is just implicit. I'll put this on my long-term TODO list... -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
On Tue, Aug 25, 2020 at 12:45 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Tue, Aug 25, 2020 at 5:01 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 3:00 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > There seems to be no reason to use GFP_ATOMIC in these cases. > > > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > > --- > > > security/selinux/hooks.c | 6 +++--- > > > security/selinux/ss/policydb.c | 10 +++++----- > > > security/selinux/ss/services.c | 4 ++-- > > > 3 files changed, 10 insertions(+), 10 deletions(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index 89d3753b7bd5d..4de962daffbde 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -6854,7 +6854,7 @@ static int selinux_lockdown(enum lockdown_reason what) > > > > > > if (WARN(invalid_reason, "Invalid lockdown reason")) { > > > audit_log(audit_context(), > > > - GFP_ATOMIC, AUDIT_SELINUX_ERR, > > > + GFP_KERNEL, AUDIT_SELINUX_ERR, > > > "lockdown_reason=invalid"); > > > return -EINVAL; > > > } > > > > Have you audited all callers of security_locked_down() to ensure that > > they are never holding any locks around the call? That's the only one > > I saw that might be a problem now or in the future. > > Hm... didn't realize there were so many :) I'll try to go over them. > > As a side note, it would be nice if we had the may (not) sleep > requirements documented somehow for each hook at the LSM level, > ideally with a might_sleep() check at the beginning of each > secuity_*() function that is expected to be called only from a > non-atomic context... Some hooks already have arguments that specify > this (inode_permission, inode_follow_link), but for others it is just > implicit. I'll put this on my long-term TODO list... Generally the core SELinux permission checking code assumes it can be called from any context (including hardirq) and under any locking conditions, and hooks that are for permission checking (not security blob allocation/management) do the same. This allows permission checks to be performed while locks are held by the caller. and from arbitrary contexts I'd be inclined to just leave selinux_lockdown unchanged given that it might be called anywhere much like capable().
On Wed, Aug 26, 2020 at 9:02 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > Generally the core SELinux permission checking code assumes it can be > called from any context (including hardirq) and under any locking > conditions, and hooks that are for permission checking (not security > blob allocation/management) do the same. This allows permission > checks to be performed while locks are held by the caller. and from > arbitrary contexts I'd be inclined to just leave selinux_lockdown > unchanged given that it might be called anywhere much like capable(). Agreed. The code paths relating to policy load, etc. are good candidates for an ATOMIC->KERNEL conversion, assuming no other constraints, but I'm somewhat nervous of converting stuff in hook.c that doesn't have a hard sleep-ability defined. The other question about this patchset is motivation: were you seeing problem reports relating to SELinux memory failures when the system was under pressure, or is this preemptive? -- paul moore www.paul-moore.com
On Wed, Aug 26, 2020 at 3:51 PM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Aug 26, 2020 at 9:02 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > Generally the core SELinux permission checking code assumes it can be > > called from any context (including hardirq) and under any locking > > conditions, and hooks that are for permission checking (not security > > blob allocation/management) do the same. This allows permission > > checks to be performed while locks are held by the caller. and from > > arbitrary contexts I'd be inclined to just leave selinux_lockdown > > unchanged given that it might be called anywhere much like capable(). > > Agreed. The code paths relating to policy load, etc. are good > candidates for an ATOMIC->KERNEL conversion, assuming no other > constraints, but I'm somewhat nervous of converting stuff in hook.c > that doesn't have a hard sleep-ability defined. OK, I can drop the selinux_lockdown() hunk then. The other hooks.c hunks both have an existing GFP_KERNEL allocation in the same function, so I'd tend to keep them for consistency. Or do you want me to rather convert the GFP_KERNELs to GFP_ATOMICs there? > > The other question about this patchset is motivation: were you seeing > problem reports relating to SELinux memory failures when the system > was under pressure, or is this preemptive? No, I just went over the GFP_* usage in security/selinux/ when scoping how many GFP_ATOMIC ones could be changed to GFP_KERNEL after the refcount patch and found these that could be switched right away. -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
On Wed, Aug 26, 2020 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Wed, Aug 26, 2020 at 3:51 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Aug 26, 2020 at 9:02 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > Generally the core SELinux permission checking code assumes it can be > > > called from any context (including hardirq) and under any locking > > > conditions, and hooks that are for permission checking (not security > > > blob allocation/management) do the same. This allows permission > > > checks to be performed while locks are held by the caller. and from > > > arbitrary contexts I'd be inclined to just leave selinux_lockdown > > > unchanged given that it might be called anywhere much like capable(). > > > > Agreed. The code paths relating to policy load, etc. are good > > candidates for an ATOMIC->KERNEL conversion, assuming no other > > constraints, but I'm somewhat nervous of converting stuff in hook.c > > that doesn't have a hard sleep-ability defined. > > OK, I can drop the selinux_lockdown() hunk then. The other hooks.c > hunks both have an existing GFP_KERNEL allocation in the same > function, so I'd tend to keep them for consistency. Or do you want me > to rather convert the GFP_KERNELs to GFP_ATOMICs there? No, I was fine with the other parts of the patch just not the lockdown one, because it can be called from many places like capable().
On Wed, Aug 26, 2020 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Wed, Aug 26, 2020 at 3:51 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Aug 26, 2020 at 9:02 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > Generally the core SELinux permission checking code assumes it can be > > > called from any context (including hardirq) and under any locking > > > conditions, and hooks that are for permission checking (not security > > > blob allocation/management) do the same. This allows permission > > > checks to be performed while locks are held by the caller. and from > > > arbitrary contexts I'd be inclined to just leave selinux_lockdown > > > unchanged given that it might be called anywhere much like capable(). > > > > Agreed. The code paths relating to policy load, etc. are good > > candidates for an ATOMIC->KERNEL conversion, assuming no other > > constraints, but I'm somewhat nervous of converting stuff in hook.c > > that doesn't have a hard sleep-ability defined. > > OK, I can drop the selinux_lockdown() hunk then. The other hooks.c > hunks both have an existing GFP_KERNEL allocation in the same > function, so I'd tend to keep them for consistency. Or do you want me > to rather convert the GFP_KERNELs to GFP_ATOMICs there? I was speaking generically in that I'm not comfortable converting ATOMIC allocations to KERNEL allocations in the hooks.c layer without some guarantee/constraint. In the case of both _setxattr and _setprocattr we are already doing KERNEL allocations in those functions so switching the audit allocations to KERNEL isn't necessarily changing the behavior of the SELinux hook, it can sleep now regardless. > > The other question about this patchset is motivation: were you seeing > > problem reports relating to SELinux memory failures when the system > > was under pressure, or is this preemptive? > > No, I just went over the GFP_* usage in security/selinux/ when scoping > how many GFP_ATOMIC ones could be changed to GFP_KERNEL after the > refcount patch and found these that could be switched right away. Okay, thanks.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 89d3753b7bd5d..4de962daffbde 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3171,7 +3171,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, audit_size = 0; } ab = audit_log_start(audit_context(), - GFP_ATOMIC, AUDIT_SELINUX_ERR); + GFP_KERNEL, AUDIT_SELINUX_ERR); audit_log_format(ab, "op=setxattr invalid_context="); audit_log_n_untrustedstring(ab, value, audit_size); audit_log_end(ab); @@ -6388,7 +6388,7 @@ static int selinux_setprocattr(const char *name, void *value, size_t size) else audit_size = size; ab = audit_log_start(audit_context(), - GFP_ATOMIC, + GFP_KERNEL, AUDIT_SELINUX_ERR); audit_log_format(ab, "op=fscreate invalid_context="); audit_log_n_untrustedstring(ab, value, audit_size); @@ -6854,7 +6854,7 @@ static int selinux_lockdown(enum lockdown_reason what) if (WARN(invalid_reason, "Invalid lockdown reason")) { audit_log(audit_context(), - GFP_ATOMIC, AUDIT_SELINUX_ERR, + GFP_KERNEL, AUDIT_SELINUX_ERR, "lockdown_reason=invalid"); return -EINVAL; } diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 9fccf417006b0..c1437de04e1d9 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1577,7 +1577,7 @@ static int sens_read(struct policydb *p, struct symtab *s, void *fp) __le32 buf[2]; u32 len; - levdatum = kzalloc(sizeof(*levdatum), GFP_ATOMIC); + levdatum = kzalloc(sizeof(*levdatum), GFP_KERNEL); if (!levdatum) return -ENOMEM; @@ -1588,12 +1588,12 @@ static int sens_read(struct policydb *p, struct symtab *s, void *fp) len = le32_to_cpu(buf[0]); levdatum->isalias = le32_to_cpu(buf[1]); - rc = str_read(&key, GFP_ATOMIC, fp, len); + rc = str_read(&key, GFP_KERNEL, fp, len); if (rc) goto bad; rc = -ENOMEM; - levdatum->level = kmalloc(sizeof(*levdatum->level), GFP_ATOMIC); + levdatum->level = kmalloc(sizeof(*levdatum->level), GFP_KERNEL); if (!levdatum->level) goto bad; @@ -1618,7 +1618,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp) __le32 buf[3]; u32 len; - catdatum = kzalloc(sizeof(*catdatum), GFP_ATOMIC); + catdatum = kzalloc(sizeof(*catdatum), GFP_KERNEL); if (!catdatum) return -ENOMEM; @@ -1630,7 +1630,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp) catdatum->value = le32_to_cpu(buf[1]); catdatum->isalias = le32_to_cpu(buf[2]); - rc = str_read(&key, GFP_ATOMIC, fp, len); + rc = str_read(&key, GFP_KERNEL, fp, len); if (rc) goto bad; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index a48fc1b337ba9..fa61a54bc1440 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -109,7 +109,7 @@ static int selinux_set_mapping(struct policydb *pol, i++; /* Allocate space for the class records, plus one for class zero */ - out_map->mapping = kcalloc(++i, sizeof(*out_map->mapping), GFP_ATOMIC); + out_map->mapping = kcalloc(++i, sizeof(*out_map->mapping), GFP_KERNEL); if (!out_map->mapping) return -ENOMEM; @@ -2982,7 +2982,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values) int old_state = newpolicy->policydb.bool_val_to_struct[i]->state; if (new_state != old_state) { - audit_log(audit_context(), GFP_ATOMIC, + audit_log(audit_context(), GFP_KERNEL, AUDIT_MAC_CONFIG_CHANGE, "bool=%s val=%d old_val=%d auid=%u ses=%u", sym_name(&newpolicy->policydb, SYM_BOOLS, i),
There seems to be no reason to use GFP_ATOMIC in these cases. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/hooks.c | 6 +++--- security/selinux/ss/policydb.c | 10 +++++----- security/selinux/ss/services.c | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-)