[V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Joseph Salisbury]

Hi Rainer,

A kernel bug report was opened against Ubuntu [0].  After a kernel
bisect, it was found that reverting the following commit resolved this bug:

commit 3822b5c2fc62e3de8a0f33806ff279fb7df92432
Author: Rainer Weikusat <rweikusat@mobileactivedefense.com>
Date:   Wed Dec 16 20:09:25 2015 +0000

    af_unix: Revert 'lock_interruptible' in stream receive code

      
The regression was introduced as of v4.4-rc6.

I was hoping to get your feedback, since you are the patch author.  Do
you think gathering any additional data will help diagnose this issue,
or would it be best to submit a revert request?

    
Thanks,
    
Joe

[0] http://pad.lv/1540731

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Rainer Weikusat]

Joseph Salisbury <joseph.salisbury@canonical.com> writes:
> Hi Rainer,
>
> A kernel bug report was opened against Ubuntu [0].  After a kernel
> bisect, it was found that reverting the following commit resolved this bug:
>
> commit 3822b5c2fc62e3de8a0f33806ff279fb7df92432
> Author: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> Date:   Wed Dec 16 20:09:25 2015 +0000
>
>     af_unix: Revert 'lock_interruptible' in stream receive code
>
>       
> The regression was introduced as of v4.4-rc6.
>
> I was hoping to get your feedback, since you are the patch author.  Do
> you think gathering any additional data will help diagnose this issue,
> or would it be best to submit a revert request?

Funny little problem :-). The code using the interruptible lock cleared
err as side effect hence the

out:
	return copied ? : err;

at the end of unix_stream_read_generic didn't return the -ENOTSUP put
into err at the start of the function if copied was zero after the loop
because the size of the passed data buffer was zero.

The following patch should fix this:

---------
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..c3e1a08 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2300,6 +2300,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
        else
                skip = 0;
 
+       err = 0;
        do {
                int chunk;
                bool drop_skb;
----------

I was just about to go the the supermarket to buy an apple when I
received the mail. I didn't even compile the change above yet, however,
I'll do so once I'm back and then submit something formal.

Here's a test program which can be compiled with a C compiler:
------------
#define _GNU_SOURCE
    
#include <stdlib.h>
#include <stdio.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <assert.h>
#include <errno.h>
#include <string.h>

int main(void)
{
    enum { server, client, size };
    int socket_fd[size];
    int const opt = 1;

    assert(socketpair(AF_LOCAL, SOCK_STREAM, 0, socket_fd) == 0);

    char const msg[] = "A random message";
    send(socket_fd[client], msg, sizeof msg, MSG_DONTWAIT | MSG_NOSIGNAL);

    assert(setsockopt(socket_fd[server], SOL_SOCKET, SO_PASSCRED, &opt, sizeof(opt)) != -1);

    union {
        struct cmsghdr cmh;
        char control[CMSG_SPACE(sizeof(struct ucred))];
    } control_un;

    control_un.cmh.cmsg_len = CMSG_LEN(sizeof(struct ucred));
    control_un.cmh.cmsg_level = SOL_SOCKET;
    control_un.cmh.cmsg_type = SCM_CREDENTIALS;

    struct msghdr msgh;
    msgh.msg_name = NULL;
    msgh.msg_namelen = 0;
    msgh.msg_iov = NULL;
    msgh.msg_iovlen = 0;
    msgh.msg_control = control_un.control;
    msgh.msg_controllen = sizeof(control_un.control);

    errno = 0;

    if (recvmsg(socket_fd[server], &msgh, MSG_PEEK) == -1)
    {
        printf("Error: %s\n", strerror(errno));
        exit(EXIT_FAILURE);
    }
    else
    {
        printf("Success!\n");
        exit(EXIT_SUCCESS);
    }
}

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Joseph Salisbury]

