[PATCH 1/2] Revert "af_unix: dont send SCM_CREDENTIAL when dest socket is NULL"

[PATCH 1/2] Revert "af_unix: dont send SCM_CREDENTIAL when dest socket is NULL"

[Eric W. Biederman]

This reverts commit 14134f6584212d585b310ce95428014b653dfaf6.

The problem that the above patch was meant to address is that af_unix
messages are not being coallesced because we are sending unnecesarry
credentials.  Not sending credentials in maybe_add_creds totally
breaks unconnected unix domain sockets that wish to send credentails
to other sockets.

In practice this break some versions of udev because they receive a
message and the sending uid is bogus so they drop the message.

Cc: stable@vger.kernel.org
Reported-by: Sven Joachim <svenjoac@gmx.de>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 net/unix/af_unix.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 971282b..f153a8d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1412,8 +1412,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
 	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();
 	}
-- 
1.7.5.4

[PATCH 2/2] af_unix: If we don't care about credentials coallesce all messages

[Eric W. Biederman]

It was reported that the following LSB test case failed
https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 because we
were not coallescing unix stream messages when the application was
expecting us to.

The problem was that the first send was before the socket was accepted
and thus sock->sk_socket was NULL in maybe_add_creds, and the second
send after the socket was accepted had a non-NULL value for sk->socket
and thus we could tell the credentials were not needed so we did not
bother.

The unnecessary credentials on the first message cause
unix_stream_recvmsg to start verifying that all messages had the same
credentials before coallescing and then the coallescing failed because
the second message had no credentials.

Ignoring credentials when we don't care in unix_stream_recvmsg fixes a
long standing pessimization which would fail to coallesce messages when
reading from a unix stream socket if the senders were different even if
we did not care about their credentials.

I have tested this and verified that the in the LSB test case mentioned
above that the messages do coallesce now, while the were failing to
coallesce without this change.

Reported-by: Karel Srot <ksrot@redhat.com>
Reported-by: Ding Tianhong <dingtianhong@huawei.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 net/unix/af_unix.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f153a8d..2db702d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1993,7 +1993,7 @@ again:
 			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;
-- 
1.7.5.4

[PATCH 3/2] scm: Stop passing struct cred

[Eric W. Biederman]

Now that uids and gids are completely encapsulated in kuid_t
and kgid_t we no longer need to pass struct cred which allowed
us to test both the uid and the user namespace for equality.

Passing struct cred potentially allows us to pass the entire group
list as BSD does but I don't believe the cost of cache line misses
justifies retaining code for a future potential application.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

