af_unix: Revert 'lock_interruptible' in stream receive code

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 <[email protected]>
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

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:

---------

index 49d5093..c3e1a08 100644
@@ -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? :-)

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

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.

[...]

[...]

[...]

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;
}

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.

---

index 49d5093..138787d 100644
@@ -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,

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.

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).

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.

Rainer Weikusat <[email protected]> writes:

[...]

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

Rainer Weikusat <[email protected]> writes:

[...]

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.

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.

---

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

index 49d5093..138787d 100644
@@ -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,

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.

---

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.

index 49d5093..c1e4dd7 100644
@@ -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,

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.

---

index 49d5093..c1e4dd7 100644
@@ -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,

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.

---

With Fixes: fixed.

index 49d5093..c1e4dd7 100644
@@ -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,

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.

---

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

index 49d5093..c1e4dd7 100644
@@ -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,

From: Rainer Weikusat <[email protected]>
Date: Mon, 08 Feb 2016 18:47:19 +0000

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.

That's not what I said.

[...]

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.

From: Ben Hutchings <[email protected]>
Date: Wed, 17 Feb 2016 00:24:51 +0000

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.

Hello.

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

[...]

MBR, Sergei

[...]

[...]

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.

[...]

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.