On 02/05/2016 02:59 PM, Rainer Weikusat wrote:
> Joseph Salisbury <joseph.salisbury@canonical.com> writes:
>> Hi Rainer,
>>
>> A kernel bug report was opened against Ubuntu [0].  After a kernel
>> bisect, it was found that reverting the following commit resolved this bug:
>>
>> commit 3822b5c2fc62e3de8a0f33806ff279fb7df92432
>> Author: Rainer Weikusat <rweikusat@mobileactivedefense.com>
>> Date:   Wed Dec 16 20:09:25 2015 +0000
>>
>>     af_unix: Revert 'lock_interruptible' in stream receive code
>>
>>       
>> The regression was introduced as of v4.4-rc6.
>>
>> I was hoping to get your feedback, since you are the patch author.  Do
>> you think gathering any additional data will help diagnose this issue,
>> or would it be best to submit a revert request?
> Funny little problem :-). The code using the interruptible lock cleared
> err as side effect hence the
>
> out:
> 	return copied ? : err;
>
> at the end of unix_stream_read_generic didn't return the -ENOTSUP put
> into err at the start of the function if copied was zero after the loop
> because the size of the passed data buffer was zero.
>
> The following patch should fix this:
>
> ---------
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 49d5093..c3e1a08 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2300,6 +2300,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
>         else
>                 skip = 0;
>  
> +       err = 0;
>         do {
>                 int chunk;
>                 bool drop_skb;
> ----------
>
> I was just about to go the the supermarket to buy an apple when I
> received the mail. I didn't even compile the change above yet, however,
> I'll do so once I'm back and then submit something formal.
>
> Here's a test program which can be compiled with a C compiler:
> ------------
> #define _GNU_SOURCE
>     
> #include <stdlib.h>
> #include <stdio.h>
> #include <sys/socket.h>
> #include <sys/stat.h>
> #include <assert.h>
> #include <errno.h>
> #include <string.h>
>
> int main(void)
> {
>     enum { server, client, size };
>     int socket_fd[size];
>     int const opt = 1;
>
>     assert(socketpair(AF_LOCAL, SOCK_STREAM, 0, socket_fd) == 0);
>
>     char const msg[] = "A random message";
>     send(socket_fd[client], msg, sizeof msg, MSG_DONTWAIT | MSG_NOSIGNAL);
>
>     assert(setsockopt(socket_fd[server], SOL_SOCKET, SO_PASSCRED, &opt, sizeof(opt)) != -1);
>
>     union {
>         struct cmsghdr cmh;
>         char control[CMSG_SPACE(sizeof(struct ucred))];
>     } control_un;
>
>     control_un.cmh.cmsg_len = CMSG_LEN(sizeof(struct ucred));
>     control_un.cmh.cmsg_level = SOL_SOCKET;
>     control_un.cmh.cmsg_type = SCM_CREDENTIALS;
>
>     struct msghdr msgh;
>     msgh.msg_name = NULL;
>     msgh.msg_namelen = 0;
>     msgh.msg_iov = NULL;
>     msgh.msg_iovlen = 0;
>     msgh.msg_control = control_un.control;
>     msgh.msg_controllen = sizeof(control_un.control);
>
>     errno = 0;
>
>     if (recvmsg(socket_fd[server], &msgh, MSG_PEEK) == -1)
>     {
>         printf("Error: %s\n", strerror(errno));
>         exit(EXIT_FAILURE);
>     }
>     else
>     {
>         printf("Success!\n");
>         exit(EXIT_SUCCESS);
>     }
> }
Thanks for the feedback.  Just curious, was it a green apple or a red
apple? :-)

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Rainer Weikusat]

Joseph Salisbury <joseph.salisbury@canonical.com> writes:
> On 02/05/2016 02:59 PM, Rainer Weikusat wrote:

[recvmsg w/o iovecs returning ENOTSUP for CMSG requests]

>> Funny little problem :-). The code using the interruptible lock cleared
>> err as side effect hence the
>>
>> out:
>> 	return copied ? : err;
>>
>> at the end of unix_stream_read_generic didn't return the -ENOTSUP put
>> into err at the start of the function if copied was zero after the loop
>> because the size of the passed data buffer was zero.

There are more problems wrt handling control-message only reads in this
code. In particular, changing the test program as follows:

    if (fork() == 0) {
	sleep(1);
	send(socket_fd[client], msg, sizeof msg, MSG_DONTWAIT | MSG_NOSIGNAL);

	_exit(0);
    }

makes the recvmsg fail with EAGAIN and judging from the code (I didn't
test this yet), it will return without an error but also without
credentials if the

err = -EAGAIN
if (!timeo)
	break;

is changed to

if (!timeo) {
	err = -EAGAIN;
        break
}

because the following

mutex_lock(&u->readlock);
continue;

will cause the

do {
} while (size)

loop condition to be evaluated and since size is 0 (AIUI), the loop will
terminate immediately.

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Rainer Weikusat]

