| Message ID | 20200825152045.1719298-2-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: RCU conversion follow-ups | expand |
On Tue, Aug 25, 2020 at 11:20 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. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 8381614627569..ec4570d6c38f7 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -3915,7 +3899,10 @@ int security_read_policy(struct selinux_state *state, > if (!selinux_initialized(state)) > return -EINVAL; > > - *len = security_policydb_len(state); > + rcu_read_lock(); > + policy = rcu_dereference(state->policy); > + *len = policy->policydb.len; > + rcu_read_unlock(); > > *data = vmalloc_user(*len); > if (!*data) We don't actually need to take rcu_read_lock() here at all, and can use rcu_dereference_check(..., 1) or rcu_deference_protected() because the fsi->mutex is held. We should also fix the code immediately below this diff context to not double dereference the policy pointer and just reuse the already dereferenced pointer (also not requiring rcu_read_lock).
On Tue, Aug 25, 2020 at 6:03 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 11:20 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. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index 8381614627569..ec4570d6c38f7 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -3915,7 +3899,10 @@ int security_read_policy(struct selinux_state *state, > > if (!selinux_initialized(state)) > > return -EINVAL; > > > > - *len = security_policydb_len(state); > > + rcu_read_lock(); > > + policy = rcu_dereference(state->policy); > > + *len = policy->policydb.len; > > + rcu_read_unlock(); > > > > *data = vmalloc_user(*len); > > if (!*data) > > We don't actually need to take rcu_read_lock() here at all, and can > use rcu_dereference_check(..., 1) or rcu_deference_protected() because > the fsi->mutex is held. We should also fix the code immediately below > this diff context to not double dereference the policy pointer and > just reuse the already dereferenced pointer (also not requiring > rcu_read_lock). OK, I'll fix this up to just fetch the pointer as you suggest (and adapt the corresponding hunks in the other patches).
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..ec4570d6c38f7 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 @@ -3915,7 +3899,10 @@ int security_read_policy(struct selinux_state *state, if (!selinux_initialized(state)) return -EINVAL; - *len = security_policydb_len(state); + rcu_read_lock(); + policy = rcu_dereference(state->policy); + *len = policy->policydb.len; + rcu_read_unlock(); *data = vmalloc_user(*len); if (!*data)
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. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/include/security.h | 1 - security/selinux/selinuxfs.c | 12 ++++++------ security/selinux/ss/services.c | 21 ++++----------------- 3 files changed, 10 insertions(+), 24 deletions(-)