[PATCH] scm: fold __scm_send() into scm_send()

[PATCH] scm: fold __scm_send() into scm_send()

[Ingo Oeser]

From: Ingo Oeser <ioe-lkml@rameria.de>

Fold __scm_send() into scm_send() and remove that interface completly
from the kernel.

Signed-off-by: Ingo Oeser <ioe-klml@rameria.de>
---
Inspired by the patch to inline scm_send()
I did the next logical step :-)

Regards

Ingo Oeser

diff --git a/include/net/scm.h b/include/net/scm.h
index eb44e5a..ec8b891 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -26,11 +26,9 @@ struct scm_cookie
 
 extern void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm);
 extern void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm);
-extern int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm);
 extern void __scm_destroy(struct scm_cookie *scm);
 extern struct scm_fp_list * scm_fp_dup(struct scm_fp_list *fpl);
-extern int scm_send(struct socket *sock, struct msghdr *msg,
-			struct scm_cookie *scm);
+extern int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm);
 extern void scm_recv(struct socket *sock, struct msghdr *msg,
 		struct scm_cookie *scm, int flags);
 
diff --git a/net/core/scm.c b/net/core/scm.c
index b6dee90..6adbe60 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -110,10 +110,21 @@ void __scm_destroy(struct scm_cookie *sc
 	}
 }
 
-int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p)
+int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
 {
 	struct cmsghdr *cmsg;
 	int err;
+	struct task_struct *tsk = current;
+	scm->creds = (struct ucred) {
+		.uid = tsk->uid,
+		.gid = tsk->gid,
+		.pid = tsk->tgid
+	};
+	scm->fp = NULL;
+	scm->sid = security_sk_sid(sock->sk, NULL, 0);
+	scm->seq = 0;
+	if (msg->msg_controllen <= 0)
+		return 0;
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg))
 	{
@@ -136,15 +147,15 @@ int __scm_send(struct socket *sock, stru
 		switch (cmsg->cmsg_type)
 		{
 		case SCM_RIGHTS:
-			err=scm_fp_copy(cmsg, &p->fp);
+			err=scm_fp_copy(cmsg, &scm->fp);
 			if (err<0)
 				goto error;
 			break;
 		case SCM_CREDENTIALS:
 			if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct ucred)))
 				goto error;
-			memcpy(&p->creds, CMSG_DATA(cmsg), sizeof(struct ucred));
-			err = scm_check_creds(&p->creds);
+			memcpy(&scm->creds, CMSG_DATA(cmsg), sizeof(struct ucred));
+			err = scm_check_creds(&scm->creds);
 			if (err)
 				goto error;
 			break;
@@ -153,15 +164,15 @@ int __scm_send(struct socket *sock, stru
 		}
 	}
 
-	if (p->fp && !p->fp->count)
+	if (scm->fp && !scm->fp->count)
 	{
-		kfree(p->fp);
-		p->fp = NULL;
+		kfree(scm->fp);
+		scm->fp = NULL;
 	}
 	return 0;
 	
 error:
-	scm_destroy(p);
+	scm_destroy(scm);
 	return err;
 }
 
@@ -284,22 +295,6 @@ struct scm_fp_list *scm_fp_dup(struct sc
 	return new_fpl;
 }
 
-int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
-{
-	struct task_struct *p = current;
-	scm->creds = (struct ucred) {
-		.uid = p->uid,
-		.gid = p->gid,
-		.pid = p->tgid
-	};
-	scm->fp = NULL;
-	scm->sid = security_sk_sid(sock->sk, NULL, 0);
-	scm->seq = 0;
-	if (msg->msg_controllen <= 0)
-		return 0;
-	return __scm_send(sock, msg, scm);
-}
-
 void scm_recv(struct socket *sock, struct msghdr *msg,
 		struct scm_cookie *scm, int flags)
 {
@@ -332,7 +326,6 @@ void scm_recv(struct socket *sock, struc
 }
 
 EXPORT_SYMBOL(__scm_destroy);
-EXPORT_SYMBOL(__scm_send);
 EXPORT_SYMBOL(scm_send);
 EXPORT_SYMBOL(scm_recv);
 EXPORT_SYMBOL(put_cmsg);

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Benjamin LaHaise]

On Mon, Mar 13, 2006 at 09:05:31PM +0100, Ingo Oeser wrote:
> From: Ingo Oeser <ioe-lkml@rameria.de>
> 
> Fold __scm_send() into scm_send() and remove that interface completly
> from the kernel.

