On Tue, Aug 25, 2020 at 9:15 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > 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. One alternative would be to move the mutex from selinux_fs_info to selinux_state, at which point the mutex would already be accessible to the security server code through the state parameters. This also makes sense from the perspective that the mutex is already used to synchronize not only selinuxfs-private state (e.g. pending bools) but also policy changes. I think this will be needed anyway for the patches to measure SELinux state because that call chain does not go through selinuxfs and thus has no access to selinux_fs_info. > > > > 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). I don't suppose you have editor settings to help automate this?