Included in this patchset because there are trivial dependencies,
and since we are sort of arguing about this anyway.  This definitely is
not for stable.

 include/net/af_unix.h |    3 ++-
 include/net/scm.h     |   16 ++++++----------
 net/core/scm.c        |   16 ----------------
 net/unix/af_unix.c    |   16 ++++++++--------
 4 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 0a996a3..a8836e8 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -29,7 +29,8 @@ struct unix_address {
 
 struct unix_skb_parms {
 	struct pid		*pid;		/* Skb credentials	*/
-	const struct cred	*cred;
+	kuid_t			uid;
+	kgid_t			gid;
 	struct scm_fp_list	*fp;		/* Passed files		*/
 #ifdef CONFIG_SECURITY_NETWORK
 	u32			secid;		/* Security ID		*/
diff --git a/include/net/scm.h b/include/net/scm.h
index 975cca0..5a4c6a9 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -26,7 +26,6 @@ struct scm_fp_list {
 
 struct scm_cookie {
 	struct pid		*pid;		/* Skb credentials */
-	const struct cred	*cred;
 	struct scm_fp_list	*fp;		/* Passed files		*/
 	struct scm_creds	creds;		/* Skb credentials	*/
 #ifdef CONFIG_SECURITY_NETWORK
@@ -51,23 +50,18 @@ static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_co
 #endif /* CONFIG_SECURITY_NETWORK */
 
 static __inline__ void scm_set_cred(struct scm_cookie *scm,
-				    struct pid *pid, const struct cred *cred)
+				    struct pid *pid, kuid_t uid, kgid_t gid)
 {
 	scm->pid  = get_pid(pid);
-	scm->cred = cred ? get_cred(cred) : NULL;
 	scm->creds.pid = pid_vnr(pid);
-	scm->creds.uid = cred ? cred->euid : INVALID_UID;
-	scm->creds.gid = cred ? cred->egid : INVALID_GID;
+	scm->creds.uid = uid;
+	scm->creds.gid = gid;
 }
 
 static __inline__ void scm_destroy_cred(struct scm_cookie *scm)
 {
 	put_pid(scm->pid);
 	scm->pid  = NULL;
-
-	if (scm->cred)
-		put_cred(scm->cred);
-	scm->cred = NULL;
 }
 
 static __inline__ void scm_destroy(struct scm_cookie *scm)
@@ -81,8 +75,10 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
 			       struct scm_cookie *scm, bool forcecreds)
 {
 	memset(scm, 0, sizeof(*scm));
+	scm->creds.uid = INVALID_UID;
+	scm->creds.gid = INVALID_GID;
 	if (forcecreds)
-		scm_set_cred(scm, task_tgid(current), current_cred());
+		scm_set_cred(scm, task_tgid(current), current_euid(), current_egid());
 	unix_get_peersec_dgram(sock, scm);
 	if (msg->msg_controllen <= 0)
 		return 0;
diff --git a/net/core/scm.c b/net/core/scm.c
index 2dc6cda..83b2b38 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -187,22 +187,6 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p)
 
 			p->creds.uid = uid;
 			p->creds.gid = gid;
-
-			if (!p->cred ||
-			    !uid_eq(p->cred->euid, uid) ||
-			    !gid_eq(p->cred->egid, gid)) {
-				struct cred *cred;
-				err = -ENOMEM;
-				cred = prepare_creds();
-				if (!cred)
-					goto error;
-
-				cred->uid = cred->euid = uid;
-				cred->gid = cred->egid = gid;
-				if (p->cred)
-					put_cred(p->cred);
-				p->cred = cred;
-			}
 			break;
 		}
 		default:
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 2db702d..f5594b5 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1340,7 +1340,6 @@ static void unix_destruct_scm(struct sk_buff *skb)
 	struct scm_cookie scm;
 	memset(&scm, 0, sizeof(scm));
 	scm.pid  = UNIXCB(skb).pid;
-	scm.cred = UNIXCB(skb).cred;
 	if (UNIXCB(skb).fp)
 		unix_detach_fds(&scm, skb);
 
@@ -1391,8 +1390,8 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
 	int err = 0;
 
 	UNIXCB(skb).pid  = get_pid(scm->pid);
-	if (scm->cred)
-		UNIXCB(skb).cred = get_cred(scm->cred);
+	UNIXCB(skb).uid = scm->creds.uid;
+	UNIXCB(skb).gid = scm->creds.gid;
 	UNIXCB(skb).fp = NULL;
 	if (scm->fp && send_fds)
 		err = unix_attach_fds(scm, skb);
@@ -1409,13 +1408,13 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
 static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
 			    const struct sock *other)
 {
-	if (UNIXCB(skb).cred)
+	if (UNIXCB(skb).pid)
 		return;
 	if (test_bit(SOCK_PASSCRED, &sock->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();
+		current_euid_egid(&UNIXCB(skb).uid, &UNIXCB(skb).gid);
 	}
 }
 
@@ -1819,7 +1818,7 @@ static int unix_dgram_recvmsg(struct kiocb *iocb, struct socket *sock,
 		siocb->scm = &tmp_scm;
 		memset(&tmp_scm, 0, sizeof(tmp_scm));
 	}
-	scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
+	scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid);
 	unix_set_secdata(siocb->scm, skb);
 
 	if (!(flags & MSG_PEEK)) {
@@ -1991,11 +1990,12 @@ again:
 		if (check_creds) {
 			/* Never glue messages from different writers */
 			if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
-			    (UNIXCB(skb).cred != siocb->scm->cred))
+			    !uid_eq(UNIXCB(skb).uid, siocb->scm->creds.uid) ||
+			    !gid_eq(UNIXCB(skb).gid, siocb->scm->creds.gid))
 				break;
 		} else if (test_bit(SOCK_PASSCRED, &sock->flags)) {
 			/* Copy credentials */
-			scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
+			scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).uid, UNIXCB(skb).gid);
 			check_creds = 1;
 		}
 
