On Fri, Sep 11, 2020 at 10:23 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Fri, Sep 11, 2020 at 10:07 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Wed, Sep 9, 2020 at 6:28 PM Lakshmi Ramasubramanian
> > <nramas@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > > Suggested-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> > > ---
> > > 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?
I vaguely remember having a discussion around the naming back then,
but I can't recall who preferred what and when. I'm sure it's all in
the archives for anyone who cares to look.
Regardless, I think _set() and _get() make the most sense here. I can
also see an argument being made for "_enabled()" on some bigger flags.
My personal opinion is that the SELinux kernel code, and kernel code
in general, is a bit of a mess when it comes to naming consistency.
Considering the age, size, and number of contributors the current
state really shouldn't be too surprising (and honestly it isn't that
bad considering everything). Perhaps someday we can look at that, but
that is so far down the priority list it isn't worth discussing; I'd
tackle the coding style inconsistencies before this.
--
paul moore
www.paul-moore.com
