| Message ID | 20200820141850.60244-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC] selinux: enable proper lockdep checking for policy rcu access | expand |
On Thu, Aug 20, 2020 at 4:19 PM Stephen Smalley <stephen.smalley.work@gmail.com> 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@gmail.com> > --- > 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. 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?).
On Fri, Aug 21, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Aug 20, 2020 at 4:19 PM Stephen Smalley > <stephen.smalley.work@gmail.com> 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@gmail.com> > > --- > > 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.
On Fri, Aug 21, 2020 at 8:22 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Fri, Aug 21, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Aug 20, 2020 at 4:19 PM Stephen Smalley > > <stephen.smalley.work@gmail.com> 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@gmail.com> > > > --- > > > 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.
On Tue, Aug 25, 2020 at 9:15 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Aug 21, 2020 at 8:22 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Fri, Aug 21, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Thu, Aug 20, 2020 at 4:19 PM Stephen Smalley > > > <stephen.smalley.work@gmail.com> 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@gmail.com> > > > > --- > > > > 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?
On Tue, Aug 25, 2020 at 10:06 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 9:15 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Fri, Aug 21, 2020 at 8:22 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > On Fri, Aug 21, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Thu, Aug 20, 2020 at 4:19 PM Stephen Smalley > > > > <stephen.smalley.work@gmail.com> wrote: ... > > 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. That seems reasonable to me. > > > > 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? Not really, but some of that is simply because I tend to bounce around between editors depending on what type of work I'm doing. My fingers have more or less gotten used to it and do the right thing as a matter of habit these days. In the non-kernel projects I maintain there is a script which you can run that checks the formatting/style, and optionally fixes it for you (in the case of C, it's a wrapper around astyle); I lean on that a lot for those projects. It would be nice to have something like that here, but we would need to do a lot of style fixes first. I keep threatening to do that, but it never quite seems worth it; perhaps I should start doing that slowly.
diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h index b09343346e3f..4659193dc49d 100644 --- a/security/selinux/include/conditional.h +++ b/security/selinux/include/conditional.h @@ -16,7 +16,8 @@ int security_get_bools(struct selinux_policy *policy, u32 *len, char ***names, int **values); -int security_set_bools(struct selinux_state *state, u32 len, int *values); +int security_set_bools(struct selinux_state *state, struct mutex *mutex, + u32 len, int *values); int security_get_bool_value(struct selinux_state *state, u32 index); diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 505e51264d51..87eac1f2e6ed 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -209,12 +209,12 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void) } int security_mls_enabled(struct selinux_state *state); -int security_load_policy(struct selinux_state *state, +int security_load_policy(struct selinux_state *state, struct mutex *mutex, void *data, size_t len, struct selinux_policy **newpolicyp); -void selinux_policy_commit(struct selinux_state *state, +void selinux_policy_commit(struct selinux_state *state, struct mutex *mutex, struct selinux_policy *newpolicy); -void selinux_policy_cancel(struct selinux_state *state, +void selinux_policy_cancel(struct selinux_state *state, struct mutex *mutex, struct selinux_policy *policy); int security_read_policy(struct selinux_state *state, void **data, size_t *len); diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 131816878e50..a054683359dd 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -560,7 +560,8 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, if (copy_from_user(data, buf, count) != 0) goto out; - length = security_load_policy(fsi->state, data, count, &newpolicy); + length = security_load_policy(fsi->state, &fsi->mutex, data, count, + &newpolicy); if (length) { pr_warn_ratelimited("SELinux: failed to load policy\n"); goto out; @@ -568,11 +569,11 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, length = sel_make_policy_nodes(fsi, newpolicy); if (length) { - selinux_policy_cancel(fsi->state, newpolicy); + selinux_policy_cancel(fsi->state, &fsi->mutex, newpolicy); goto out1; } - selinux_policy_commit(fsi->state, newpolicy); + selinux_policy_commit(fsi->state, &fsi->mutex, newpolicy); length = count; @@ -1309,8 +1310,9 @@ static ssize_t sel_commit_bools_write(struct file *filep, length = 0; if (new_value && fsi->bool_pending_values) - length = security_set_bools(fsi->state, fsi->bool_num, - fsi->bool_pending_values); + length = security_set_bools(fsi->state, &fsi->mutex, + fsi->bool_num, + fsi->bool_pending_values); if (!length) length = count; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 838161462756..a9fff3592768 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2159,17 +2159,13 @@ static void selinux_policy_cond_free(struct selinux_policy *policy) } void selinux_policy_cancel(struct selinux_state *state, + struct mutex *mutex, struct selinux_policy *policy) { struct selinux_policy *oldpolicy; - /* - * NOTE: We do not need to take the rcu read lock - * around the code below because other policy-modifying - * operations are already excluded by selinuxfs via - * fsi->mutex. - */ - oldpolicy = rcu_dereference_check(state->policy, 1); + oldpolicy = rcu_dereference_protected(state->policy, + lockdep_is_held(mutex)); sidtab_cancel_convert(oldpolicy->sidtab); selinux_policy_free(policy); @@ -2187,18 +2183,14 @@ static void selinux_notify_policy_change(struct selinux_state *state, } void selinux_policy_commit(struct selinux_state *state, + struct mutex *mutex, struct selinux_policy *newpolicy) { struct selinux_policy *oldpolicy; u32 seqno; - /* - * NOTE: We do not need to take the rcu read lock - * around the code below because other policy-modifying - * operations are already excluded by selinuxfs via - * fsi->mutex. - */ - oldpolicy = rcu_dereference_check(state->policy, 1); + oldpolicy = rcu_dereference_protected(state->policy, + lockdep_is_held(mutex)); /* If switching between different policy types, log MLS status */ if (oldpolicy) { @@ -2249,7 +2241,8 @@ void selinux_policy_commit(struct selinux_state *state, * This function will flush the access vector cache after * loading the new policy. */ -int security_load_policy(struct selinux_state *state, void *data, size_t len, +int security_load_policy(struct selinux_state *state, struct mutex *mutex, + void *data, size_t len, struct selinux_policy **newpolicyp) { struct selinux_policy *newpolicy, *oldpolicy; @@ -2289,13 +2282,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len, return 0; } - /* - * NOTE: We do not need to take the rcu read lock - * around the code below because other policy-modifying - * operations are already excluded by selinuxfs via - * fsi->mutex. - */ - oldpolicy = rcu_dereference_check(state->policy, 1); + oldpolicy = rcu_dereference_protected(state->policy, + lockdep_is_held(mutex)); /* Preserve active boolean values from the old policy */ rc = security_preserve_bools(oldpolicy, newpolicy); @@ -2992,7 +2980,8 @@ int security_get_bools(struct selinux_policy *policy, } -int security_set_bools(struct selinux_state *state, u32 len, int *values) +int security_set_bools(struct selinux_state *state, struct mutex *mutex, + u32 len, int *values) { struct selinux_policy *newpolicy, *oldpolicy; int rc; @@ -3001,14 +2990,8 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values) if (!selinux_initialized(state)) return -EINVAL; - /* - * NOTE: We do not need to take the rcu read lock - * around the code below because other policy-modifying - * operations are already excluded by selinuxfs via - * fsi->mutex. - */ - - oldpolicy = rcu_dereference_check(state->policy, 1); + oldpolicy = rcu_dereference_protected(state->policy, + lockdep_is_held(mutex)); /* Consistency check on number of booleans, should never fail */ if (WARN_ON(len != oldpolicy->policydb.p_bools.nprim))
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@gmail.com> --- 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(-)