On Wed, Apr 3, 2013 at 5:47 PM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
> Eric Dumazet <eric.dumazet@xxxxxxxxx> writes:
>
>> On Wed, 2013-04-03 at 17:05 -0700, Eric W. Biederman wrote:
>>> Sven Joachim <svenjoac@xxxxxx> 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.dumazet@xxxxxxxxx>
Date: Mon Sep 19 05:52:27 2011 +0000
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 stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html