-- 
1.7.5.4

Re: [PATCH 1/2] Revert "af_unix: dont send SCM_CREDENTIAL when dest socket is NULL"

[dingtianhong]

On 2013/4/4 10:13, Eric W. Biederman wrote:
> 
> This reverts commit 14134f6584212d585b310ce95428014b653dfaf6.
> 
> The problem that the above patch was meant to address is that af_unix
> messages are not being coallesced because we are sending unnecesarry
> credentials.  Not sending credentials in maybe_add_creds totally
> breaks unconnected unix domain sockets that wish to send credentails
> to other sockets.
> 

thanks for check the question and make a fix solution, but I still doubt that if unconnected unix
domain socket wish to send credentails to oher sockets, why dont set
SOCK_PASSCRED on sock->flags, I think the user need to decide the param
and shouldnt send creds by default way.

Ding

> In practice this break some versions of udev because they receive a
> message and the sending uid is bogus so they drop the message.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Sven Joachim <svenjoac@gmx.de>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  net/unix/af_unix.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 971282b..f153a8d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1412,8 +1412,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
>  	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();
>  	}
> 

Re: [PATCH 2/2] af_unix: If we don't care about credentials coallesce all messages

[dingtianhong]

On 2013/4/4 10:14, Eric W. Biederman wrote:
> 
> It was reported that the following LSB test case failed
> https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 because we
> were not coallescing unix stream messages when the application was
> expecting us to.
> 
> The problem was that the first send was before the socket was accepted
> and thus sock->sk_socket was NULL in maybe_add_creds, and the second
> send after the socket was accepted had a non-NULL value for sk->socket
> and thus we could tell the credentials were not needed so we did not
> bother.
> 
> The unnecessary credentials on the first message cause
> unix_stream_recvmsg to start verifying that all messages had the same
> credentials before coallescing and then the coallescing failed because
> the second message had no credentials.
> 
> Ignoring credentials when we don't care in unix_stream_recvmsg fixes a
> long standing pessimization which would fail to coallesce messages when
> reading from a unix stream socket if the senders were different even if
> we did not care about their credentials.
> 
> I have tested this and verified that the in the LSB test case mentioned
> above that the messages do coallesce now, while the were failing to
> coallesce without this change.
> 
> Reported-by: Karel Srot <ksrot@redhat.com>
> Reported-by: Ding Tianhong <dingtianhong@huawei.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  net/unix/af_unix.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index f153a8d..2db702d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1993,7 +1993,7 @@ again:
>  			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;
> 

As your opinion, I think the way is better:

		if (test_bit(SOCK_PASSCRED, &sock->flags)) {
                        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;
                        }
                }

Ding

Re: [PATCH 1/2] Revert "af_unix: dont send SCM_CREDENTIAL when dest socket is NULL"

[Eric W. Biederman]

dingtianhong <dingtianhong@huawei.com> writes:

> On 2013/4/4 10:13, Eric W. Biederman wrote:
>> 
>> This reverts commit 14134f6584212d585b310ce95428014b653dfaf6.
>> 
>> The problem that the above patch was meant to address is that af_unix
>> messages are not being coallesced because we are sending unnecesarry
>> credentials.  Not sending credentials in maybe_add_creds totally
>> breaks unconnected unix domain sockets that wish to send credentails
>> to other sockets.
>> 
>
> thanks for check the question and make a fix solution, but I still doubt that if unconnected unix
> domain socket wish to send credentails to oher sockets, why dont set
> SOCK_PASSCRED on sock->flags, I think the user need to decide the param
> and shouldnt send creds by default way.

The big issue is the semantics are the receiver sets SOCK_PASSCRED when
they want to receive credentials.  When transmitting packets from
unconnected or unaccepted sockets we don't know if the receiver has set
SOCK_PASSCRED so when in doubt transmit. Historically we always
tranmitted credentials.

Furthermore we have a real regression in udev that breaks systems, so
this patch must be reverted.

Eric

Re: [PATCH 2/2] af_unix: If we don't care about credentials coallesce all messages

[Eric W. Biederman]

dingtianhong <dingtianhong@huawei.com> writes:

> On 2013/4/4 10:14, Eric W. Biederman wrote:
>> 
>> It was reported that the following LSB test case failed
>> https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 because we
>> were not coallescing unix stream messages when the application was
>> expecting us to.
>> 
>> The problem was that the first send was before the socket was accepted
>> and thus sock->sk_socket was NULL in maybe_add_creds, and the second
>> send after the socket was accepted had a non-NULL value for sk->socket
>> and thus we could tell the credentials were not needed so we did not
>> bother.
>> 
>> The unnecessary credentials on the first message cause
>> unix_stream_recvmsg to start verifying that all messages had the same
>> credentials before coallescing and then the coallescing failed because
>> the second message had no credentials.
>> 
>> Ignoring credentials when we don't care in unix_stream_recvmsg fixes a
>> long standing pessimization which would fail to coallesce messages when
>> reading from a unix stream socket if the senders were different even if
>> we did not care about their credentials.
>> 
>> I have tested this and verified that the in the LSB test case mentioned
>> above that the messages do coallesce now, while the were failing to
>> coallesce without this change.
>> 
>> Reported-by: Karel Srot <ksrot@redhat.com>
>> Reported-by: Ding Tianhong <dingtianhong@huawei.com>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  net/unix/af_unix.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index f153a8d..2db702d 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1993,7 +1993,7 @@ again:
>>  			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;
>> 
>
> As your opinion, I think the way is better:
>
> 		if (test_bit(SOCK_PASSCRED, &sock->flags)) {
>                         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;
>                         }
>                 }

It is a smidge clearer in intent, but there is no functional
difference.  The lines get really long.

Shrug.

Patches are always welcome.

Beyond getting something correct for the right reasons I don't care.

Eric

Re: [PATCH 1/2] Revert "af_unix: dont send SCM_CREDENTIAL when dest socket is NULL"

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 03 Apr 2013 19:13:35 -0700

> 
> This reverts commit 14134f6584212d585b310ce95428014b653dfaf6.
> 
> The problem that the above patch was meant to address is that af_unix
> messages are not being coallesced because we are sending unnecesarry
> credentials.  Not sending credentials in maybe_add_creds totally
> breaks unconnected unix domain sockets that wish to send credentails
> to other sockets.
> 
> In practice this break some versions of udev because they receive a
> message and the sending uid is bogus so they drop the message.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Sven Joachim <svenjoac@gmx.de>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Applied.

Re: [PATCH 2/2] af_unix: If we don't care about credentials coallesce all messages

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 03 Apr 2013 19:14:47 -0700

> 
> It was reported that the following LSB test case failed
> https://lsbbugs.linuxfoundation.org/attachment.cgi?id=2144 because we
> were not coallescing unix stream messages when the application was
> expecting us to.
> 
> The problem was that the first send was before the socket was accepted
> and thus sock->sk_socket was NULL in maybe_add_creds, and the second
> send after the socket was accepted had a non-NULL value for sk->socket
> and thus we could tell the credentials were not needed so we did not
> bother.
> 
> The unnecessary credentials on the first message cause
> unix_stream_recvmsg to start verifying that all messages had the same
> credentials before coallescing and then the coallescing failed because
> the second message had no credentials.
> 
> Ignoring credentials when we don't care in unix_stream_recvmsg fixes a
> long standing pessimization which would fail to coallesce messages when
> reading from a unix stream socket if the senders were different even if
> we did not care about their credentials.
> 
> I have tested this and verified that the in the LSB test case mentioned
> above that the messages do coallesce now, while the were failing to
> coallesce without this change.
> 
> Reported-by: Karel Srot <ksrot@redhat.com>
> Reported-by: Ding Tianhong <dingtianhong@huawei.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Applied.

Re: [PATCH 3/2] scm: Stop passing struct cred

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 03 Apr 2013 20:28:16 -0700

> 
> Now that uids and gids are completely encapsulated in kuid_t
> and kgid_t we no longer need to pass struct cred which allowed
> us to test both the uid and the user namespace for equality.
> 
> Passing struct cred potentially allows us to pass the entire group
> list as BSD does but I don't believe the cost of cache line misses
> justifies retaining code for a future potential application.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

I'll apply this to net-next once #1 and #2 propagate there.

Thanks!