Whoa, what are you doing here?  Uninlining scm_send() is a Bad Thing to do 
given that scm_send() is in the AF_UNIX hot path.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <dont@kvack.org>.

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Andrew Morton]

Benjamin LaHaise <bcrl@kvack.org> wrote:
>
> On Mon, Mar 13, 2006 at 09:05:31PM +0100, Ingo Oeser wrote:
> > From: Ingo Oeser <ioe-lkml@rameria.de>
> > 
> > Fold __scm_send() into scm_send() and remove that interface completly
> > from the kernel.
> 
> Whoa, what are you doing here?
>

scm_send() and scm_recv() are already uninlined in Dave's tree - this patch
does further consolidation.

>  Uninlining scm_send() is a Bad Thing to do 
> given that scm_send() is in the AF_UNIX hot path.

scm_send() and scm_recv() are in _two_ AF_UNIX hotpaths...

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Andrew Morton]

Ingo Oeser <ioe-lkml@rameria.de> wrote:
>
>  -int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
>  -{
>  -	struct task_struct *p = current;
>  -	scm->creds = (struct ucred) {
>  -		.uid = p->uid,
>  -		.gid = p->gid,
>  -		.pid = p->tgid
>  -	};
>  -	scm->fp = NULL;
>  -	scm->sid = security_sk_sid(sock->sk, NULL, 0);
>  -	scm->seq = 0;
>  -	if (msg->msg_controllen <= 0)
>  -		return 0;
>  -	return __scm_send(sock, msg, scm);
>  -}

It's worth noting that scm_send() will call security_sk_sid() even if
(msg->msg_controllen <= 0).

If that test is likely to be true with any frequency then perhaps we can
optimise things...

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Ingo Oeser]

Hi Chris,

Andrew Morton wrote:
> Ingo Oeser <ioe-lkml@rameria.de> wrote:
> >
> >  -int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> >  -{
> >  -	struct task_struct *p = current;
> >  -	scm->creds = (struct ucred) {
> >  -		.uid = p->uid,
> >  -		.gid = p->gid,
> >  -		.pid = p->tgid
> >  -	};
> >  -	scm->fp = NULL;
> >  -	scm->sid = security_sk_sid(sock->sk, NULL, 0);
> >  -	scm->seq = 0;
> >  -	if (msg->msg_controllen <= 0)
> >  -		return 0;
> >  -	return __scm_send(sock, msg, scm);
> >  -}
> 
> It's worth noting that scm_send() will call security_sk_sid() even if
> (msg->msg_controllen <= 0).

Chris, do you know if this is needed in this case?

> If that test is likely to be true with any frequency then perhaps we can
> optimise things...

That test seems to be the original intention for the splitup. 

The security modules just put their hooks here. Maybe we can
fold these hooks into __scm_send() and have the old
splitup again to get the old code paths back.

It seems that the credential copy in af_unix.c 

memcpy(UNIXCREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
if (siocb->scm->fp)
            unix_attach_fds(siocb->scm, skb);

doesn't depend on the "msg_controllen <= 0" test. If we can introduce this 
dependency there, we can put credential setup into __scm_send().

I would suggest we fold these two lines into a function and decide this later.

Chris, would this suffice?

Regards

Ingo Oeser

BTW: ioe-lkml@rameria.de is simply netdev@axxeo.de at work :-)

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Chris Wright]

* Ingo Oeser (netdev@axxeo.de) wrote:
> Hi Chris,
> 
> Andrew Morton wrote:
> > Ingo Oeser <ioe-lkml@rameria.de> wrote:
> > >
> > >  -int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> > >  -{
> > >  -	struct task_struct *p = current;
> > >  -	scm->creds = (struct ucred) {
> > >  -		.uid = p->uid,
> > >  -		.gid = p->gid,
> > >  -		.pid = p->tgid
> > >  -	};
> > >  -	scm->fp = NULL;
> > >  -	scm->sid = security_sk_sid(sock->sk, NULL, 0);
> > >  -	scm->seq = 0;
> > >  -	if (msg->msg_controllen <= 0)
> > >  -		return 0;
> > >  -	return __scm_send(sock, msg, scm);
> > >  -}
> > 
> > It's worth noting that scm_send() will call security_sk_sid() even if
> > (msg->msg_controllen <= 0).
> 
> Chris, do you know if this is needed in this case?