The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	goto out;

This has the unfortunate side effect of possibly causing the error code
to bleed through to the final

out:
	return copied ? : err;

and then to be wrongly returned if no data was copied because the caller
didn't supply a data buffer, as demonstrated by the program available at

http://pad.lv/1540731

Change it such that err is only set if an error condition was detected.

Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..138787d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (sk->sk_state != TCP_ESTABLISHED) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (flags & MSG_OOB) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Rainer Weikusat]

Rainer Weikusat <rw@doppelsaurus.mobileactivedefense.com> writes:
> Joseph Salisbury <joseph.salisbury@canonical.com> writes:
>> On 02/05/2016 02:59 PM, Rainer Weikusat wrote:
>
> [recvmsg w/o iovecs returning ENOTSUP for CMSG requests]

[...]

> There are more problems wrt handling control-message only reads in this
> code.

[...]

> it will return without an error but also without credentials if the

[...]

> because the following
>
> mutex_lock(&u->readlock);
> continue;
>
> will cause the
>
> do {
> } while (size)
>
> loop condition to be evaluated and since size is 0 (AIUI), the loop will
> terminate immediately.

As I suspected, the test program included below doesn't really receive
the credentials (tested with a 4.5.0-rc2-net w/ the previous patch
applied). As that's a minor, additional problem, I'll fix that, too.

---
#define _GNU_SOURCE
    
#include <stdlib.h>
#include <stdio.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <assert.h>
#include <errno.h>
#include <string.h>
#include <unistd.h>

int main(void)
{
    enum { server, client, size };
    int socket_fd[size];
    int const opt = 1;

    assert(socketpair(AF_LOCAL, SOCK_STREAM, 0, socket_fd) == 0);
    assert(setsockopt(socket_fd[server], SOL_SOCKET, SO_PASSCRED, &opt, sizeof(opt)) != -1);

    char const msg[] = "A random message";

    if (fork() == 0) {
	sleep(1);
	send(socket_fd[client], msg, sizeof msg, MSG_DONTWAIT | MSG_NOSIGNAL);

	_exit(0);
    }

    union {
        struct cmsghdr cmh;
        char control[CMSG_SPACE(sizeof(struct ucred))];
    } control_un;

    control_un.cmh.cmsg_len = CMSG_LEN(sizeof(struct ucred));
    control_un.cmh.cmsg_level = SOL_SOCKET;
    control_un.cmh.cmsg_type = SCM_CREDENTIALS;

    struct msghdr msgh;
    msgh.msg_name = NULL;
    msgh.msg_namelen = 0;
    msgh.msg_iov = NULL;
    msgh.msg_iovlen = 0;
    msgh.msg_control = control_un.control;
    msgh.msg_controllen = sizeof(control_un.control);

    if (recvmsg(socket_fd[server], &msgh, MSG_PEEK) == -1)
    {
        printf("Error: %s\n", strerror(errno));
        exit(EXIT_FAILURE);
    }
    else
    {
	struct ucred *ucred;
	
        printf("Success?\n");

	ucred = (void *)CMSG_DATA(&control_un.cmh);
	printf("...  pid %ld, uid %d, gid %d\n",
	       (long)ucred->pid, ucred->uid, ucred->gid);
    }

    return 0;
}

[PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Rainer Weikusat]

The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	goto out;

This has the unfortunate side effect of possibly causing the error code
to bleed through to the final

out:
	return copied ? : err;

and then to be wrongly returned if no data was copied because the caller
didn't supply a data buffer, as demonstrated by the program available at

http://pad.lv/1540731

Change it such that err is only set if an error condition was detected.

Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---

With proper subject this time (at least I hope so).

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..138787d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (sk->sk_state != TCP_ESTABLISHED) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (flags & MSG_OOB) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Eric Dumazet]

On Fri, 2016-02-05 at 21:44 +0000, Rainer Weikusat wrote:
> The present unix_stream_read_generic contains various code sequences of
> the form
> 
> err = -EDISASTER;
> if (<test>)
> 	goto out;
> 
> This has the unfortunate side effect of possibly causing the error code
> to bleed through to the final
> 
> out:
> 	return copied ? : err;
> 
> and then to be wrongly returned if no data was copied because the caller
> didn't supply a data buffer, as demonstrated by the program available at
> 
> http://pad.lv/1540731
> 
> Change it such that err is only set if an error condition was detected.


