| Message ID | 20200826135906.1912186-2-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: RCU conversion follow-ups | expand |
On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > Remove the security_policydb_len() calls from sel_open_policy() and > instead update the inode size from the size returned from > security_read_policy(). > > Since after this change security_policydb_len() is only called from > security_load_policy(), remove it entirely and just open-code it there. > > Also, since security_load_policy() is always called with fsi->mutex > held, make it dereference the policy pointer directly and drop the > unnecessary RCU locking. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> One comment below but nonetheless: Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 8381614627569..7cc2f7486c18f 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -3912,11 +3896,17 @@ int security_read_policy(struct selinux_state *state, > int rc; > struct policy_file fp; > > - if (!selinux_initialized(state)) > + /* > + * 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. > + */ > + policy = rcu_dereference_check(state->policy, 1); > + if (!policy) > return -EINVAL; If/when my patch to move the mutex to selinux_state and use it in rcu_dereference_protected() lands, we'll want to change this one over too.
On Wed, Aug 26, 2020 at 10:05 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > Remove the security_policydb_len() calls from sel_open_policy() and > > instead update the inode size from the size returned from > > security_read_policy(). > > > > Since after this change security_policydb_len() is only called from > > security_load_policy(), remove it entirely and just open-code it there. > > > > Also, since security_load_policy() is always called with fsi->mutex > > held, make it dereference the policy pointer directly and drop the > > unnecessary RCU locking. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > One comment below but nonetheless: > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index 8381614627569..7cc2f7486c18f 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -3912,11 +3896,17 @@ int security_read_policy(struct selinux_state *state, > > int rc; > > struct policy_file fp; > > > > - if (!selinux_initialized(state)) > > + /* > > + * 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. > > + */ > > + policy = rcu_dereference_check(state->policy, 1); > > + if (!policy) > > return -EINVAL; > > If/when my patch to move the mutex to selinux_state and use it in > rcu_dereference_protected() lands, we'll want to change this one over > too. FWIW, I felt the mutex move was more significant than this patchset so I merged it first. Ondrej, would you mind rebasing this patch and making the changes above? Thanks.
On Thu, Aug 27, 2020 at 9:56 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Aug 26, 2020 at 10:05 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > Remove the security_policydb_len() calls from sel_open_policy() and > > > instead update the inode size from the size returned from > > > security_read_policy(). > > > > > > Since after this change security_policydb_len() is only called from > > > security_load_policy(), remove it entirely and just open-code it there. > > > > > > Also, since security_load_policy() is always called with fsi->mutex > > > held, make it dereference the policy pointer directly and drop the > > > unnecessary RCU locking. > > > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > > > One comment below but nonetheless: > > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > > index 8381614627569..7cc2f7486c18f 100644 > > > --- a/security/selinux/ss/services.c > > > +++ b/security/selinux/ss/services.c > > > @@ -3912,11 +3896,17 @@ int security_read_policy(struct selinux_state *state, > > > int rc; > > > struct policy_file fp; > > > > > > - if (!selinux_initialized(state)) > > > + /* > > > + * 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. > > > + */ > > > + policy = rcu_dereference_check(state->policy, 1); > > > + if (!policy) > > > return -EINVAL; > > > > If/when my patch to move the mutex to selinux_state and use it in > > rcu_dereference_protected() lands, we'll want to change this one over > > too. > > FWIW, I felt the mutex move was more significant than this patchset so > I merged it first. Ondrej, would you mind rebasing this patch and > making the changes above? Oh, just in case it wasn't clear from my comments above, I think this patch is fine :)
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 505e51264d51c..839774929a10d 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -218,7 +218,6 @@ void selinux_policy_cancel(struct selinux_state *state, struct selinux_policy *policy); int security_read_policy(struct selinux_state *state, void **data, size_t *len); -size_t security_policydb_len(struct selinux_state *state); int security_policycap_supported(struct selinux_state *state, unsigned int req_cap); diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index d1872adf0c474..fc44c4b8c0692 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -417,16 +417,16 @@ static int sel_open_policy(struct inode *inode, struct file *filp) if (!plm) goto err; - if (i_size_read(inode) != security_policydb_len(state)) { - inode_lock(inode); - i_size_write(inode, security_policydb_len(state)); - inode_unlock(inode); - } - rc = security_read_policy(state, &plm->data, &plm->len); if (rc) goto err; + if ((size_t)i_size_read(inode) != plm->len) { + inode_lock(inode); + i_size_write(inode, plm->len); + inode_unlock(inode); + } + fsi->policy_opened = 1; filp->private_data = plm; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 8381614627569..7cc2f7486c18f 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2331,22 +2331,6 @@ err: return rc; } -size_t security_policydb_len(struct selinux_state *state) -{ - struct selinux_policy *policy; - size_t len; - - if (!selinux_initialized(state)) - return 0; - - rcu_read_lock(); - policy = rcu_dereference(state->policy); - len = policy->policydb.len; - rcu_read_unlock(); - - return len; -} - /** * security_port_sid - Obtain the SID for a port. * @protocol: protocol number @@ -3912,11 +3896,17 @@ int security_read_policy(struct selinux_state *state, int rc; struct policy_file fp; - if (!selinux_initialized(state)) + /* + * 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. + */ + policy = rcu_dereference_check(state->policy, 1); + if (!policy) return -EINVAL; - *len = security_policydb_len(state); - + *len = policy->policydb.len; *data = vmalloc_user(*len); if (!*data) return -ENOMEM; @@ -3924,11 +3914,7 @@ int security_read_policy(struct selinux_state *state, fp.data = *data; fp.len = *len; - rcu_read_lock(); - policy = rcu_dereference(state->policy); rc = policydb_write(&policy->policydb, &fp); - rcu_read_unlock(); - if (rc) return rc;
Remove the security_policydb_len() calls from sel_open_policy() and instead update the inode size from the size returned from security_read_policy(). Since after this change security_policydb_len() is only called from security_load_policy(), remove it entirely and just open-code it there. Also, since security_load_policy() is always called with fsi->mutex held, make it dereference the policy pointer directly and drop the unnecessary RCU locking. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/include/security.h | 1 - security/selinux/selinuxfs.c | 12 +++++------ security/selinux/ss/services.c | 32 ++++++++--------------------- 3 files changed, 15 insertions(+), 30 deletions(-)