On Fri, Aug 21, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > On Thu, Aug 20, 2020 at 4:19 PM Stephen Smalley > <stephen.smalley.work@xxxxxxxxx> wrote: > > In the previous change to convert the policy rwlock to RCU, > > the update code was using rcu_dereference_check(..., 1) with > > a comment to explain why it was safe without taking rcu_read_lock() > > since the mutex used to provide exclusion was taken at a higher > > level in selinuxfs. This change passes the mutex down to the > > necessary functions and replaces rcu_dereference_check(..., 1) > > with rcu_dereference_protected(..., lockdep_is_held(mutex)) so > > that lockdep checking is correctly applied and the dependency > > is made explicit in the code rather than relying on comments. > > > > Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> > > --- > > This is relative to the convert policy read-write lock to RCU patch, > > https://patchwork.kernel.org/patch/11724997/. > > > > security/selinux/include/conditional.h | 3 +- > > security/selinux/include/security.h | 6 ++-- > > security/selinux/selinuxfs.c | 12 ++++--- > > security/selinux/ss/services.c | 45 ++++++++------------------ > > 4 files changed, 26 insertions(+), 40 deletions(-) > > Thanks for trying this out! I indeed like it more this way. The only > thing I'd suggest is to perhaps name the mutex argument a little more > descriptively, e.g. "check_mutex" or "rcu_mutex". I understand it'll > make it harder to wrap some of the long lines, but I tend to think > it's worth it in this case. I considered calling it policy_mutex but wasn't sure it was necessary/worthwhile and also thought it might be confusing (obvious question becomes why isn't that mutex part of struct selinux_policy, but that's a layering thing). I'll wait to see what name Paul prefers before spinning another patch. > Speaking about wrapping lines... I noticed only now that in this and > earlier patches you align wrapped argument lists only by tabs (without > extra spaces to align to the first argument). I'm not sure what is the > preferred kernel style in this case, but I personally find the finely > aligned argument lists much nicer to read (and I have always been > aligning them like this in my patches). Obviously, I can't enforce my > preferred style here, but I thought I'd raise this, since I had the > impression we were trying to follow this style previously for new code > (could be just confirmation bias on my part, though) and it might not > have been your intention to change it (changed editor/settings?). I'm using the emacs mode settings from Documentation/process/codingstyle.rst. I don't see anything in the coding style document to suggest use of extra spaces for aligned argument lists; if anything use of spaces rather than tabs for indentation seems discouraged. I don't really care either way but would like editor settings to ensure consistency.