Well, if you replace the traditional flow

err = -XXXX;
if (test)
     goto out;

Then please add unlikely() to at least give a hint to the compiler.

if (unlikely(test)) {
    err = -XXX;
    goto out;
}

And please add a 'Fixes: .... ' tag for bug fixes.

Thanks.

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Rainer Weikusat]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Fri, 2016-02-05 at 21:44 +0000, Rainer Weikusat wrote:
>> The present unix_stream_read_generic contains various code sequences of
>> the form
>> 
>> err = -EDISASTER;
>> if (<test>)
>> 	goto out;
>> 
>> This has the unfortunate side effect of possibly causing the error code
>> to bleed through to the final
>> 
>> out:
>> 	return copied ? : err;
>> 
>> and then to be wrongly returned if no data was copied because the caller
>> didn't supply a data buffer, as demonstrated by the program available at
>> 
>> http://pad.lv/1540731
>> 
>> Change it such that err is only set if an error condition was detected.
>
>
> Well, if you replace the traditional flow
>
> err = -XXXX;
> if (test)
>      goto out;
>
> Then please add unlikely() to at least give a hint to the compiler.

There are really four of these, the two leading ones,

if (unlikely(sk->sk_state != TCP_ESTABLISHED)) {
	err = -EINVAL;
	goto out;
}

if (unlikely(flags & MSG_OOB)) {
	err = -EOPNOTSUPP;
	goto out;
}

one in between which was already in the function,

unix_state_lock(sk);
if (sock_flag(sk, SOCK_DEAD)) {
	err = -ECONNRESET;
	goto unlock;
}
[at the beginning of the loop]

and lastly,

unix_state_unlock(sk);
if (!timeo) {
	err = -EAGAIN;
	break;
}

mutex_unlock(&u->readlock);

timeo = unix_stream_data_wait(sk, timeo, last,
			      last_len);

As can be seen here, I've added unlikely() to the first two, left the
middle-one alone and didn't change the last one: That's not really an
error but a non-blocking read. My gut feeling about that would be that's
it's rather likely than unlikely but since I have no real information on
this, I don't know how to annotate it correctly (I'll gladly add
whatever annotation somebody else considers sensible).

> And please add a 'Fixes: .... ' tag for bug fixes.

Similarly, if you think this should be considered as 'fixing' anything
in particular, I'll also gladly add that. In my opinion, the real
problem is that the function disagrees with itself on how to use the err
variable: The start uses that to record an error which might need to be
reported, the return statement uses it to indicate that an error has
occurred. Hence, some kind of in-between translation must occur.
The mutex_lock_interruptible happened to do that but that was never it's
intended purpose.

NB: This is not an attempt to start an argument about that, it just
summarizes my understanding of the situation and I don't insist on this
viewpoint.

I'll send an updated patch with the changes so far, ie, the two
unlikelys.

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Rainer Weikusat]

The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	goto out;

This has the unfortunate side effect of possibly causing the error code
to bleed through to the final

out:
	return copied ? : err;

and then to be wrongly returned if no data was copied because the caller
didn't supply a data buffer, as demonstrated by the program available at

http://pad.lv/1540731

Change it such that err is only set if an error condition was detected.

Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---

With unlikely() added to the two leading error checks. I've also checked
that this actually generates better code (for me at least). Without an
annotation, the non-blocking read case is treated like the others, ie, a
jump to some code at the end of the function loading the error code into
a register, followed by a 2nd jump to the termination sequence.

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..c1e4dd7 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (unlikely(sk->sk_state != TCP_ESTABLISHED)) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (unlikely(flags & MSG_OOB)) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Rainer Weikusat]

Rainer Weikusat <rw@doppelsaurus.mobileactivedefense.com> writes:

[...]

> the real problem is that the function disagrees with itself on how to
> use the err variable: The start uses that to record an error which
> might need to be reported, the return statement uses it to indicate
> that an error has occurred.

This should have been "as indication that an errors has occured" (if no
data was copied).

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Rainer Weikusat]

Rainer Weikusat <rw@doppelsaurus.mobileactivedefense.com> writes:

[...]

> The start uses that to record an error which might need to be
> reported, the return statement uses it to indicate that an error has
> occurred. Hence, some kind of in-between translation must occur.  The
> mutex_lock_interruptible happened to do that but that was never it's
> intended purpose.

