From: [email protected]
Date: Sun, 12 Mar 2006 17:37:34 -0800
> From: Andrew Morton <[email protected]>
>
> Doh.
All applied, thanks. :-)
From: Ingo Oeser <[email protected]>
Fold __scm_send() into scm_send() and remove that interface completly
from the kernel.
Signed-off-by: Ingo Oeser <[email protected]>
---
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);
On Mon, Mar 13, 2006 at 09:05:31PM +0100, Ingo Oeser wrote:
> From: Ingo Oeser <[email protected]>
>
> 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: <[email protected]>.
Benjamin LaHaise <[email protected]> wrote:
>
> On Mon, Mar 13, 2006 at 09:05:31PM +0100, Ingo Oeser wrote:
> > From: Ingo Oeser <[email protected]>
> >
> > 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...
Ingo Oeser <[email protected]> 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...
Hi Chris,
Andrew Morton wrote:
> Ingo Oeser <[email protected]> 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: [email protected] is simply [email protected] at work :-)
* Ingo Oeser ([email protected]) wrote:
> Hi Chris,
>
> Andrew Morton wrote:
> > Ingo Oeser <[email protected]> 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
* Chris Wright ([email protected]) wrote:
> * Ingo Oeser ([email protected]) wrote:
> > Hi Chris,
> >
> > Andrew Morton wrote:
> > > Ingo Oeser <[email protected]> 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
Chris Wright <[email protected]> wrote:
>
> * Chris Wright ([email protected]) wrote:
> > * Ingo Oeser ([email protected]) wrote:
> > > Hi Chris,
> > >
> > > Andrew Morton wrote:
> > > > Ingo Oeser <[email protected]> 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.
* Andrew Morton ([email protected]) wrote:
> Chris Wright <[email protected]> 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
From: Chris Wright <[email protected]>
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.
* David S. Miller ([email protected]) wrote:
> From: Chris Wright <[email protected]>
> 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
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
<[email protected]>
From: James Morris <[email protected]>
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.
On Mon, 2006-03-20 at 15:15 -0800, Chris Wright wrote:
> * Andrew Morton ([email protected]) wrote:
> > Chris Wright <[email protected]> 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
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