On Monday 25 February 2008 11:42:09 am Paul Moore wrote:
> On Monday 25 February 2008 11:40:33 am Paul Moore wrote:
> > The RCU/spinlock locking approach for the nlbl_state in the
> > sk_security_struct was almost certainly overkill. This patch
> > removes both the RCU and spinlock locking, relying on the existing
> > socket locks to handle the case of multiple writers. This change
> > also makes several code reductions possible.
> >
> > Less locking, less code - it's a Good Thing.
> >
> > Signed-off-by: Paul Moore <paul.moore@xxxxxx>
>
> Hmm, I was playing around with annotations and it looks like it
> didn't come through ... this patch is compile tested only.
Now it's boot tested too. Simple tests show no regressions.
> > ---
> >
> > security/selinux/hooks.c | 4 +-
> > security/selinux/include/netlabel.h | 16 -------
> > security/selinux/include/objsec.h | 1
> > security/selinux/netlabel.c | 81
> > ++++++----------------------------- 4 files changed, 15
> > insertions(+), 87 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 75c2e99..2caf519 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -280,7 +280,7 @@ static int sk_alloc_security(struct sock *sk,
> > int family, gfp_t priority) ssec->sid = SECINITSID_UNLABELED;
> > sk->sk_security = ssec;
> >
> > - selinux_netlbl_sk_security_init(ssec, family);
> > + selinux_netlbl_sk_security_reset(ssec, family);
> >
> > return 0;
> > }
> > @@ -4120,7 +4120,7 @@ static void selinux_sk_clone_security(const
> > struct sock *sk, struct sock *newsk) newssec->peer_sid =
> > ssec->peer_sid;
> > newssec->sclass = ssec->sclass;
> >
> > - selinux_netlbl_sk_security_clone(ssec, newssec);
> > + selinux_netlbl_sk_security_reset(newssec, newsk->sk_family);
> > }
> >
> > static void selinux_sk_getsecid(struct sock *sk, u32 *secid)
> > diff --git a/security/selinux/include/netlabel.h
> > b/security/selinux/include/netlabel.h index 00a2809..9a9e7cd 100644
> > --- a/security/selinux/include/netlabel.h
> > +++ b/security/selinux/include/netlabel.h
> > @@ -41,10 +41,6 @@ void selinux_netlbl_cache_invalidate(void);
> >
> > void selinux_netlbl_sk_security_reset(struct sk_security_struct
> > *ssec, int family);
> > -void selinux_netlbl_sk_security_init(struct sk_security_struct
> > *ssec, - int family);
> > -void selinux_netlbl_sk_security_clone(struct sk_security_struct
> > *ssec, - struct sk_security_struct *newssec);
> >
> > int selinux_netlbl_skbuff_getsid(struct sk_buff *skb,
> > u16 family,
> > @@ -73,18 +69,6 @@ static inline void
> > selinux_netlbl_sk_security_reset( {
> > return;
> > }
> > -static inline void selinux_netlbl_sk_security_init(
> > - struct sk_security_struct
> > *ssec, - int family)
> > -{
> > - return;
> > -}
> > -static inline void selinux_netlbl_sk_security_clone(
> > - struct sk_security_struct
> > *ssec, - struct sk_security_struct *newssec)
> > -{
> > - return;
> > -}
> >
> > static inline int selinux_netlbl_skbuff_getsid(struct sk_buff
> > *skb, u16 family,
> > diff --git a/security/selinux/include/objsec.h
> > b/security/selinux/include/objsec.h index c6c2bb4..0b74077 100644
> > --- a/security/selinux/include/objsec.h
> > +++ b/security/selinux/include/objsec.h
> > @@ -120,7 +120,6 @@ struct sk_security_struct {
> > NLBL_REQUIRE,
> > NLBL_LABELED,
> > } nlbl_state;
> > - spinlock_t nlbl_lock; /* protects nlbl_state */
> > #endif
> > };
> >
> > diff --git a/security/selinux/netlabel.c
> > b/security/selinux/netlabel.c index 0fa2be4..ccf71f6 100644
> > --- a/security/selinux/netlabel.c
> > +++ b/security/selinux/netlabel.c
> > @@ -69,9 +69,7 @@ static int selinux_netlbl_sidlookup_cached(struct
> > sk_buff *skb, *
> > * Description:
> > * Attempt to label a socket using the NetLabel mechanism using
> > the given - * SID. Returns zero values on success, negative values
> > on failure. The - * caller is responsibile for calling
> > rcu_read_lock() before calling this - * this function and
> > rcu_read_unlock() after this function returns. + * SID. Returns
> > zero values on success, negative values on failure. *
> > */
> > static int selinux_netlbl_sock_setsid(struct sock *sk, u32 sid)
> > @@ -86,11 +84,8 @@ static int selinux_netlbl_sock_setsid(struct
> > sock *sk, u32 sid) if (rc != 0)
> > goto sock_setsid_return;
> > rc = netlbl_sock_setattr(sk, &secattr);
> > - if (rc == 0) {
> > - spin_lock_bh(&sksec->nlbl_lock);
> > + if (rc == 0)
> > sksec->nlbl_state = NLBL_LABELED;
> > - spin_unlock_bh(&sksec->nlbl_lock);
> > - }
> >
> > sock_setsid_return:
> > netlbl_secattr_destroy(&secattr);
> > @@ -129,45 +124,6 @@ void selinux_netlbl_sk_security_reset(struct
> > sk_security_struct *ssec, }
> >
> > /**
> > - * selinux_netlbl_sk_security_init - Setup the NetLabel fields
> > - * @ssec: the sk_security_struct
> > - * @family: the socket family
> > - *
> > - * Description:
> > - * Called when a new sk_security_struct is allocated to initialize
> > the NetLabel - * fields.
> > - *
> > - */
> > -void selinux_netlbl_sk_security_init(struct sk_security_struct
> > *ssec, - int family)
> > -{
> > - /* No locking needed, we are the only one who has access to ssec
> > */ - selinux_netlbl_sk_security_reset(ssec, family);
> > - spin_lock_init(&ssec->nlbl_lock);
> > -}
> > -
> > -/**
> > - * selinux_netlbl_sk_security_clone - Copy the NetLabel fields
> > - * @ssec: the original sk_security_struct
> > - * @newssec: the cloned sk_security_struct
> > - *
> > - * Description:
> > - * Clone the NetLabel specific sk_security_struct fields from
> > @ssec to - * @newssec.
> > - *
> > - */
> > -void selinux_netlbl_sk_security_clone(struct sk_security_struct
> > *ssec, - struct sk_security_struct *newssec)
> > -{
> > - /* We don't need to take newssec->nlbl_lock because we are the
> > only - * thread with access to newssec, but we do need to take the
> > RCU read - * lock as other threads could have access to ssec */
> > - rcu_read_lock();
> > - selinux_netlbl_sk_security_reset(newssec, ssec->sk->sk_family);
> > - rcu_read_unlock();
> > -}
> > -
> > -/**
> > * selinux_netlbl_skbuff_getsid - Get the sid of a packet using
> > NetLabel * @skb: the packet
> > * @family: protocol family
> > @@ -221,12 +177,8 @@ void selinux_netlbl_sock_graft(struct sock
> > *sk, struct socket *sock) struct netlbl_lsm_secattr secattr;
> > u32 nlbl_peer_sid;
> >
> > - rcu_read_lock();
> > -
> > - if (sksec->nlbl_state != NLBL_REQUIRE) {
> > - rcu_read_unlock();
> > + if (sksec->nlbl_state != NLBL_REQUIRE)
> > return;
> > - }
> >
> > netlbl_secattr_init(&secattr);
> > if (netlbl_sock_getattr(sk, &secattr) == 0 &&
> > @@ -239,8 +191,6 @@ void selinux_netlbl_sock_graft(struct sock *sk,
> > struct socket *sock) * here we will pick up the pieces in later
> > calls to
> > * selinux_netlbl_inode_permission(). */
> > selinux_netlbl_sock_setsid(sk, sksec->sid);
> > -
> > - rcu_read_unlock();
> > }
> >
> > /**
> > @@ -254,16 +204,13 @@ void selinux_netlbl_sock_graft(struct sock
> > *sk, struct socket *sock) */
> > int selinux_netlbl_socket_post_create(struct socket *sock)
> > {
> > - int rc = 0;
> > struct sock *sk = sock->sk;
> > struct sk_security_struct *sksec = sk->sk_security;
> >
> > - rcu_read_lock();
> > - if (sksec->nlbl_state == NLBL_REQUIRE)
> > - rc = selinux_netlbl_sock_setsid(sk, sksec->sid);
> > - rcu_read_unlock();
> > + if (sksec->nlbl_state != NLBL_REQUIRE)
> > + return 0;
> >
> > - return rc;
> > + return selinux_netlbl_sock_setsid(sk, sksec->sid);
> > }
> >
> > /**
> > @@ -288,21 +235,21 @@ int selinux_netlbl_inode_permission(struct
> > inode *inode, int mask) if (!S_ISSOCK(inode->i_mode) ||
> > ((mask & (MAY_WRITE | MAY_APPEND)) == 0))
> > return 0;
> > +
> > sock = SOCKET_I(inode);
> > sk = sock->sk;
> > sksec = sk->sk_security;
> > -
> > - rcu_read_lock();
> > - if (sksec->nlbl_state != NLBL_REQUIRE) {
> > - rcu_read_unlock();
> > + if (sksec->nlbl_state != NLBL_REQUIRE)
> > return 0;
> > - }
> > +
> > local_bh_disable();
> > bh_lock_sock_nested(sk);
> > - rc = selinux_netlbl_sock_setsid(sk, sksec->sid);
> > + if (likely(sksec->nlbl_state == NLBL_REQUIRE))
> > + rc = selinux_netlbl_sock_setsid(sk, sksec->sid);
> > + else
> > + rc = 0;
> > bh_unlock_sock(sk);
> > local_bh_enable();
> > - rcu_read_unlock();
> >
> > return rc;
> > }
> > @@ -385,7 +332,6 @@ int selinux_netlbl_socket_setsockopt(struct
> > socket *sock, struct sk_security_struct *sksec = sk->sk_security;
> > struct netlbl_lsm_secattr secattr;
> >
> > - rcu_read_lock();
> > if (level == IPPROTO_IP && optname == IP_OPTIONS &&
> > sksec->nlbl_state == NLBL_LABELED) {
> > netlbl_secattr_init(&secattr);
> > @@ -396,7 +342,6 @@ int selinux_netlbl_socket_setsockopt(struct
> > socket *sock, rc = -EACCES;
> > netlbl_secattr_destroy(&secattr);
> > }
> > - rcu_read_unlock();
> >
> > return rc;
> > }
--
paul moore
linux security @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
