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

[Joseph Salisbury <joseph.salisbury@canonical.com>]

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