This whole thing is looking broken.  I'm still trying to find the original
patch which caused the series of broken patches on top.

thanks,
-chris

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Chris Wright]

* Chris Wright (chrisw@sous-sol.org) wrote:
> * Ingo Oeser (netdev@axxeo.de) wrote:
> > Hi Chris,
> > 
> > Andrew Morton wrote:
> > > Ingo Oeser <ioe-lkml@rameria.de> wrote:
> > > >
> > > >  -int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> > > >  -{
> > > >  -	struct task_struct *p = current;
> > > >  -	scm->creds = (struct ucred) {
> > > >  -		.uid = p->uid,
> > > >  -		.gid = p->gid,
> > > >  -		.pid = p->tgid
> > > >  -	};
> > > >  -	scm->fp = NULL;
> > > >  -	scm->sid = security_sk_sid(sock->sk, NULL, 0);
> > > >  -	scm->seq = 0;
> > > >  -	if (msg->msg_controllen <= 0)
> > > >  -		return 0;
> > > >  -	return __scm_send(sock, msg, scm);
> > > >  -}
> > > 
> > > It's worth noting that scm_send() will call security_sk_sid() even if
> > > (msg->msg_controllen <= 0).
> > 
> > Chris, do you know if this is needed in this case?
> 
> This whole thing is looking broken.  I'm still trying to find the original
> patch which caused the series of broken patches on top.

OK, it starts here from Catherine's patch:

include/net/scm.h::scm_recv()
+	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
+		err = security_sid_to_context(scm->sid, &scontext, &scontext_len);
+		if (!err)
+ 			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, scontext_len, scontext);
+ }

Catherine, the security_sid_to_context() is a raw SELinux function which
crept into core code and should not have been there.  The fallout fixes
included conditionally exporting security_sid_to_context, and finally
scm_send/recv unlining.  The end result in -mm looks broken to me.
Specifically, it now does:

	ucred->uid = tsk->uid;
	ucred->gid = tsk->gid;
	ucred->pid = tsk->tgid;
	scm->fp = NULL;
	scm->seq = 0;
	if (msg->msg_controllen <= 0)
		return 0;

	scm->sid = security_sk_sid(sock->sk, NULL, 0);

The point of Catherine's original patch was to make sure there's always
a security identifier associated with AF_UNIX messages.  So receiver
can always check it (same as having credentials even w/out sender
control message passing them).  Now we will have garbage for sid.

thanks,
-chris

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Andrew Morton]

Chris Wright <chrisw@sous-sol.org> wrote:
>
> * Chris Wright (chrisw@sous-sol.org) wrote:
> > * Ingo Oeser (netdev@axxeo.de) wrote:
> > > Hi Chris,
> > > 
> > > Andrew Morton wrote:
> > > > Ingo Oeser <ioe-lkml@rameria.de> wrote:
> > > > >
> > > > >  -int scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> > > > >  -{
> > > > >  -	struct task_struct *p = current;
> > > > >  -	scm->creds = (struct ucred) {
> > > > >  -		.uid = p->uid,
> > > > >  -		.gid = p->gid,
> > > > >  -		.pid = p->tgid
> > > > >  -	};
> > > > >  -	scm->fp = NULL;
> > > > >  -	scm->sid = security_sk_sid(sock->sk, NULL, 0);
> > > > >  -	scm->seq = 0;
> > > > >  -	if (msg->msg_controllen <= 0)
> > > > >  -		return 0;
> > > > >  -	return __scm_send(sock, msg, scm);
> > > > >  -}
> > > > 
> > > > It's worth noting that scm_send() will call security_sk_sid() even if
> > > > (msg->msg_controllen <= 0).
> > > 
> > > Chris, do you know if this is needed in this case?
> > 
> > This whole thing is looking broken.  I'm still trying to find the original
> > patch which caused the series of broken patches on top.
> 
> OK, it starts here from Catherine's patch:
> 
> include/net/scm.h::scm_recv()
> +	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> +		err = security_sid_to_context(scm->sid, &scontext, &scontext_len);
> +		if (!err)
> + 			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, scontext_len, scontext);
> + }
> 
> Catherine, the security_sid_to_context() is a raw SELinux function which
> crept into core code and should not have been there.  The fallout fixes
> included conditionally exporting security_sid_to_context, and finally
> scm_send/recv unlining.

Yes.  So we're OK up the uninlining, right?

