On Tue, Aug 25, 2020 at 5:01 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Tue, Aug 25, 2020 at 3:00 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > There seems to be no reason to use GFP_ATOMIC in these cases.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > 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.
