Re: [PATCH v2] selinux: Add helper functions to get and set checkreqprot

[Stephen Smalley <stephen.smalley.work@gmail.com>]

On Fri, Sep 11, 2020 at 10:07 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Sep 9, 2020 at 6:28 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
> >
> > checkreqprot data member in selinux_state struct is accessed directly by
> > SELinux functions to get and set. This could cause unexpected read or
> > write access to this data member due to compiler optimizations and/or
> > compiler's reordering of access to this field.
> >
> > Add helper functions to get and set checkreqprot data member in
> > selinux_state struct. These helper functions use READ_ONCE and
> > WRITE_ONCE macros to ensure atomic read or write of memory for
> > this data member.
> >
> > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > ---
> >  security/selinux/hooks.c            |  6 +++---
> >  security/selinux/include/security.h | 10 ++++++++++
> >  security/selinux/selinuxfs.c        |  5 +++--
> >  3 files changed, 16 insertions(+), 5 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> > index cbdd3c7aff8b..cc29177c8858 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -143,6 +143,16 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
> >  }
> >  #endif
> >
> > +static inline bool checkreqprot_enabled(const struct selinux_state *state)
> > +{
> > +       return READ_ONCE(state->checkreqprot);
> > +}
> > +
> > +static inline void checkreqprot_set(struct selinux_state *state, bool value)
> > +{
> > +       WRITE_ONCE(state->checkreqprot, value);
> > +}
>
> This is a nitpick, and I recognize that Stephen already suggested the
> use of "*_set()" and "*_enabled()" for names, but if we are going to
> name the setter "*_set()" let's also name the getter "*_get()".

I just suggested that we be consistent with the existing naming for
enforcing_*(), which I thought came from you?