>  The end result in -mm looks broken to me.
> Specifically, it now does:
> 
> 	ucred->uid = tsk->uid;
> 	ucred->gid = tsk->gid;
> 	ucred->pid = tsk->tgid;
> 	scm->fp = NULL;
> 	scm->seq = 0;
> 	if (msg->msg_controllen <= 0)
> 		return 0;
> 
> 	scm->sid = security_sk_sid(sock->sk, NULL, 0);
> 
> The point of Catherine's original patch was to make sure there's always
> a security identifier associated with AF_UNIX messages.  So receiver
> can always check it (same as having credentials even w/out sender
> control message passing them).  Now we will have garbage for sid.

This answers the question I've been asking all and sundry for a week, thanks ;)

So:

- scm-fold-__scm_send-into-scm_send.patch is OK

- scm_send-speedup.patch is wrong

- Catherine's patch introduces a possibly-significant performance
  problem: we're now calling the expensive-on-SELinux security_sk_sid()
  more frequently than we used to.

- That "initialise scm->creds via a temporary struct" trick still
  generates bad code.


I actually have enough to be going on with here - I'll drop it all.

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> Chris Wright <chrisw@sous-sol.org> wrote:
> > Catherine, the security_sid_to_context() is a raw SELinux function which
> > crept into core code and should not have been there.  The fallout fixes
> > included conditionally exporting security_sid_to_context, and finally
> > scm_send/recv unlining.
> 
> Yes.  So we're OK up the uninlining, right?

Yes, although sid_to_context is meant to be analog to the other
get_peersec calls, and should really be made a proper part of the
interface (can be done later, correctness is the issue at hand).

> >  The end result in -mm looks broken to me.
> > Specifically, it now does:
> > 
> > 	ucred->uid = tsk->uid;
> > 	ucred->gid = tsk->gid;
> > 	ucred->pid = tsk->tgid;
> > 	scm->fp = NULL;
> > 	scm->seq = 0;
> > 	if (msg->msg_controllen <= 0)
> > 		return 0;
> > 
> > 	scm->sid = security_sk_sid(sock->sk, NULL, 0);
> > 
> > The point of Catherine's original patch was to make sure there's always
> > a security identifier associated with AF_UNIX messages.  So receiver
> > can always check it (same as having credentials even w/out sender
> > control message passing them).  Now we will have garbage for sid.
> 
> This answers the question I've been asking all and sundry for a week, thanks ;)
> So:
> 
> - scm-fold-__scm_send-into-scm_send.patch is OK

Yes.

> - scm_send-speedup.patch is wrong

Yes.

> - Catherine's patch introduces a possibly-significant performance
>   problem: we're now calling the expensive-on-SELinux security_sk_sid()
>   more frequently than we used to.

I don't expect security_sk_sid() to be terribly expensive.  It's not
an AVC check, it's just propagating a label.  But I've not done any
benchmarking on that.

thanks,
-chris

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Stephen Smalley]

On Mon, 2006-03-20 at 15:15 -0800, Chris Wright wrote:
> * Andrew Morton (akpm@osdl.org) wrote:
> > Chris Wright <chrisw@sous-sol.org> wrote:
> > > Catherine, the security_sid_to_context() is a raw SELinux function which
> > > crept into core code and should not have been there.  The fallout fixes
> > > included conditionally exporting security_sid_to_context, and finally
> > > scm_send/recv unlining.
> > 
> > Yes.  So we're OK up the uninlining, right?
> 
> Yes, although sid_to_context is meant to be analog to the other
> get_peersec calls, and should really be made a proper part of the
> interface (can be done later, correctness is the issue at hand).

Yes, Catherine was told that she shouldn't be directly exporting
security_sid_to_context, and was allegedly working on a fix.  Note
however that the expected solution is not a LSM interface but a set of
properly encapsulated interfaces exported directly from SELinux, based
on the iptables context matching patches by James.  The same style of
interface is being put forth for the audit LSPP work.  The indirection
of LSM serves no purpose here, as these users are specifically looking
for functionality provided only by SELinux.

> I don't expect security_sk_sid() to be terribly expensive.  It's not
> an AVC check, it's just propagating a label.  But I've not done any
> benchmarking on that.

No permission check there, but it looks like it does read lock
sk_callback_lock.  Not sure if that is truly justified here.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Stephen Smalley]

