Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1328006 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream  receive code Joseph Salisbury <joseph.salisbury@canonical.com> - 2016-02-05 20:00 +0100
    Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-05 21:00 +0100
      Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in  stream receive code Joseph Salisbury <joseph.salisbury@canonical.com> - 2016-02-05 21:10 +0100
        Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-05 22:20 +0100
          Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-05 23:10 +0100
    Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-05 22:50 +0100
      Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in  stream receive code Eric Dumazet <eric.dumazet@gmail.com> - 2016-02-06 00:10 +0100
        Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-07 19:50 +0100
          Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-07 21:40 +0100
          Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in stream receive code Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-07 23:30 +0100
            Re: [V4.4-rc6 Regression] af_unix: Revert 'lock_interruptible' in  stream receive code Eric Dumazet <eric.dumazet@gmail.com> - 2016-02-08 04:40 +0100
    [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-05 23:40 +0100
      Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-07 20:30 +0100
        [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-08 16:40 +0100
          Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-08 19:10 +0100
            [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-02-08 19:50 +0100
          Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless  there was an error Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-02-08 19:40 +0100

#1328006 — [V4.4-rc6 Regression] 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 <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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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? :-)

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web