On Mon, Sep 14, 2020 at 9:25 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Fri, Sep 11, 2020 at 12:40 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.
> >
> > Rename enforcing_enabled() to enforcing_get() to be consistent
> > with the corresponding set function name.
>
> I thought Paul said to only use the new names for checkreqprot_*() and
> not to touch enforcing_*()? I don't really care either way about the
> names but usually we wouldn't mix renaming of something else with the
> introduction of these new helpers in a single patch.
It's generally a good idea when someone has provided feedback on a
patch to limit the changes in the next revision to just those changes
which have been suggested. It helps to keep the patch focused on the
original issue, makes the follow-up reviews easier, and shortens the
develop/reverge/merge cycle. It generally isn't too bad of a problem
in the SELinux code, but there are other subsystems where several
large patch revisions have been wasted because the patch author lost
focus and started making additional changes outside the scope of the
comments. If you want to submit the additional changes anyway, my
recommendation would be to split them out into a second patch in a
patch series (make sure the original patch is first!).
Of course the best solution is always to ask if you are unsure :)
While I don't check my upstream email quite as often as some folks
here, I promise to respond to any follow-up questions if no one
answers first.
... and please don't let our small nitpicks discourage you from
submitting patches, we really do appreciate help and contributions :)
> FWIW, looking at the history, the enforcing functions were originally
> named is_enforcing() and set_enforcing() in aa8e712cee93d520e96a2ca8
> ("selinux: wrap global selinux state") . Then Paul renamed them to
> enforcing_enabled() and enforcing_set() in e5a5ca96a42ca7eee19cf869
> ("selinux: rename the {is,set}_enforcing() functions").
--
paul moore
www.paul-moore.com