On Tue, 2006-03-21 at 08:32 -0500, Stephen Smalley wrote:
> > I don't expect security_sk_sid() to be terribly expensive.  It's not
> > an AVC check, it's just propagating a label.  But I've not done any
> > benchmarking on that.
> 
> No permission check there, but it looks like it does read lock
> sk_callback_lock.  Not sure if that is truly justified here.

Ah, that is because it is also called from the xfrm code, introduced by
Trent's patches.  But that locking shouldn't be necessary from scm_send,
right?  So she likely wants a separate hook for it to avoid that
overhead, or even just a direct SELinux interface?
  
-- 
Stephen Smalley
National Security Agency

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Xiaolan Zhang]

Hi, Stephen and James,

Looks like the selinux_sk_ctxid() call implemented in James' patch also 
requires the sk_callback_lock (see below).  I am planning to introduce a 
new exported fucntion selinux_sock_ctxid() which does not require any 
locking.  Comments?

thanks,
Catherine

Stephen Smalley <sds@tycho.nsa.gov> wrote on 03/21/2006 08:42:08 AM:

> On Tue, 2006-03-21 at 08:32 -0500, Stephen Smalley wrote:
> > > I don't expect security_sk_sid() to be terribly expensive.  It's not
> > > an AVC check, it's just propagating a label.  But I've not done any
> > > benchmarking on that.
> > 
> > No permission check there, but it looks like it does read lock
> > sk_callback_lock.  Not sure if that is truly justified here.
> 
> Ah, that is because it is also called from the xfrm code, introduced by
> Trent's patches.  But that locking shouldn't be necessary from scm_send,
> right?  So she likely wants a separate hook for it to avoid that
> overhead, or even just a direct SELinux interface?
> 
> -- 
> Stephen Smalley
> National Security Agency
> 

Re: [PATCH] scm: fold __scm_send() into scm_send()

[David S. Miller]

From: Chris Wright <chrisw@sous-sol.org>
Date: Mon, 20 Mar 2006 13:36:36 -0800

> The point of Catherine's original patch was to make sure there's always
> a security identifier associated with AF_UNIX messages.  So receiver
> can always check it (same as having credentials even w/out sender
> control message passing them).  Now we will have garbage for sid.

I'm seriously considering backing out Catherine's AF_UNIX patch from
the net-2.6.17 tree before submitting it to Linus later today so that
none of this crap goes in right now.

It appears that this needs a lot more sorting out, so for now that's
probably the right thing to do.

Re: [PATCH] scm: fold __scm_send() into scm_send()

[Chris Wright]

* David S. Miller (davem@davemloft.net) wrote:
> From: Chris Wright <chrisw@sous-sol.org>
> Date: Mon, 20 Mar 2006 13:36:36 -0800
> 
> > The point of Catherine's original patch was to make sure there's always
> > a security identifier associated with AF_UNIX messages.  So receiver
> > can always check it (same as having credentials even w/out sender
> > control message passing them).  Now we will have garbage for sid.
> 
> I'm seriously considering backing out Catherine's AF_UNIX patch from
> the net-2.6.17 tree before submitting it to Linus later today so that
> none of this crap goes in right now.
> 
> It appears that this needs a lot more sorting out, so for now that's
> probably the right thing to do.

I won't object.  I checked your tree, it looks OK to me.  The actual
broken patch appears in -mm, and the security_sid_to_context snafu
is primarily cosmetic at this point (the exports, etc fixed the real
compilation issues AFAICT).  But, again, if you want to drop that's fine
w/ me.  I'm sure Catherine can cleanup and resend as needed.

thanks,
-chris

Re: [PATCH] scm: fold __scm_send() into scm_send()

[James Morris]

On Mon, 20 Mar 2006, David S. Miller wrote:

> I'm seriously considering backing out Catherine's AF_UNIX patch from
> the net-2.6.17 tree before submitting it to Linus later today so that
> none of this crap goes in right now.

I believe Catherine is away this week, so it's probably best to drop the 
code and wait till she gets back and we can get it 100% right.

Sorry, this is my fault, I should have caught this problem.



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

Re: [PATCH] scm: fold __scm_send() into scm_send()

[David S. Miller]

From: James Morris <jmorris@namei.org>
Date: Mon, 20 Mar 2006 19:37:51 -0500 (EST)

> I believe Catherine is away this week, so it's probably best to drop the 
> code and wait till she gets back and we can get it 100% right.

Ok, agreed.

> Sorry, this is my fault, I should have caught this problem.

No worries.