Additional information: The 'trick' of using recvmsg w/o a receive
buffer in order to retrieve control messages in fact wouldn't have
worked with the unix_stream_recvmsg prior to introduction of the
interruptible lock as that (judging from the git source) would have
triggered all the same issues,

	- -EOPNOTSUP if a msg was available

        - -EAGAIN if the code had to wait

        - not receiving the creds if the -EAGAIN hadn't happened because
          of the continue (that's the other patch)

IOW, that's a feature inadvertendly added by an otherwise useless code
change (mea culpa).

Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code

[Eric Dumazet]

On Sun, 2016-02-07 at 22:24 +0000, Rainer Weikusat wrote:
> Rainer Weikusat <rw@doppelsaurus.mobileactivedefense.com> writes:
> 
> [...]
> 
> > The start uses that to record an error which might need to be
> > reported, the return statement uses it to indicate that an error has
> > occurred. Hence, some kind of in-between translation must occur.  The
> > mutex_lock_interruptible happened to do that but that was never it's
> > intended purpose.
> 
> Additional information: The 'trick' of using recvmsg w/o a receive
> buffer in order to retrieve control messages in fact wouldn't have
> worked with the unix_stream_recvmsg prior to introduction of the
> interruptible lock as that (judging from the git source) would have
> triggered all the same issues,
> 
> 	- -EOPNOTSUP if a msg was available
> 
>         - -EAGAIN if the code had to wait
> 
>         - not receiving the creds if the -EAGAIN hadn't happened because
>           of the continue (that's the other patch)
> 
> IOW, that's a feature inadvertendly added by an otherwise useless code
> change (mea culpa).

This is exactly the needed information for stable teams.

Goal is here is not to blame someone (you, me ... it does not matter) ,
but give to stable teams the point the problem showed up.

See the 'Fixes' tag as a time saver for people like me.

It is incredibly useful when hutting bugs, because each commit can
easily point to the 'bug origin'.

Having spent time lately in af_unix code insanity, I really can tell.

At the time someone fixes a bug, he/she has a clear view of what is
happening, but months later, he/she often has to start again the commits
analysis.

Thanks a lot.

[PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Rainer Weikusat]

The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	goto out;

This has the unfortunate side effect of possibly causing the error code
to bleed through to the final

out:
	return copied ? : err;

and then to be wrongly returned if no data was copied because the caller
didn't supply a data buffer, as demonstrated by the program available at

http://pad.lv/1540731

Change it such that err is only set if an error condition was detected.

Fixes: 3822b5c2fc62
Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..c1e4dd7 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (unlikely(sk->sk_state != TCP_ESTABLISHED)) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (unlikely(flags & MSG_OOB)) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Rainer Weikusat]

The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	goto out;

This has the unfortunate side effect of possibly causing the error code
to bleed through to the final

out:
	return copied ? : err;

and then to be wrongly returned if no data was copied because the caller
didn't supply a data buffer, as demonstrated by the program available at

http://pad.lv/1540731

Change it such that err is only set if an error condition was detected.

Fixes: 3822b5c2fc62 ("af_unix: Revert 'lock_interruptible' in stream receive code")
Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---

With Fixes: fixed.

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..c1e4dd7 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (unlikely(sk->sk_state != TCP_ESTABLISHED)) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (unlikely(flags & MSG_OOB)) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Sergei Shtylyov]

Hello.

On 02/08/2016 06:33 PM, Rainer Weikusat wrote:

> The present unix_stream_read_generic contains various code sequences of
> the form
>
> err = -EDISASTER;
> if (<test>)
> 	goto out;
>
> This has the unfortunate side effect of possibly causing the error code
> to bleed through to the final
>
> out:
> 	return copied ? : err;
>
> and then to be wrongly returned if no data was copied because the caller
> didn't supply a data buffer, as demonstrated by the program available at
>
> http://pad.lv/1540731
>
> Change it such that err is only set if an error condition was detected.
>
> Fixes: 3822b5c2fc62

    You also need to specify the patch summary like this: ("<summary").

> Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
[...]

MBR, Sergei

[PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Rainer Weikusat]

The present unix_stream_read_generic contains various code sequences of
the form

err = -EDISASTER;
if (<test>)
	goto out;

This has the unfortunate side effect of possibly causing the error code
to bleed through to the final

out:
	return copied ? : err;

