On 9/14/20 8:04 AM, Paul Moore wrote:
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!).
Sorry about that - I will resubmit the patch with the change to rename
only checkreqprot helper function.
-lakshmi
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").
