[PATCH][RFC] remove unused backpointers from security objects

[PATCH][RFC] remove unused backpointers from security objects

[James Morris]

Please review.

---

Author: James Morris <jmorris@namei.org>
Date:   Mon Feb 25 15:22:42 2008 +1100

    SELinux: remove unused backpointers from security objects
    
    Remove unused backpoiters from security objects.
    
    Signed-off-by: James Morris <jmorris@namei.org>

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 75c2e99..7c95548 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -161,7 +161,6 @@ static int task_alloc_security(struct task_struct *task)
 	if (!tsec)
 		return -ENOMEM;
 
-	tsec->task = task;
 	tsec->osid = tsec->sid = tsec->ptrace_sid = SECINITSID_UNLABELED;
 	task->security = tsec;
 
@@ -218,7 +217,6 @@ static int file_alloc_security(struct file *file)
 	if (!fsec)
 		return -ENOMEM;
 
-	fsec->file = file;
 	fsec->sid = tsec->sid;
 	fsec->fown_sid = tsec->sid;
 	file->f_security = fsec;
@@ -275,7 +273,6 @@ static int sk_alloc_security(struct sock *sk, int family, gfp_t priority)
 	if (!ssec)
 		return -ENOMEM;
 
-	ssec->sk = sk;
 	ssec->peer_sid = SECINITSID_UNLABELED;
 	ssec->sid = SECINITSID_UNLABELED;
 	sk->sk_security = ssec;
@@ -1864,7 +1861,6 @@ static int selinux_bprm_alloc_security(struct linux_binprm *bprm)
 	if (!bsec)
 		return -ENOMEM;
 
-	bsec->bprm = bprm;
 	bsec->sid = SECINITSID_UNLABELED;
 	bsec->set = 0;
 