and then to be wrongly returned if no data was copied because the caller
didn't supply a data buffer, as demonstrated by the program available at

http://pad.lv/1540731

Change it such that err is only set if an error condition was detected.

Fixes: 3822b5c2fc62 ("af_unix: Revert 'lock_interruptible' in stream receive code")
Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---

And the subject again fixed and, since another correction was necessary,
anyway, a Reported-by added. 

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 49d5093..c1e4dd7 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
 	size_t size = state->size;
 	unsigned int last_len;
 
-	err = -EINVAL;
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (unlikely(sk->sk_state != TCP_ESTABLISHED)) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EOPNOTSUPP;
-	if (flags & MSG_OOB)
+	if (unlikely(flags & MSG_OOB)) {
+		err = -EOPNOTSUPP;
 		goto out;
+	}
 
 	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
 	timeo = sock_rcvtimeo(sk, noblock);
@@ -2329,9 +2331,11 @@ again:
 				goto unlock;
 
 			unix_state_unlock(sk);
-			err = -EAGAIN;
-			if (!timeo)
+			if (!timeo) {
+				err = -EAGAIN;
 				break;
+			}
+
 			mutex_unlock(&u->readlock);
 
 			timeo = unix_stream_data_wait(sk, timeo, last,

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Joseph Salisbury]

On 02/05/2016 05:30 PM, Rainer Weikusat wrote:
> The present unix_stream_read_generic contains various code sequences of
> the form
>
> err = -EDISASTER;
> if (<test>)
> 	goto out;
>
> This has the unfortunate side effect of possibly causing the error code
> to bleed through to the final
>
> out:
> 	return copied ? : err;
>
> and then to be wrongly returned if no data was copied because the caller
> didn't supply a data buffer, as demonstrated by the program available at
>
> http://pad.lv/1540731
>
> Change it such that err is only set if an error condition was detected.
>
> Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> ---
>
> With proper subject this time (at least I hope so).
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 49d5093..138787d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2277,13 +2277,15 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state)
>  	size_t size = state->size;
>  	unsigned int last_len;
>  
> -	err = -EINVAL;
> -	if (sk->sk_state != TCP_ESTABLISHED)
> +	if (sk->sk_state != TCP_ESTABLISHED) {
> +		err = -EINVAL;
>  		goto out;
> +	}
>  
> -	err = -EOPNOTSUPP;
> -	if (flags & MSG_OOB)
> +	if (flags & MSG_OOB) {
> +		err = -EOPNOTSUPP;
>  		goto out;
> +	}
>  
>  	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
>  	timeo = sock_rcvtimeo(sk, noblock);
> @@ -2329,9 +2331,11 @@ again:
>  				goto unlock;
>  
>  			unix_state_unlock(sk);
> -			err = -EAGAIN;
> -			if (!timeo)
> +			if (!timeo) {
> +				err = -EAGAIN;
>  				break;
> +			}
> +
>  			mutex_unlock(&u->readlock);
>  
>  			timeo = unix_stream_data_wait(sk, timeo, last,
I tested your patch, Rainer.  I can confirm that it fixes the reported
bug[0].

Thanks for the quick response!

Joe

[0] http://pad.lv/1540731

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Rainer Weikusat]

Joseph Salisbury <joseph.salisbury@canonical.com> writes:
> On 02/05/2016 05:30 PM, Rainer Weikusat wrote:
>> The present unix_stream_read_generic contains various code sequences of
>> the form
>>
>> err = -EDISASTER;
>> if (<test>)
>> 	goto out;

[...]

>> Change it such that err is only set if an error condition was detected.

[...]

> I tested your patch, Rainer.  I can confirm that it fixes the reported
> bug[0].

This is only a partial fix: The launchpad test case will no longer fail
with EOPNOTSUPP and it will actually receive the credentials because the
message they're attached to was available by the time of the
recvmsg. But if this isn't the case, ie, if the receiver has to wait for
a message, the continue in the do { } while (size) loop will cause the
loop to be terminated without copying the credential information as the
continue will cause the size (of the remaining 'receive area' which is
zero for this case) to be checked before any of the other loop code is
executed again.

I posted a test case for this and a patch addressing that elsewhere in
this thread.

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Fri, 2016-02-05 at 22:30 +0000, Rainer Weikusat wrote:
> The present unix_stream_read_generic contains various code sequences of
> the form
> 
> err = -EDISASTER;
> if ()
> 	goto out;
[...]

