Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Fri, 2013-04-05 at 00:47 -0400, David Miller wrote: > From: Sven Joachim > Date: Wed, 03 Apr 2013 13:41:32 +0200 > > > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: > > > >> 3.8-stable review patch. If anyone has any objections, please let me know. > > > > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and > > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the > > patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 > > here, and 65534 is the uid of user "nobody". > > Greg and Ben, please stop this patch from all of the -stable trees. > > I'm going to revert it and use Eric Biederman's fix instead. > > Thanks! OK, dropped it. Ben. -- Ben Hutchings I'm not a reverse psychological virus. Please don't copy me into your sig. signature.asc Description: This is a digitally signed message part
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Fri, 2013-04-05 at 00:47 -0400, David Miller wrote: From: Sven Joachim svenj...@gmx.de Date: Wed, 03 Apr 2013 13:41:32 +0200 On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: 3.8-stable review patch. If anyone has any objections, please let me know. I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and 3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 here, and 65534 is the uid of user nobody. Greg and Ben, please stop this patch from all of the -stable trees. I'm going to revert it and use Eric Biederman's fix instead. Thanks! OK, dropped it. Ben. -- Ben Hutchings I'm not a reverse psychological virus. Please don't copy me into your sig. signature.asc Description: This is a digitally signed message part
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Fri, Apr 05, 2013 at 12:47:15AM -0400, David Miller wrote: > From: Sven Joachim > Date: Wed, 03 Apr 2013 13:41:32 +0200 > > > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: > > > >> 3.8-stable review patch. If anyone has any objections, please let me know. > > > > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and > > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the > > patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 > > here, and 65534 is the uid of user "nobody". > > Greg and Ben, please stop this patch from all of the -stable trees. Now dropped from the 3.4.x and 3.8.x -stable trees (it wasn't part of the 3.0.x network patches that I can tell). > I'm going to revert it and use Eric Biederman's fix instead. That seems better, breaking older versions of udev, while fun at times, did seem to annoy a bunch of Ubuntu and Debian users :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Fri, Apr 05, 2013 at 12:47:15AM -0400, David Miller wrote: From: Sven Joachim svenj...@gmx.de Date: Wed, 03 Apr 2013 13:41:32 +0200 On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: 3.8-stable review patch. If anyone has any objections, please let me know. I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and 3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 here, and 65534 is the uid of user nobody. Greg and Ben, please stop this patch from all of the -stable trees. Now dropped from the 3.4.x and 3.8.x -stable trees (it wasn't part of the 3.0.x network patches that I can tell). I'm going to revert it and use Eric Biederman's fix instead. That seems better, breaking older versions of udev, while fun at times, did seem to annoy a bunch of Ubuntu and Debian users :) thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
From: Sven Joachim Date: Wed, 03 Apr 2013 13:41:32 +0200 > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: > >> 3.8-stable review patch. If anyone has any objections, please let me know. > > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the > patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 > here, and 65534 is the uid of user "nobody". Greg and Ben, please stop this patch from all of the -stable trees. I'm going to revert it and use Eric Biederman's fix instead. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
From: Sven Joachim svenj...@gmx.de Date: Wed, 03 Apr 2013 13:41:32 +0200 On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: 3.8-stable review patch. If anyone has any objections, please let me know. I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and 3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 here, and 65534 is the uid of user nobody. Greg and Ben, please stop this patch from all of the -stable trees. I'm going to revert it and use Eric Biederman's fix instead. Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Andy Lutomirski writes:
> On Wed, Apr 3, 2013 at 5:47 PM, Eric W. Biederman
> wrote:
>>
>> No. The patch is still bogus.
>>
>> If the problem is that we are not coallescing messages in stream_recvmsg
>> we need a different fix.
>>
>> Probably something like:
>>
>> if (check_creds) {
>> /* Never glue messages from different writers */
>> if ((UNIXCB(skb).pid != siocb->scm->pid) ||
>> (UNIXCB(skb).cred != siocb->scm->cred))
>> break;
>> - } else {
>> + } else if (test_bit(SOCK_PASSCRED, >flags)) {
>> /* Copy credentials */
>> scm_set_cred(siocb->scm, UNIXCB(skb).pid,
>> UNIXCB(skb).cred);
>> check_creds = 1;
>> }
>
> I'm confused. Isn't this making the problem worse, not better?
For udev that is a don't care.
For the case where we are coallescing messages this ensures we always
collaesce messages if we don't care about the credentials. Which turns
out to be a fix for a long standing pessimization, that no on has
bother to complain about.
> With my patches, the cost should go way down and it could be made
> unconditional, but that's still probably not a good -stable change.
Reducing the cost and the complexity as far as we can is good, but we
really want small steps as we optimize the case of sending credentials.
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Eric Dumazet writes: > On Wed, 2013-04-03 at 17:19 -0700, Eric Dumazet wrote: > >> Well, yes, this commit fixes a real bug : We were coalescing two >> messages into a single one, even if the senders were different. > > By the way, the 'LSB' test program can be found here : > > https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 And we have two sends and one recvfrom, and no loops. So the the problem must be a failure to coalesce messages. There is a race between creating the sending and receiving sockets. At a first glance that race looks like we put the cred on the first message and not on the second message because we are connected by the time the second messages is sent. Which would definitely cause a failure to coallesce messages. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, Apr 3, 2013 at 5:47 PM, Eric W. Biederman wrote:
> Eric Dumazet writes:
>
>> On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
>>> Sven Joachim writes:
>>>
>>> > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
>>> >
>>> >> 3.8-stable review patch. If anyone has any objections, please let me
>>> >> know.
>>> >
>>> > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
>>> > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the
>>> > patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
>>> > here, and 65534 is the uid of user "nobody".
>>>
>>> Hmm.
>>>
>>> Ok. I don't understand the commit that was being backported here. I am
>>> pretty certain it a fix for a problem that did not exist.
>>>
>>> Unless I am completely mis-reading scm_recv we only generate a
>>> SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
>>> Which means that the only harm that can come from adding scm credentials
>>> to a disconnected af_unix socket is a loss in efficiency.
>>>
>>> Not adding scm credentials to be passed to userspace as the commit below
>>> is doing can result is bogus data being passed to userspace. Which is
>>> very actively WRONG.
>>>
>>> Now before scm_recv does anything we first call scm_set_cred. If no
>>> credential was passed to scm_set_cred we set the uid to INVALID_UID.
>>> Which scm_recv in the call from_kuid_munged translates into 65534 for
>>> reporting to userspace.
>>>
>>> So this is is pretty clearly a case of us not sending the unix
>>> credentials.
>>>
>>> Since not sending credential is just a performance optimization I can
>>> see no earthly reason why the commit below should have been applied in
>>> the first place, and no reason why it should have been backported in the
>>> second place. So my vote is that we revert this bogus commit. Upstream
>>> and then backport the revert.
>>>
>>> Am I missing something?
>>
>> Well, yes, this commit fixes a real bug : We were coalescing two
>> messages into a single one, even if the senders were different.
>
> What???
>
> As far as I can tell this patch can only server to _allow_ coalescing two
> messages into a single one.
>
>> Copy of a reply I did :
>>
>> So the problem is that two messages have different credentials,
>> because other->sk_socket changed between first and second message.
>
>
>> and unix_stream_recvmsg() has the following check :
>>
>> if (check_creds) {
>> /* Never glue messages from different writers */
>> if ((UNIXCB(skb).pid != siocb->scm->pid) ||
>> (UNIXCB(skb).cred != siocb->scm->cred))
>> break;
>> } else {
>> /* Copy credentials */
>> scm_set_cred(siocb->scm, UNIXCB(skb).pid,
>> UNIXCB(skb).cred);
>> check_creds = 1;
>> }
>>
>> So the patch was good, and we need a followup, like the one I posted
>> today ?
>
> No. The patch is still bogus.
>
> If the problem is that we are not coallescing messages in stream_recvmsg
> we need a different fix.
>
> Probably something like:
>
> if (check_creds) {
> /* Never glue messages from different writers */
> if ((UNIXCB(skb).pid != siocb->scm->pid) ||
> (UNIXCB(skb).cred != siocb->scm->cred))
> break;
> - } else {
> + } else if (test_bit(SOCK_PASSCRED, >flags)) {
> /* Copy credentials */
> scm_set_cred(siocb->scm, UNIXCB(skb).pid,
> UNIXCB(skb).cred);
> check_creds = 1;
> }
I'm confused. Isn't this making the problem worse, not better?
The original don't-always-pass-creds logic came from (I think):
commit 16e5726269611b71c930054ffe9b858c1cea88eb
Author: Eric Dumazet
Date: Mon Sep 19 05:52:27 2011 +
af_unix: dont send SCM_CREDENTIALS by default
Since commit 7361c36c5224 (af_unix: Allow credentials to work across
user and pid namespaces) af_unix performance dropped a lot.
This is because we now take a reference on pid and cred in each write(),
and release them in read(), usually done from another process,
eventually from another cpu. This triggers false sharing.
With my patches, the cost should go way down and it could be made
unconditional, but that's still probably not a good -stable change.
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Eric Dumazet writes:
> On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
>> Sven Joachim writes:
>>
>> > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
>> >
>> >> 3.8-stable review patch. If anyone has any objections, please let me
>> >> know.
>> >
>> > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
>> > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the
>> > patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
>> > here, and 65534 is the uid of user "nobody".
>>
>> Hmm.
>>
>> Ok. I don't understand the commit that was being backported here. I am
>> pretty certain it a fix for a problem that did not exist.
>>
>> Unless I am completely mis-reading scm_recv we only generate a
>> SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
>> Which means that the only harm that can come from adding scm credentials
>> to a disconnected af_unix socket is a loss in efficiency.
>>
>> Not adding scm credentials to be passed to userspace as the commit below
>> is doing can result is bogus data being passed to userspace. Which is
>> very actively WRONG.
>>
>> Now before scm_recv does anything we first call scm_set_cred. If no
>> credential was passed to scm_set_cred we set the uid to INVALID_UID.
>> Which scm_recv in the call from_kuid_munged translates into 65534 for
>> reporting to userspace.
>>
>> So this is is pretty clearly a case of us not sending the unix
>> credentials.
>>
>> Since not sending credential is just a performance optimization I can
>> see no earthly reason why the commit below should have been applied in
>> the first place, and no reason why it should have been backported in the
>> second place. So my vote is that we revert this bogus commit. Upstream
>> and then backport the revert.
>>
>> Am I missing something?
>
> Well, yes, this commit fixes a real bug : We were coalescing two
> messages into a single one, even if the senders were different.
What???
As far as I can tell this patch can only server to _allow_ coalescing two
messages into a single one.
> Copy of a reply I did :
>
> So the problem is that two messages have different credentials,
> because other->sk_socket changed between first and second message.
> and unix_stream_recvmsg() has the following check :
>
> if (check_creds) {
> /* Never glue messages from different writers */
> if ((UNIXCB(skb).pid != siocb->scm->pid) ||
> (UNIXCB(skb).cred != siocb->scm->cred))
> break;
> } else {
> /* Copy credentials */
> scm_set_cred(siocb->scm, UNIXCB(skb).pid,
> UNIXCB(skb).cred);
> check_creds = 1;
> }
>
> So the patch was good, and we need a followup, like the one I posted
> today ?
No. The patch is still bogus.
If the problem is that we are not coallescing messages in stream_recvmsg
we need a different fix.
Probably something like:
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb->scm->pid) ||
(UNIXCB(skb).cred != siocb->scm->cred))
break;
- } else {
+ } else if (test_bit(SOCK_PASSCRED, >flags)) {
/* Copy credentials */
scm_set_cred(siocb->scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
Although comapring comparing the applicable uids and gids might be
sensible as well.
> Some user apps dont know about uid 65534.
What??? The problem is that the app wanted the uid and we gave it
garbage. You can't fix wanting a valid uid by not passing a uid.
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 975cca0..42359d8 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock,
> struct msghdr *msg,
> return;
> }
>
> - if (test_bit(SOCK_PASSCRED, >flags)) {
> + if (test_bit(SOCK_PASSCRED, >flags) && scm->creds.pid) {
> struct user_namespace *current_ns = current_user_ns();
> struct ucred ucreds = {
> .pid = scm->creds.pid,
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 17:19 -0700, Eric Dumazet wrote: > Well, yes, this commit fixes a real bug : We were coalescing two > messages into a single one, even if the senders were different. By the way, the 'LSB' test program can be found here : https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
> Sven Joachim writes:
>
> > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
> >
> >> 3.8-stable review patch. If anyone has any objections, please let me know.
> >
> > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
> > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the
> > patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
> > here, and 65534 is the uid of user "nobody".
>
> Hmm.
>
> Ok. I don't understand the commit that was being backported here. I am
> pretty certain it a fix for a problem that did not exist.
>
> Unless I am completely mis-reading scm_recv we only generate a
> SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
> Which means that the only harm that can come from adding scm credentials
> to a disconnected af_unix socket is a loss in efficiency.
>
> Not adding scm credentials to be passed to userspace as the commit below
> is doing can result is bogus data being passed to userspace. Which is
> very actively WRONG.
>
> Now before scm_recv does anything we first call scm_set_cred. If no
> credential was passed to scm_set_cred we set the uid to INVALID_UID.
> Which scm_recv in the call from_kuid_munged translates into 65534 for
> reporting to userspace.
>
> So this is is pretty clearly a case of us not sending the unix
> credentials.
>
> Since not sending credential is just a performance optimization I can
> see no earthly reason why the commit below should have been applied in
> the first place, and no reason why it should have been backported in the
> second place. So my vote is that we revert this bogus commit. Upstream
> and then backport the revert.
>
> Am I missing something?
Well, yes, this commit fixes a real bug : We were coalescing two
messages into a single one, even if the senders were different.
Copy of a reply I did :
So the problem is that two messages have different credentials,
because other->sk_socket changed between first and second message.
and unix_stream_recvmsg() has the following check :
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb->scm->pid) ||
(UNIXCB(skb).cred != siocb->scm->cred))
break;
} else {
/* Copy credentials */
scm_set_cred(siocb->scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
So the patch was good, and we need a followup, like the one I posted today ?
Some user apps dont know about uid 65534.
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..42359d8 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock, struct
msghdr *msg,
return;
}
- if (test_bit(SOCK_PASSCRED, >flags)) {
+ if (test_bit(SOCK_PASSCRED, >flags) && scm->creds.pid) {
struct user_namespace *current_ns = current_user_ns();
struct ucred ucreds = {
.pid = scm->creds.pid,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Andy Lutomirski writes:
> On Wed, Apr 3, 2013 at 11:43 AM, Eric Dumazet wrote:
>> On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
>>
>>>
>>> This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
>>> race. There's a fix (that needs both a new version from me and a review
>>> by someone) here:
>>>
>>> http://www.spinics.net/lists/netdev/msg229948.html
>>
>> Hmm... this is not a stable candidate, IMHO.
>
> Agreed.
>
>>
>> This has to be fixed (if needed) in a more easy way.
>>
>> What about this one liner ?
>>
>> CC Eric W. Biederman as he wrote commit
>> dbe9a4173ea53b72b2c3
>> (scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
>>
>> diff --git a/include/net/scm.h b/include/net/scm.h
>> index 975cca0..42359d8 100644
>> --- a/include/net/scm.h
>> +++ b/include/net/scm.h
>> @@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock,
>> struct msghdr *msg,
>> return;
>> }
>>
>> - if (test_bit(SOCK_PASSCRED, >flags)) {
>> + if (test_bit(SOCK_PASSCRED, >flags) && scm->creds.pid) {
>> struct user_namespace *current_ns = current_user_ns();
>> struct ucred ucreds = {
>> .pid = scm->creds.pid,
>>
>>
>
> That looks like it's correct. If it gets applied, I'll respin my
> patches on top of it.
>
> (This approach may be a POSIX violation for all I know, and it's even
> possible that some really fragile userspace breaks. But I doubt it,
> and anything that will break as a result is already operating in a
> highly confused state; hence the original problem.)
It certainly looks like we are not giving userspace what userspace asked
for, which can break in all kinds of subtle ways. And I can't possibly
see how not giving udev any information will when udev asked for the
sender will fix anything.
I think we need to answer why in the world do we do not want to pass
credentials from an unconnected unix mode socket, before we ask
why don't we want to deliver credentials that we didn't pass when
passing of credentials was explicitly requested.
If the only concern about the LSB test case is performance I think we
need to revert the original commit and just stop passing a struct cred
pointer. If there is a concern about the data I think we need a better
explanation of what those LSB test cases were that broke.
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Sven Joachim writes:
> On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
>
>> 3.8-stable review patch. If anyone has any objections, please let me know.
>
> I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
> 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the
> patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
> here, and 65534 is the uid of user "nobody".
Hmm.
Ok. I don't understand the commit that was being backported here. I am
pretty certain it a fix for a problem that did not exist.
Unless I am completely mis-reading scm_recv we only generate a
SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
Which means that the only harm that can come from adding scm credentials
to a disconnected af_unix socket is a loss in efficiency.
Not adding scm credentials to be passed to userspace as the commit below
is doing can result is bogus data being passed to userspace. Which is
very actively WRONG.
Now before scm_recv does anything we first call scm_set_cred. If no
credential was passed to scm_set_cred we set the uid to INVALID_UID.
Which scm_recv in the call from_kuid_munged translates into 65534 for
reporting to userspace.
So this is is pretty clearly a case of us not sending the unix
credentials.
Since not sending credential is just a performance optimization I can
see no earthly reason why the commit below should have been applied in
the first place, and no reason why it should have been backported in the
second place. So my vote is that we revert this bogus commit. Upstream
and then backport the revert.
Am I missing something?
Eric
>> From: dingtianhong
>>
>> [ Upstream commit 14134f6584212d585b310ce95428014b653dfaf6 ]
>>
>> SCM_SCREDENTIALS should apply to write() syscalls only either source or
>> destination
>> socket asserted SOCK_PASSCRED. The original implememtation in
>> maybe_add_creds is wrong,
>> and breaks several LSB testcases ( i.e.
>> /tset/LSB.os/netowkr/recvfrom/T.recvfrom).
>>
>> Origionally-authored-by: Karel Srot
>> Signed-off-by: Ding Tianhong
>> Acked-by: Eric Dumazet
>> Signed-off-by: David S. Miller
>> Signed-off-by: Greg Kroah-Hartman
>> ---
>> net/unix/af_unix.c |4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1414,8 +1414,8 @@ static void maybe_add_creds(struct sk_bu
>> if (UNIXCB(skb).cred)
>> return;
>> if (test_bit(SOCK_PASSCRED, >flags) ||
>> -!other->sk_socket ||
>> -test_bit(SOCK_PASSCRED, >sk_socket->flags)) {
>> +(other->sk_socket &&
>> +test_bit(SOCK_PASSCRED, >sk_socket->flags))) {
>> UNIXCB(skb).pid = get_pid(task_tgid(current));
>> UNIXCB(skb).cred = get_current_cred();
>> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Hi On Wednesday 03 April 2013, Greg Kroah-Hartman wrote: > On Wed, Apr 03, 2013 at 05:10:40PM +0200, Sven Joachim wrote: > > On 2013-04-03 16:00 +0200, Eric Dumazet wrote: > > > On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: > > >> On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: > > >> > 3.8-stable review patch. If anyone has any objections, please let me > > >> > know. > > >> > > >> I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and > > >> 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the > > >> patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 > > >> here, and 65534 is the uid of user "nobody". […] I can reproduce this issue on several systems with a Debian unstable (udev 175, same version as in the upcoming Debian 7.0 'wheezy') userland. Reverting only this patch from 3.8.6-rc1 avoids the problem. > > > It might be a wrong sender (application bug or bad identity), and udevd > > > correctly discards the incoming message. > > > > How would I find out the culprit? > > Try running 'udevadm monitor' as root and see if something shows up > there. There are no re-occuring messages on an idle system, connecting a USB stick (not mounting) results in these messages. broken (3.8.6-rc1, with "af_unix: dont send SCM_CREDENTIAL when dest socket is NULL" applied): KERNEL[11739.713368] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1 (usb) KERNEL[11739.713514] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0 (usb) UDEV [11739.713663] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1 (usb) KERNEL[11739.713677] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7 (scsi) KERNEL[11739.713688] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/scsi_host/host7 (scsi_host) UDEV [11739.713734] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0 (usb) UDEV [11739.713797] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7 (scsi) UDEV [11739.713893] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/scsi_host/host7 (scsi_host) UDEV [11739.718583] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1 (usb) UDEV [11739.720709] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0 (usb) UDEV [11739.720900] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7 (scsi) UDEV [11739.721191] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/scsi_host/host7 (scsi_host) KERNEL[11740.747996] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0 (scsi) KERNEL[11740.748032] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0 (scsi) KERNEL[11740.748042] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_disk/7:0:0:0 (scsi_disk) KERNEL[11740.748048] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_device/7:0:0:0 (scsi_device) KERNEL[11740.748103] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_generic/sg5 (scsi_generic) UDEV [11740.748129] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0 (scsi) KERNEL[11740.748146] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/bsg/7:0:0:0 (bsg) UDEV [11740.748251] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0 (scsi) UDEV [11740.748349] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0 (scsi) UDEV [11740.748459] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_disk/7:0:0:0 (scsi_disk) UDEV [11740.748533] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_device/7:0:0:0 (scsi_device) UDEV [11740.748910] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_generic/sg5 (scsi_generic) UDEV [11740.749039] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/bsg/7:0:0:0 (bsg) UDEV [11740.750204] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0 (scsi) UDEV [11740.750531] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_disk/7:0:0:0 (scsi_disk) UDEV [11740.750652] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_device/7:0:0:0 (scsi_device) UDEV [11740.751143] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/bsg/7:0:0:0 (bsg) UDEV [11740.751323] add
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, Apr 3, 2013 at 11:43 AM, Eric Dumazet wrote:
> On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
>
>>
>> This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
>> race. There's a fix (that needs both a new version from me and a review
>> by someone) here:
>>
>> http://www.spinics.net/lists/netdev/msg229948.html
>
> Hmm... this is not a stable candidate, IMHO.
Agreed.
>
> This has to be fixed (if needed) in a more easy way.
>
> What about this one liner ?
>
> CC Eric W. Biederman as he wrote commit
> dbe9a4173ea53b72b2c3
> (scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
>
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 975cca0..42359d8 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock,
> struct msghdr *msg,
> return;
> }
>
> - if (test_bit(SOCK_PASSCRED, >flags)) {
> + if (test_bit(SOCK_PASSCRED, >flags) && scm->creds.pid) {
> struct user_namespace *current_ns = current_user_ns();
> struct ucred ucreds = {
> .pid = scm->creds.pid,
>
>
That looks like it's correct. If it gets applied, I'll respin my
patches on top of it.
(This approach may be a POSIX violation for all I know, and it's even
possible that some really fragile userspace breaks. But I doubt it,
and anything that will break as a result is already operating in a
highly confused state; hence the original problem.)
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
>
> This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
> race. There's a fix (that needs both a new version from me and a review
> by someone) here:
>
> http://www.spinics.net/lists/netdev/msg229948.html
Hmm... this is not a stable candidate, IMHO.
This has to be fixed (if needed) in a more easy way.
What about this one liner ?
CC Eric W. Biederman as he wrote commit
dbe9a4173ea53b72b2c3
(scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..42359d8 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock, struct
msghdr *msg,
return;
}
- if (test_bit(SOCK_PASSCRED, >flags)) {
+ if (test_bit(SOCK_PASSCRED, >flags) && scm->creds.pid) {
struct user_namespace *current_ns = current_user_ns();
struct ucred ucreds = {
.pid = scm->creds.pid,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On 04/03/2013 08:35 AM, Eric Dumazet wrote: > On Wed, 2013-04-03 at 17:10 +0200, Sven Joachim wrote: >> On 2013-04-03 16:00 +0200, Eric Dumazet wrote: > >> >>> It might be a wrong sender (application bug or bad identity), and udevd >>> correctly discards the incoming message. >> >> How would I find out the culprit? > > Change udevd to display the pid as well, and hopefully track the sender. > > udevd receives uid and pid in the credentials. This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a race. There's a fix (that needs both a new version from me and a review by someone) here: http://www.spinics.net/lists/netdev/msg229948.html --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 17:10 +0200, Sven Joachim wrote: > On 2013-04-03 16:00 +0200, Eric Dumazet wrote: > > > It might be a wrong sender (application bug or bad identity), and udevd > > correctly discards the incoming message. > > How would I find out the culprit? Change udevd to display the pid as well, and hopefully track the sender. udevd receives uid and pid in the credentials. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, Apr 03, 2013 at 05:10:40PM +0200, Sven Joachim wrote: > On 2013-04-03 16:00 +0200, Eric Dumazet wrote: > > > On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: > >> On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: > >> > >> > 3.8-stable review patch. If anyone has any objections, please let me > >> > know. > >> > >> I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and > >> 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the > >> patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 > >> here, and 65534 is the uid of user "nobody". > > > > And if you use a 3.1 kernel (before commit > > 16e5726269611b71c930054ffe9b858c1cea88eb) are you seeing this message ? > > No (tested with 3.1.10). > > > It might be a wrong sender (application bug or bad identity), and udevd > > correctly discards the incoming message. > > How would I find out the culprit? Try running 'udevadm monitor' as root and see if something shows up there. I can't reproduce this here, running a newer version of udev (195), sorry, I don't have any systems with older udev releases. Note, someone else posted this same error earlier today on the linux-usb mailing list, saying that USB storage devices would not automount anymore. Does that work properly for you? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On 2013-04-03 16:00 +0200, Eric Dumazet wrote: > On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: >> On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: >> >> > 3.8-stable review patch. If anyone has any objections, please let me know. >> >> I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and >> 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the >> patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 >> here, and 65534 is the uid of user "nobody". > > And if you use a 3.1 kernel (before commit > 16e5726269611b71c930054ffe9b858c1cea88eb) are you seeing this message ? No (tested with 3.1.10). > It might be a wrong sender (application bug or bad identity), and udevd > correctly discards the incoming message. How would I find out the culprit? Cheers, Sven -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: > On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: > > > 3.8-stable review patch. If anyone has any objections, please let me know. > > I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and > 3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the > patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 > here, and 65534 is the uid of user "nobody". And if you use a 3.1 kernel (before commit 16e5726269611b71c930054ffe9b858c1cea88eb) are you seeing this message ? It might be a wrong sender (application bug or bad identity), and udevd correctly discards the incoming message. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
> 3.8-stable review patch. If anyone has any objections, please let me know.
I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
3.9-rc5: "udevd[56]: sender uid=65534, message ignored". Reverting the
patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
here, and 65534 is the uid of user "nobody".
Cheers,
Sven
> From: dingtianhong
>
> [ Upstream commit 14134f6584212d585b310ce95428014b653dfaf6 ]
>
> SCM_SCREDENTIALS should apply to write() syscalls only either source or
> destination
> socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds
> is wrong,
> and breaks several LSB testcases ( i.e.
> /tset/LSB.os/netowkr/recvfrom/T.recvfrom).
>
> Origionally-authored-by: Karel Srot
> Signed-off-by: Ding Tianhong
> Acked-by: Eric Dumazet
> Signed-off-by: David S. Miller
> Signed-off-by: Greg Kroah-Hartman
> ---
> net/unix/af_unix.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1414,8 +1414,8 @@ static void maybe_add_creds(struct sk_bu
> if (UNIXCB(skb).cred)
> return;
> if (test_bit(SOCK_PASSCRED, >flags) ||
> - !other->sk_socket ||
> - test_bit(SOCK_PASSCRED, >sk_socket->flags)) {
> + (other->sk_socket &&
> + test_bit(SOCK_PASSCRED, >sk_socket->flags))) {
> UNIXCB(skb).pid = get_pid(task_tgid(current));
> UNIXCB(skb).cred = get_current_cred();
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Andy Lutomirski l...@amacapital.net writes:
On Wed, Apr 3, 2013 at 5:47 PM, Eric W. Biederman ebied...@xmission.com
wrote:
No. The patch is still bogus.
If the problem is that we are not coallescing messages in stream_recvmsg
we need a different fix.
Probably something like:
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb-scm-pid) ||
(UNIXCB(skb).cred != siocb-scm-cred))
break;
- } else {
+ } else if (test_bit(SOCK_PASSCRED, sock-flags)) {
/* Copy credentials */
scm_set_cred(siocb-scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
I'm confused. Isn't this making the problem worse, not better?
For udev that is a don't care.
For the case where we are coallescing messages this ensures we always
collaesce messages if we don't care about the credentials. Which turns
out to be a fix for a long standing pessimization, that no on has
bother to complain about.
With my patches, the cost should go way down and it could be made
unconditional, but that's still probably not a good -stable change.
Reducing the cost and the complexity as far as we can is good, but we
really want small steps as we optimize the case of sending credentials.
Eric
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
3.8-stable review patch. If anyone has any objections, please let me know.
I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the
patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
here, and 65534 is the uid of user nobody.
Cheers,
Sven
From: dingtianhong dingtianh...@huawei.com
[ Upstream commit 14134f6584212d585b310ce95428014b653dfaf6 ]
SCM_SCREDENTIALS should apply to write() syscalls only either source or
destination
socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds
is wrong,
and breaks several LSB testcases ( i.e.
/tset/LSB.os/netowkr/recvfrom/T.recvfrom).
Origionally-authored-by: Karel Srot ks...@redhat.com
Signed-off-by: Ding Tianhong dingtianh...@huawei.com
Acked-by: Eric Dumazet eduma...@google.com
Signed-off-by: David S. Miller da...@davemloft.net
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
---
net/unix/af_unix.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1414,8 +1414,8 @@ static void maybe_add_creds(struct sk_bu
if (UNIXCB(skb).cred)
return;
if (test_bit(SOCK_PASSCRED, sock-flags) ||
- !other-sk_socket ||
- test_bit(SOCK_PASSCRED, other-sk_socket-flags)) {
+ (other-sk_socket
+ test_bit(SOCK_PASSCRED, other-sk_socket-flags))) {
UNIXCB(skb).pid = get_pid(task_tgid(current));
UNIXCB(skb).cred = get_current_cred();
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: 3.8-stable review patch. If anyone has any objections, please let me know. I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and 3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 here, and 65534 is the uid of user nobody. And if you use a 3.1 kernel (before commit 16e5726269611b71c930054ffe9b858c1cea88eb) are you seeing this message ? It might be a wrong sender (application bug or bad identity), and udevd correctly discards the incoming message. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On 2013-04-03 16:00 +0200, Eric Dumazet wrote: On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: 3.8-stable review patch. If anyone has any objections, please let me know. I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and 3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 here, and 65534 is the uid of user nobody. And if you use a 3.1 kernel (before commit 16e5726269611b71c930054ffe9b858c1cea88eb) are you seeing this message ? No (tested with 3.1.10). It might be a wrong sender (application bug or bad identity), and udevd correctly discards the incoming message. How would I find out the culprit? Cheers, Sven -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, Apr 03, 2013 at 05:10:40PM +0200, Sven Joachim wrote: On 2013-04-03 16:00 +0200, Eric Dumazet wrote: On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: 3.8-stable review patch. If anyone has any objections, please let me know. I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and 3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 here, and 65534 is the uid of user nobody. And if you use a 3.1 kernel (before commit 16e5726269611b71c930054ffe9b858c1cea88eb) are you seeing this message ? No (tested with 3.1.10). It might be a wrong sender (application bug or bad identity), and udevd correctly discards the incoming message. How would I find out the culprit? Try running 'udevadm monitor' as root and see if something shows up there. I can't reproduce this here, running a newer version of udev (195), sorry, I don't have any systems with older udev releases. Note, someone else posted this same error earlier today on the linux-usb mailing list, saying that USB storage devices would not automount anymore. Does that work properly for you? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 17:10 +0200, Sven Joachim wrote: On 2013-04-03 16:00 +0200, Eric Dumazet wrote: It might be a wrong sender (application bug or bad identity), and udevd correctly discards the incoming message. How would I find out the culprit? Change udevd to display the pid as well, and hopefully track the sender. udevd receives uid and pid in the credentials. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On 04/03/2013 08:35 AM, Eric Dumazet wrote: On Wed, 2013-04-03 at 17:10 +0200, Sven Joachim wrote: On 2013-04-03 16:00 +0200, Eric Dumazet wrote: It might be a wrong sender (application bug or bad identity), and udevd correctly discards the incoming message. How would I find out the culprit? Change udevd to display the pid as well, and hopefully track the sender. udevd receives uid and pid in the credentials. This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a race. There's a fix (that needs both a new version from me and a review by someone) here: http://www.spinics.net/lists/netdev/msg229948.html --Andy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
race. There's a fix (that needs both a new version from me and a review
by someone) here:
http://www.spinics.net/lists/netdev/msg229948.html
Hmm... this is not a stable candidate, IMHO.
This has to be fixed (if needed) in a more easy way.
What about this one liner ?
CC Eric W. Biederman as he wrote commit
dbe9a4173ea53b72b2c3
(scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..42359d8 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock, struct
msghdr *msg,
return;
}
- if (test_bit(SOCK_PASSCRED, sock-flags)) {
+ if (test_bit(SOCK_PASSCRED, sock-flags) scm-creds.pid) {
struct user_namespace *current_ns = current_user_ns();
struct ucred ucreds = {
.pid = scm-creds.pid,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, Apr 3, 2013 at 11:43 AM, Eric Dumazet eric.duma...@gmail.com wrote:
On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
race. There's a fix (that needs both a new version from me and a review
by someone) here:
http://www.spinics.net/lists/netdev/msg229948.html
Hmm... this is not a stable candidate, IMHO.
Agreed.
This has to be fixed (if needed) in a more easy way.
What about this one liner ?
CC Eric W. Biederman as he wrote commit
dbe9a4173ea53b72b2c3
(scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..42359d8 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock,
struct msghdr *msg,
return;
}
- if (test_bit(SOCK_PASSCRED, sock-flags)) {
+ if (test_bit(SOCK_PASSCRED, sock-flags) scm-creds.pid) {
struct user_namespace *current_ns = current_user_ns();
struct ucred ucreds = {
.pid = scm-creds.pid,
That looks like it's correct. If it gets applied, I'll respin my
patches on top of it.
(This approach may be a POSIX violation for all I know, and it's even
possible that some really fragile userspace breaks. But I doubt it,
and anything that will break as a result is already operating in a
highly confused state; hence the original problem.)
--Andy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Hi On Wednesday 03 April 2013, Greg Kroah-Hartman wrote: On Wed, Apr 03, 2013 at 05:10:40PM +0200, Sven Joachim wrote: On 2013-04-03 16:00 +0200, Eric Dumazet wrote: On Wed, 2013-04-03 at 13:41 +0200, Sven Joachim wrote: On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote: 3.8-stable review patch. If anyone has any objections, please let me know. I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and 3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175 here, and 65534 is the uid of user nobody. […] I can reproduce this issue on several systems with a Debian unstable (udev 175, same version as in the upcoming Debian 7.0 'wheezy') userland. Reverting only this patch from 3.8.6-rc1 avoids the problem. It might be a wrong sender (application bug or bad identity), and udevd correctly discards the incoming message. How would I find out the culprit? Try running 'udevadm monitor' as root and see if something shows up there. There are no re-occuring messages on an idle system, connecting a USB stick (not mounting) results in these messages. broken (3.8.6-rc1, with af_unix: dont send SCM_CREDENTIAL when dest socket is NULL applied): KERNEL[11739.713368] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1 (usb) KERNEL[11739.713514] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0 (usb) UDEV [11739.713663] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1 (usb) KERNEL[11739.713677] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7 (scsi) KERNEL[11739.713688] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/scsi_host/host7 (scsi_host) UDEV [11739.713734] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0 (usb) UDEV [11739.713797] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7 (scsi) UDEV [11739.713893] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/scsi_host/host7 (scsi_host) UDEV [11739.718583] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1 (usb) UDEV [11739.720709] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0 (usb) UDEV [11739.720900] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7 (scsi) UDEV [11739.721191] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/scsi_host/host7 (scsi_host) KERNEL[11740.747996] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0 (scsi) KERNEL[11740.748032] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0 (scsi) KERNEL[11740.748042] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_disk/7:0:0:0 (scsi_disk) KERNEL[11740.748048] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_device/7:0:0:0 (scsi_device) KERNEL[11740.748103] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_generic/sg5 (scsi_generic) UDEV [11740.748129] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0 (scsi) KERNEL[11740.748146] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/bsg/7:0:0:0 (bsg) UDEV [11740.748251] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0 (scsi) UDEV [11740.748349] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0 (scsi) UDEV [11740.748459] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_disk/7:0:0:0 (scsi_disk) UDEV [11740.748533] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_device/7:0:0:0 (scsi_device) UDEV [11740.748910] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_generic/sg5 (scsi_generic) UDEV [11740.749039] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/bsg/7:0:0:0 (bsg) UDEV [11740.750204] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0 (scsi) UDEV [11740.750531] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_disk/7:0:0:0 (scsi_disk) UDEV [11740.750652] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_device/7:0:0:0 (scsi_device) UDEV [11740.751143] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/bsg/7:0:0:0 (bsg) UDEV [11740.751323] add /devices/pci:00/:00:1a.0/usb7/7-1/7-1.1/7-1.1:1.0/host7/target7:0:0/7:0:0:0/scsi_generic/sg5 (scsi_generic) KERNEL[11740.943534] add
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Sven Joachim svenj...@gmx.de writes:
On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
3.8-stable review patch. If anyone has any objections, please let me know.
I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the
patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
here, and 65534 is the uid of user nobody.
Hmm.
Ok. I don't understand the commit that was being backported here. I am
pretty certain it a fix for a problem that did not exist.
Unless I am completely mis-reading scm_recv we only generate a
SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
Which means that the only harm that can come from adding scm credentials
to a disconnected af_unix socket is a loss in efficiency.
Not adding scm credentials to be passed to userspace as the commit below
is doing can result is bogus data being passed to userspace. Which is
very actively WRONG.
Now before scm_recv does anything we first call scm_set_cred. If no
credential was passed to scm_set_cred we set the uid to INVALID_UID.
Which scm_recv in the call from_kuid_munged translates into 65534 for
reporting to userspace.
So this is is pretty clearly a case of us not sending the unix
credentials.
Since not sending credential is just a performance optimization I can
see no earthly reason why the commit below should have been applied in
the first place, and no reason why it should have been backported in the
second place. So my vote is that we revert this bogus commit. Upstream
and then backport the revert.
Am I missing something?
Eric
From: dingtianhong dingtianh...@huawei.com
[ Upstream commit 14134f6584212d585b310ce95428014b653dfaf6 ]
SCM_SCREDENTIALS should apply to write() syscalls only either source or
destination
socket asserted SOCK_PASSCRED. The original implememtation in
maybe_add_creds is wrong,
and breaks several LSB testcases ( i.e.
/tset/LSB.os/netowkr/recvfrom/T.recvfrom).
Origionally-authored-by: Karel Srot ks...@redhat.com
Signed-off-by: Ding Tianhong dingtianh...@huawei.com
Acked-by: Eric Dumazet eduma...@google.com
Signed-off-by: David S. Miller da...@davemloft.net
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
---
net/unix/af_unix.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1414,8 +1414,8 @@ static void maybe_add_creds(struct sk_bu
if (UNIXCB(skb).cred)
return;
if (test_bit(SOCK_PASSCRED, sock-flags) ||
-!other-sk_socket ||
-test_bit(SOCK_PASSCRED, other-sk_socket-flags)) {
+(other-sk_socket
+test_bit(SOCK_PASSCRED, other-sk_socket-flags))) {
UNIXCB(skb).pid = get_pid(task_tgid(current));
UNIXCB(skb).cred = get_current_cred();
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Andy Lutomirski l...@amacapital.net writes:
On Wed, Apr 3, 2013 at 11:43 AM, Eric Dumazet eric.duma...@gmail.com wrote:
On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
race. There's a fix (that needs both a new version from me and a review
by someone) here:
http://www.spinics.net/lists/netdev/msg229948.html
Hmm... this is not a stable candidate, IMHO.
Agreed.
This has to be fixed (if needed) in a more easy way.
What about this one liner ?
CC Eric W. Biederman as he wrote commit
dbe9a4173ea53b72b2c3
(scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..42359d8 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock,
struct msghdr *msg,
return;
}
- if (test_bit(SOCK_PASSCRED, sock-flags)) {
+ if (test_bit(SOCK_PASSCRED, sock-flags) scm-creds.pid) {
struct user_namespace *current_ns = current_user_ns();
struct ucred ucreds = {
.pid = scm-creds.pid,
That looks like it's correct. If it gets applied, I'll respin my
patches on top of it.
(This approach may be a POSIX violation for all I know, and it's even
possible that some really fragile userspace breaks. But I doubt it,
and anything that will break as a result is already operating in a
highly confused state; hence the original problem.)
It certainly looks like we are not giving userspace what userspace asked
for, which can break in all kinds of subtle ways. And I can't possibly
see how not giving udev any information will when udev asked for the
sender will fix anything.
I think we need to answer why in the world do we do not want to pass
credentials from an unconnected unix mode socket, before we ask
why don't we want to deliver credentials that we didn't pass when
passing of credentials was explicitly requested.
If the only concern about the LSB test case is performance I think we
need to revert the original commit and just stop passing a struct cred
pointer. If there is a concern about the data I think we need a better
explanation of what those LSB test cases were that broke.
Eric
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
Sven Joachim svenj...@gmx.de writes:
On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
3.8-stable review patch. If anyone has any objections, please let me know.
I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the
patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
here, and 65534 is the uid of user nobody.
Hmm.
Ok. I don't understand the commit that was being backported here. I am
pretty certain it a fix for a problem that did not exist.
Unless I am completely mis-reading scm_recv we only generate a
SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
Which means that the only harm that can come from adding scm credentials
to a disconnected af_unix socket is a loss in efficiency.
Not adding scm credentials to be passed to userspace as the commit below
is doing can result is bogus data being passed to userspace. Which is
very actively WRONG.
Now before scm_recv does anything we first call scm_set_cred. If no
credential was passed to scm_set_cred we set the uid to INVALID_UID.
Which scm_recv in the call from_kuid_munged translates into 65534 for
reporting to userspace.
So this is is pretty clearly a case of us not sending the unix
credentials.
Since not sending credential is just a performance optimization I can
see no earthly reason why the commit below should have been applied in
the first place, and no reason why it should have been backported in the
second place. So my vote is that we revert this bogus commit. Upstream
and then backport the revert.
Am I missing something?
Well, yes, this commit fixes a real bug : We were coalescing two
messages into a single one, even if the senders were different.
Copy of a reply I did :
So the problem is that two messages have different credentials,
because other-sk_socket changed between first and second message.
and unix_stream_recvmsg() has the following check :
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb-scm-pid) ||
(UNIXCB(skb).cred != siocb-scm-cred))
break;
} else {
/* Copy credentials */
scm_set_cred(siocb-scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
So the patch was good, and we need a followup, like the one I posted today ?
Some user apps dont know about uid 65534.
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..42359d8 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock, struct
msghdr *msg,
return;
}
- if (test_bit(SOCK_PASSCRED, sock-flags)) {
+ if (test_bit(SOCK_PASSCRED, sock-flags) scm-creds.pid) {
struct user_namespace *current_ns = current_user_ns();
struct ucred ucreds = {
.pid = scm-creds.pid,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, 2013-04-03 at 17:19 -0700, Eric Dumazet wrote: Well, yes, this commit fixes a real bug : We were coalescing two messages into a single one, even if the senders were different. By the way, the 'LSB' test program can be found here : https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Eric Dumazet eric.duma...@gmail.com writes:
On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
Sven Joachim svenj...@gmx.de writes:
On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
3.8-stable review patch. If anyone has any objections, please let me
know.
I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the
patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
here, and 65534 is the uid of user nobody.
Hmm.
Ok. I don't understand the commit that was being backported here. I am
pretty certain it a fix for a problem that did not exist.
Unless I am completely mis-reading scm_recv we only generate a
SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
Which means that the only harm that can come from adding scm credentials
to a disconnected af_unix socket is a loss in efficiency.
Not adding scm credentials to be passed to userspace as the commit below
is doing can result is bogus data being passed to userspace. Which is
very actively WRONG.
Now before scm_recv does anything we first call scm_set_cred. If no
credential was passed to scm_set_cred we set the uid to INVALID_UID.
Which scm_recv in the call from_kuid_munged translates into 65534 for
reporting to userspace.
So this is is pretty clearly a case of us not sending the unix
credentials.
Since not sending credential is just a performance optimization I can
see no earthly reason why the commit below should have been applied in
the first place, and no reason why it should have been backported in the
second place. So my vote is that we revert this bogus commit. Upstream
and then backport the revert.
Am I missing something?
Well, yes, this commit fixes a real bug : We were coalescing two
messages into a single one, even if the senders were different.
What???
As far as I can tell this patch can only server to _allow_ coalescing two
messages into a single one.
Copy of a reply I did :
So the problem is that two messages have different credentials,
because other-sk_socket changed between first and second message.
and unix_stream_recvmsg() has the following check :
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb-scm-pid) ||
(UNIXCB(skb).cred != siocb-scm-cred))
break;
} else {
/* Copy credentials */
scm_set_cred(siocb-scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
So the patch was good, and we need a followup, like the one I posted
today ?
No. The patch is still bogus.
If the problem is that we are not coallescing messages in stream_recvmsg
we need a different fix.
Probably something like:
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb-scm-pid) ||
(UNIXCB(skb).cred != siocb-scm-cred))
break;
- } else {
+ } else if (test_bit(SOCK_PASSCRED, sock-flags)) {
/* Copy credentials */
scm_set_cred(siocb-scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
Although comapring comparing the applicable uids and gids might be
sensible as well.
Some user apps dont know about uid 65534.
What??? The problem is that the app wanted the uid and we gave it
garbage. You can't fix wanting a valid uid by not passing a uid.
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..42359d8 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock,
struct msghdr *msg,
return;
}
- if (test_bit(SOCK_PASSCRED, sock-flags)) {
+ if (test_bit(SOCK_PASSCRED, sock-flags) scm-creds.pid) {
struct user_namespace *current_ns = current_user_ns();
struct ucred ucreds = {
.pid = scm-creds.pid,
Eric
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
On Wed, Apr 3, 2013 at 5:47 PM, Eric W. Biederman ebied...@xmission.com wrote:
Eric Dumazet eric.duma...@gmail.com writes:
On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
Sven Joachim svenj...@gmx.de writes:
On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
3.8-stable review patch. If anyone has any objections, please let me
know.
I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
3.9-rc5: udevd[56]: sender uid=65534, message ignored. Reverting the
patch below on top of 3.8.6-rc1 fixes that. I'm using udev version 175
here, and 65534 is the uid of user nobody.
Hmm.
Ok. I don't understand the commit that was being backported here. I am
pretty certain it a fix for a problem that did not exist.
Unless I am completely mis-reading scm_recv we only generate a
SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
Which means that the only harm that can come from adding scm credentials
to a disconnected af_unix socket is a loss in efficiency.
Not adding scm credentials to be passed to userspace as the commit below
is doing can result is bogus data being passed to userspace. Which is
very actively WRONG.
Now before scm_recv does anything we first call scm_set_cred. If no
credential was passed to scm_set_cred we set the uid to INVALID_UID.
Which scm_recv in the call from_kuid_munged translates into 65534 for
reporting to userspace.
So this is is pretty clearly a case of us not sending the unix
credentials.
Since not sending credential is just a performance optimization I can
see no earthly reason why the commit below should have been applied in
the first place, and no reason why it should have been backported in the
second place. So my vote is that we revert this bogus commit. Upstream
and then backport the revert.
Am I missing something?
Well, yes, this commit fixes a real bug : We were coalescing two
messages into a single one, even if the senders were different.
What???
As far as I can tell this patch can only server to _allow_ coalescing two
messages into a single one.
Copy of a reply I did :
So the problem is that two messages have different credentials,
because other-sk_socket changed between first and second message.
and unix_stream_recvmsg() has the following check :
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb-scm-pid) ||
(UNIXCB(skb).cred != siocb-scm-cred))
break;
} else {
/* Copy credentials */
scm_set_cred(siocb-scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
So the patch was good, and we need a followup, like the one I posted
today ?
No. The patch is still bogus.
If the problem is that we are not coallescing messages in stream_recvmsg
we need a different fix.
Probably something like:
if (check_creds) {
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != siocb-scm-pid) ||
(UNIXCB(skb).cred != siocb-scm-cred))
break;
- } else {
+ } else if (test_bit(SOCK_PASSCRED, sock-flags)) {
/* Copy credentials */
scm_set_cred(siocb-scm, UNIXCB(skb).pid,
UNIXCB(skb).cred);
check_creds = 1;
}
I'm confused. Isn't this making the problem worse, not better?
The original don't-always-pass-creds logic came from (I think):
commit 16e5726269611b71c930054ffe9b858c1cea88eb
Author: Eric Dumazet eric.duma...@gmail.com
Date: Mon Sep 19 05:52:27 2011 +
af_unix: dont send SCM_CREDENTIALS by default
Since commit 7361c36c5224 (af_unix: Allow credentials to work across
user and pid namespaces) af_unix performance dropped a lot.
This is because we now take a reference on pid and cred in each write(),
and release them in read(), usually done from another process,
eventually from another cpu. This triggers false sharing.
With my patches, the cost should go way down and it could be made
unconditional, but that's still probably not a good -stable change.
--Andy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Eric Dumazet eric.duma...@gmail.com writes: On Wed, 2013-04-03 at 17:19 -0700, Eric Dumazet wrote: Well, yes, this commit fixes a real bug : We were coalescing two messages into a single one, even if the senders were different. By the way, the 'LSB' test program can be found here : https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 And we have two sends and one recvfrom, and no loops. So the the problem must be a failure to coalesce messages. There is a race between creating the sending and receiving sockets. At a first glance that race looks like we put the cred on the first message and not on the second message because we are connected by the time the second messages is sent. Which would definitely cause a failure to coallesce messages. Eric -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
