On Fri, Aug 21, 2020 at 8:22 AM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > 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. As I mentioned in the RCU patch thread, my preference at this point in time is to address this with comments and not pass the mutex into the security server. > > 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. FWIW, my preference is for aligned argument lists, for example: void write_program(char *language, char *description); ... with the understanding that tabs are used as much as possible and that spaces are only used to make up the difference when the gap is less than a tab (8 chars). However, I don't feel quite as strongly about this as other things, e.g. 80 char line widths, so as long as the code passes checkpatch.pl I'll merge it regardless of the argument alignment. -- paul moore www.paul-moore.com