I wish people would stop writing code like this.  At one time it may
have been a useful micro-optimisation, avoiding an extra branch in the
successful case, but gcc now appears to do that itself.  So it makes
the code less clear and runs the risk of introducing this sort of bug,
for no obvious benefit.

Ben.

-- 
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[David Miller]

From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
Date: Mon, 08 Feb 2016 18:47:19 +0000

> The present unix_stream_read_generic contains various code sequences of
> the form
> 
> err = -EDISASTER;
> if (<test>)
> 	goto out;
> 
> This has the unfortunate side effect of possibly causing the error code
> to bleed through to the final
> 
> out:
> 	return copied ? : err;
> 
> and then to be wrongly returned if no data was copied because the caller
> didn't supply a data buffer, as demonstrated by the program available at
> 
> http://pad.lv/1540731
> 
> Change it such that err is only set if an error condition was detected.
> 
> Fixes: 3822b5c2fc62 ("af_unix: Revert 'lock_interruptible' in stream receive code")
> Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>

Applied, thanks Rainer.

And BTW I disagree with some of the feedback I saw in these threads
about "if (x) goto out;" being unreadable and that it should be avoided.

That's completely wrong.

Fact is, we've all been reading code of that form for multiple decades.
So it's the style we are _MOST_ familiar with, and it is therefore the
style that is the easiest and clearest for kernel developers to understand.

Especially those of us who review hundreds of patches per day.

And it doesn't matter at all what the compiler does underneath.

Furthermore, such a style works best in the long term because if real
cleanup operations are added for exit from the function, less has to
change and such patches are therefore significantly easier to review.

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

On Tue, 2016-02-16 at 12:51 -0500, David Miller wrote:
> From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> Date: Mon, 08 Feb 2016 18:47:19 +0000
> 
> > The present unix_stream_read_generic contains various code sequences of
> > the form
> > 
> > err = -EDISASTER;
> > if ()
> >       goto out;
> > 
> > This has the unfortunate side effect of possibly causing the error code
> > to bleed through to the final
> > 
> > out:
> >       return copied ? : err;
> > 
> > and then to be wrongly returned if no data was copied because the caller
> > didn't supply a data buffer, as demonstrated by the program available at
> > 
> > http://pad.lv/1540731
> > 
> > Change it such that err is only set if an error condition was detected.
> > 
> > Fixes: 3822b5c2fc62 ("af_unix: Revert 'lock_interruptible' in stream receive code")
> > Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> > Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> 
> Applied, thanks Rainer.
> 
> And BTW I disagree with some of the feedback I saw in these threads
> about "if (x) goto out;" being unreadable and that it should be avoided.

That's not what I said.

> That's completely wrong.
> 
> Fact is, we've all been reading code of that form for multiple decades.
> So it's the style we are _MOST_ familiar with, and it is therefore the
> style that is the easiest and clearest for kernel developers to understand.
[...]

I agree that 'if (err) goto cleanup;' is widely used and is generally
understandable (though more creative uses of goto are often not).

My objection was to 'err = -EFOO; if (cond) goto cleanup;'.  That is definitely not clear and it hides mistakes like this.

Ben.

-- 
Ben Hutchings
Lowery's Law:
             If it jams, force it. If it breaks, it needed replacing anyway.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Wed, 17 Feb 2016 00:24:51 +0000

> I agree that 'if (err) goto cleanup;' is widely used and is generally
> understandable (though more creative uses of goto are often not).
> 
> My objection was to 'err = -EFOO; if (cond) goto cleanup;'.  That is
> definitely not clear and it hides mistakes like this.

I don't see any difference whatsoever.

Part of the convention of the cleanup blob at the end of the
function is that error propagate to it's return statement via
a variable.

If this code wanted to handle that in more than one way, it is
the problem of this function, not of the convention itself.

Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Wed, 17 Feb 2016 00:24:51 +0000

> I agree that 'if (err) goto cleanup;' is widely used and is generally
> understandable (though more creative uses of goto are often not).
> 
> My objection was to 'err = -EFOO; if (cond) goto cleanup;'. �That is
> definitely not clear and it hides mistakes like this.

I don't see any difference whatsoever.

Part of the convention of the cleanup blob at the end of the
function is that error propagate to it's return statement via
a variable.

If this code wanted to handle that in more than one way, it is
the problem of this function, not of the convention itself.