@@ -4542,7 +4538,6 @@ static int ipc_alloc_security(struct task_struct *task,
 		return -ENOMEM;
 
 	isec->sclass = sclass;
-	isec->ipc_perm = perm;
 	isec->sid = tsec->sid;
 	perm->security = isec;
 
@@ -4564,7 +4559,6 @@ static int msg_msg_alloc_security(struct msg_msg *msg)
 	if (!msec)
 		return -ENOMEM;
 
-	msec->msg = msg;
 	msec->sid = SECINITSID_UNLABELED;
 	msg->security = msec;
 
@@ -5175,7 +5169,6 @@ static int selinux_key_alloc(struct key *k, struct task_struct *tsk,
 	if (!ksec)
 		return -ENOMEM;
 
-	ksec->obj = k;
 	if (tsec->keycreate_sid)
 		ksec->sid = tsec->keycreate_sid;
 	else
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index c6c2bb4..474ac23 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -28,7 +28,6 @@
 #include "avc.h"
 
 struct task_security_struct {
-	struct task_struct *task;      /* back pointer to task object */
 	u32 osid;            /* SID prior to last execve */
 	u32 sid;             /* current SID */
 	u32 exec_sid;        /* exec SID */
@@ -50,7 +49,6 @@ struct inode_security_struct {
 };
 
 struct file_security_struct {
-	struct file *file;              /* back pointer to file object */
 	u32 sid;              /* SID of open file description */
 	u32 fown_sid;         /* SID of file owner (for SIGIO) */
 	u32 isid;             /* SID of inode at the time of file open */
@@ -73,18 +71,15 @@ struct superblock_security_struct {
 };
 
 struct msg_security_struct {
-	struct msg_msg *msg;		/* back pointer */
 	u32 sid;              /* SID of message */
 };
 
 struct ipc_security_struct {
-	struct kern_ipc_perm *ipc_perm; /* back pointer */
 	u16 sclass;	/* security class of this object */
 	u32 sid;              /* SID of IPC resource */
 };
 
 struct bprm_security_struct {
-	struct linux_binprm *bprm;     /* back pointer to bprm object */
 	u32 sid;                       /* SID for transformed process */
 	unsigned char set;
 
@@ -110,7 +105,6 @@ struct netnode_security_struct {
 };
 
 struct sk_security_struct {
-	struct sock *sk;		/* back pointer to sk object */
 	u32 sid;			/* SID of this object */
 	u32 peer_sid;			/* SID of peer */
 	u16 sclass;			/* sock security class */
@@ -125,7 +119,6 @@ struct sk_security_struct {
 };
 
 struct key_security_struct {
-	struct key *obj; /* back pointer */
 	u32 sid;         /* SID of key */
 };
 

-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH][RFC] remove unused backpointers from security objects

[Stephen Smalley]

On Mon, 2008-02-25 at 15:26 +1100, James Morris wrote:
> Please review.
> 
> ---
> 
> Author: James Morris <jmorris@namei.org>
> Date:   Mon Feb 25 15:22:42 2008 +1100
> 
>     SELinux: remove unused backpointers from security objects
>     
>     Remove unused backpoiters from security objects.
>     
>     Signed-off-by: James Morris <jmorris@namei.org>
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 75c2e99..7c95548 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -161,7 +161,6 @@ static int task_alloc_security(struct task_struct *task)
>  	if (!tsec)
>  		return -ENOMEM;
>  
> -	tsec->task = task;
>  	tsec->osid = tsec->sid = tsec->ptrace_sid = SECINITSID_UNLABELED;
>  	task->security = tsec;
>  
> @@ -218,7 +217,6 @@ static int file_alloc_security(struct file *file)
>  	if (!fsec)
>  		return -ENOMEM;
>  
> -	fsec->file = file;
>  	fsec->sid = tsec->sid;
>  	fsec->fown_sid = tsec->sid;
>  	file->f_security = fsec;
> @@ -275,7 +273,6 @@ static int sk_alloc_security(struct sock *sk, int family, gfp_t priority)
>  	if (!ssec)
>  		return -ENOMEM;
>  
> -	ssec->sk = sk;

Unless it has changed recently, this back pointer is still in use by
netlabel.c.  Likely could be reworked though to have the caller
(selinux_sk_clone_security) just pass in the sk pointer or even just
sk->sk_family directly.


>  	ssec->peer_sid = SECINITSID_UNLABELED;
>  	ssec->sid = SECINITSID_UNLABELED;
>  	sk->sk_security = ssec;
> @@ -1864,7 +1861,6 @@ static int selinux_bprm_alloc_security(struct linux_binprm *bprm)
>  	if (!bsec)
>  		return -ENOMEM;
>  
> -	bsec->bprm = bprm;
>  	bsec->sid = SECINITSID_UNLABELED;
>  	bsec->set = 0;
>  
> @@ -4542,7 +4538,6 @@ static int ipc_alloc_security(struct task_struct *task,
>  		return -ENOMEM;
>  
>  	isec->sclass = sclass;
> -	isec->ipc_perm = perm;
>  	isec->sid = tsec->sid;
>  	perm->security = isec;
>  
> @@ -4564,7 +4559,6 @@ static int msg_msg_alloc_security(struct msg_msg *msg)
>  	if (!msec)
>  		return -ENOMEM;
>  
> -	msec->msg = msg;
>  	msec->sid = SECINITSID_UNLABELED;
>  	msg->security = msec;
>  
> @@ -5175,7 +5169,6 @@ static int selinux_key_alloc(struct key *k, struct task_struct *tsk,
>  	if (!ksec)
>  		return -ENOMEM;
>  
> -	ksec->obj = k;
>  	if (tsec->keycreate_sid)
>  		ksec->sid = tsec->keycreate_sid;
>  	else
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index c6c2bb4..474ac23 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -28,7 +28,6 @@
>  #include "avc.h"
>  
>  struct task_security_struct {
> -	struct task_struct *task;      /* back pointer to task object */
>  	u32 osid;            /* SID prior to last execve */
>  	u32 sid;             /* current SID */
>  	u32 exec_sid;        /* exec SID */
> @@ -50,7 +49,6 @@ struct inode_security_struct {
>  };
>  
>  struct file_security_struct {
> -	struct file *file;              /* back pointer to file object */
>  	u32 sid;              /* SID of open file description */
>  	u32 fown_sid;         /* SID of file owner (for SIGIO) */
>  	u32 isid;             /* SID of inode at the time of file open */
> @@ -73,18 +71,15 @@ struct superblock_security_struct {
>  };
>  
>  struct msg_security_struct {
> -	struct msg_msg *msg;		/* back pointer */
>  	u32 sid;              /* SID of message */
>  };
>  
>  struct ipc_security_struct {
> -	struct kern_ipc_perm *ipc_perm; /* back pointer */
>  	u16 sclass;	/* security class of this object */
>  	u32 sid;              /* SID of IPC resource */
>  };
>  
>  struct bprm_security_struct {
> -	struct linux_binprm *bprm;     /* back pointer to bprm object */
>  	u32 sid;                       /* SID for transformed process */
>  	unsigned char set;
>  
> @@ -110,7 +105,6 @@ struct netnode_security_struct {
>  };
>  
>  struct sk_security_struct {
> -	struct sock *sk;		/* back pointer to sk object */
>  	u32 sid;			/* SID of this object */
>  	u32 peer_sid;			/* SID of peer */
>  	u16 sclass;			/* sock security class */
> @@ -125,7 +119,6 @@ struct sk_security_struct {
>  };
>  
>  struct key_security_struct {
> -	struct key *obj; /* back pointer */
>  	u32 sid;         /* SID of key */
>  };
>  
> 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH][RFC] remove unused backpointers from security objects

[James Morris]

On Mon, 25 Feb 2008, Stephen Smalley wrote:

> > -	ssec->sk = sk;
> 
> Unless it has changed recently, this back pointer is still in use by
> netlabel.c.  Likely could be reworked though to have the caller
> (selinux_sk_clone_security) just pass in the sk pointer or even just
> sk->sk_family directly.

Whoops, didn't have CONFIG_NETLABEL.

See updated version below, with the sk pointer (to allow the callee to 
take the rcu read lock).

---

Author: James Morris <jmorris@namei.org>
Date:   Tue Feb 26 00:27:36 2008 +1100

    SELinux: remove unused backpointers from security objects
    
    Remove unused backpoiters from security objects.
    
    Signed-off-by: James Morris <jmorris@namei.org>

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 75c2e99..5918d03 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -161,7 +161,6 @@ static int task_alloc_security(struct task_struct *task)
 	if (!tsec)
 		return -ENOMEM;
 
-	tsec->task = task;
 	tsec->osid = tsec->sid = tsec->ptrace_sid = SECINITSID_UNLABELED;
 	task->security = tsec;
 
@@ -218,7 +217,6 @@ static int file_alloc_security(struct file *file)
 	if (!fsec)
 		return -ENOMEM;
 
-	fsec->file = file;
 	fsec->sid = tsec->sid;
 	fsec->fown_sid = tsec->sid;
 	file->f_security = fsec;
@@ -275,7 +273,6 @@ static int sk_alloc_security(struct sock *sk, int family, gfp_t priority)
 	if (!ssec)
 		return -ENOMEM;
 
-	ssec->sk = sk;
 	ssec->peer_sid = SECINITSID_UNLABELED;
 	ssec->sid = SECINITSID_UNLABELED;
 	sk->sk_security = ssec;
@@ -1864,7 +1861,6 @@ static int selinux_bprm_alloc_security(struct linux_binprm *bprm)
 	if (!bsec)
 		return -ENOMEM;
 
-	bsec->bprm = bprm;
 	bsec->sid = SECINITSID_UNLABELED;
 	bsec->set = 0;
 
@@ -4120,7 +4116,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_clone(newssec, sk);
 }
 
 static void selinux_sk_getsecid(struct sock *sk, u32 *secid)
@@ -4542,7 +4538,6 @@ static int ipc_alloc_security(struct task_struct *task,
 		return -ENOMEM;
 
 	isec->sclass = sclass;
-	isec->ipc_perm = perm;
 	isec->sid = tsec->sid;
 	perm->security = isec;
 
@@ -4564,7 +4559,6 @@ static int msg_msg_alloc_security(struct msg_msg *msg)
 	if (!msec)
 		return -ENOMEM;
 
-	msec->msg = msg;
 	msec->sid = SECINITSID_UNLABELED;
 	msg->security = msec;
 
@@ -5175,7 +5169,6 @@ static int selinux_key_alloc(struct key *k, struct task_struct *tsk,
 	if (!ksec)
 		return -ENOMEM;
 
-	ksec->obj = k;
 	if (tsec->keycreate_sid)
 		ksec->sid = tsec->keycreate_sid;
 	else
diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h
index 00a2809..02bb425 100644
--- a/security/selinux/include/netlabel.h
+++ b/security/selinux/include/netlabel.h
@@ -44,7 +44,7 @@ void selinux_netlbl_sk_security_reset(struct sk_security_struct *ssec,
 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);
+				      const struct sock *sk);
 
 int selinux_netlbl_skbuff_getsid(struct sk_buff *skb,
 				 u16 family,
@@ -81,7 +81,7 @@ static inline void selinux_netlbl_sk_security_init(
 }
 static inline void selinux_netlbl_sk_security_clone(
 	                                    struct sk_security_struct *ssec,
-					    struct sk_security_struct *newssec)
+					    const struct sock *sk)
 {
 	return;
 }
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index c6c2bb4..474ac23 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -28,7 +28,6 @@
 #include "avc.h"
 
 struct task_security_struct {
-	struct task_struct *task;      /* back pointer to task object */
 	u32 osid;            /* SID prior to last execve */
 	u32 sid;             /* current SID */
 	u32 exec_sid;        /* exec SID */
@@ -50,7 +49,6 @@ struct inode_security_struct {
 };
 
 struct file_security_struct {
-	struct file *file;              /* back pointer to file object */
 	u32 sid;              /* SID of open file description */
 	u32 fown_sid;         /* SID of file owner (for SIGIO) */
 	u32 isid;             /* SID of inode at the time of file open */
@@ -73,18 +71,15 @@ struct superblock_security_struct {
 };
 
 struct msg_security_struct {
-	struct msg_msg *msg;		/* back pointer */
 	u32 sid;              /* SID of message */
 };
 
 struct ipc_security_struct {
-	struct kern_ipc_perm *ipc_perm; /* back pointer */
 	u16 sclass;	/* security class of this object */
 	u32 sid;              /* SID of IPC resource */
 };
 
 struct bprm_security_struct {
-	struct linux_binprm *bprm;     /* back pointer to bprm object */
 	u32 sid;                       /* SID for transformed process */
 	unsigned char set;
 
@@ -110,7 +105,6 @@ struct netnode_security_struct {
 };
 
 struct sk_security_struct {
-	struct sock *sk;		/* back pointer to sk object */
 	u32 sid;			/* SID of this object */
 	u32 peer_sid;			/* SID of peer */
 	u16 sclass;			/* sock security class */
@@ -125,7 +119,6 @@ struct sk_security_struct {
 };
 
 struct key_security_struct {
-	struct key *obj; /* back pointer */
 	u32 sid;         /* SID of key */
 };
 
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index 0fa2be4..7b298ef 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -148,22 +148,21 @@ void selinux_netlbl_sk_security_init(struct sk_security_struct *ssec,
 
 /**
  * selinux_netlbl_sk_security_clone - Copy the NetLabel fields
- * @ssec: the original sk_security_struct
  * @newssec: the cloned sk_security_struct
+ * @sk: sk to be cloned from
  *
  * Description:
- * Clone the NetLabel specific sk_security_struct fields from @ssec to
+ * Clone the NetLabel specific sk_security_struct fields from @sk->ssec to
  * @newssec.
  *
  */
-void selinux_netlbl_sk_security_clone(struct sk_security_struct *ssec,
-				      struct sk_security_struct *newssec)
+void selinux_netlbl_sk_security_clone(struct sk_security_struct *newssec, const struct sock *sk)
 {
 	/* 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);
+	selinux_netlbl_sk_security_reset(newssec, sk->sk_family);
 	rcu_read_unlock();
 }
 

-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH][RFC] remove unused backpointers from security objects

[James Morris]

On Tue, 26 Feb 2008, James Morris wrote:

> See updated version below, with the sk pointer (to allow the callee to 
> take the rcu read lock).

Actually, we shouldn't need that lock there, and can directly call 
selinux_netlbl_sk_security_reset, right?


-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH][RFC] remove unused backpointers from security objects

[Paul Moore]

On Monday 25 February 2008 8:47:15 am James Morris wrote:
> On Tue, 26 Feb 2008, James Morris wrote:
> > See updated version below, with the sk pointer (to allow the callee
> > to take the rcu read lock).
>
> Actually, we shouldn't need that lock there, and can directly call
> selinux_netlbl_sk_security_reset, right?

I'm looking at this right now ... based on the idea that you can safely 
read/write 32-bit values without a lock I believe the locking for 
sk_security_struct could use a bit of a tweak.  I'm still 
looking/thinking but I believe we can remove the rcu lock entirely and 
just rework selinux_netlbl_inode_permission() a little bit.

If you can spare a few hours I'll post a RFC patch ...

-- 
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@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

[PATCH] SELinux: Correct the NetLabel locking for the sk_security_struct

[Paul Moore]

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@hp.com>
---

 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;
 }


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: Correct the NetLabel locking for the sk_security_struct

[Paul Moore]

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@hp.com>

Hmm, I was playing around with annotations and it looks like it didn't 
come through ... this patch is compile tested only.

> ---
>
>  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@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: Correct the NetLabel locking for the sk_security_struct

[Paul Moore]

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@hp.com>
>
> 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@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: Correct the NetLabel locking for the sk_security_struct

[James Morris]

On Mon, 25 Feb 2008, 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.

Ok, the other thing to be concerned with is instruction reordering, and 
the possible need for memory barriers, which should be covered in this 
case by the memory barriers provided by the existing socket locks.

I'll apply this to for-akpm and then a revised version of the backpointer 
removal patch.


- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.