linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Mis-backport in af_unix patch for Linux 3.10.95
@ 2016-01-24  5:10 Sultan Qasim
  2016-01-24  8:31 ` Willy Tarreau
  0 siblings, 1 reply; 4+ messages in thread
From: Sultan Qasim @ 2016-01-24  5:10 UTC (permalink / raw)
  To: stable, linux-kernel

Hello all,

I'm an outsider to the Linux kernel community, so I apologize if this
is not the right channel to mention this. I noticed that the
backported version of the patch "af_unix: Revert 'lock_interruptible'
in stream receive code" in Linux 3.10.95 seems to have removed the
mutex_lock_interruptible from the wrong function.

Here is the backported patch:
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=3a57e783016bf43ab9326172217f564941b85b17

Here is the original:
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/net/unix/af_unix.c?id=3822b5c2fc62e3de8a0f33806ff279fb7df92432

Was it not meant to be removed from unix_stream_recvmsg instead of
unix_dgram_recvmsg? Also, the variable called "noblock" needs to be
removed from the function being changed to prevent unused variable
warnings.

Sultan Q. Khan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Mis-backport in af_unix patch for Linux 3.10.95
  2016-01-24  5:10 Mis-backport in af_unix patch for Linux 3.10.95 Sultan Qasim
@ 2016-01-24  8:31 ` Willy Tarreau
  2016-01-24 16:16   ` Sultan Qasim
  0 siblings, 1 reply; 4+ messages in thread
From: Willy Tarreau @ 2016-01-24  8:31 UTC (permalink / raw)
  To: Sultan Qasim
  Cc: stable, linux-kernel, Rainer Weikusat, Hannes Frederic Sowa,
	David S. Miller, netdev

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

Hello,

On Sun, Jan 24, 2016 at 12:10:35AM -0500, Sultan Qasim wrote:
> Hello all,
> 
> I'm an outsider to the Linux kernel community, so I apologize if this
> is not the right channel to mention this.

The simple fact that you participate, inspect the code and report bugs
makes you part of this community :-)  It's indeed the right place.
Usually when reporting an issue with a commit, we also CC the whole
signed-off-by / CC chain of that commit (which I'm doing now). For
bugs related to networking, we usually CC the netdev list as well.

> I noticed that the
> backported version of the patch "af_unix: Revert 'lock_interruptible'
> in stream receive code" in Linux 3.10.95 seems to have removed the
> mutex_lock_interruptible from the wrong function.
>
> Here is the backported patch:
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=3a57e783016bf43ab9326172217f564941b85b17
> 
> Here is the original:
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/net/unix/af_unix.c?id=3822b5c2fc62e3de8a0f33806ff279fb7df92432
> 
> Was it not meant to be removed from unix_stream_recvmsg instead of
> unix_dgram_recvmsg?

You're absolutely right, good catch! Similar controls were added to
both functions resulting in the same code appearing there, which
confused the patch process, causing the change to be applied to the
wrong location. This happens from time to time in such circumstances
when backporting to older kernels.

> Also, the variable called "noblock" needs to be
> removed from the function being changed to prevent unused variable
> warnings.

If you mean this variable in function unix_dgram_recvmsg(), it would
indeed report a warning but only due to the patch being mis-applied.
In unix_stream_recvmsg(), it's still used as well.

Does the attached patch seem better to you (not compile-tested) ?

Greg/Ben, both 3.2.76 and 3.14.59 are OK regarding this, it seems
like only 3.10.95 was affected.

Thanks,
Willy


[-- Attachment #2: 0001-af_unix-fix-incorrect-revert-of-lock_interruptible-i.patch --]
[-- Type: text/plain, Size: 1905 bytes --]

>From 77f6e82adf349cbccf7e2ec7601b25c994e0b483 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Sun, 24 Jan 2016 09:19:57 +0100
Subject: af_unix: fix incorrect revert of 'lock_interruptible' in stream
 receive code

As reported by Sultan Qasim, commit 3822b5c ("af_unix: Revert
'lock_interruptible' in stream receive code") was accidently applied
at the wrong place in the backport that appeared in 3.10.95, it
affected unix_dgram_recvmsg() instead of unix_stream_recvmsg() due
to now similar code sections there. The dgram part needs to remain
but the stream part needs to be removed.

Reported-By: Sultan Qasim <sultanqasim@gmail.com>
Fixes: 3a57e78 (3.10.95)
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 net/unix/af_unix.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f934e7b..0061d00 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1934,7 +1934,14 @@ static int unix_dgram_recvmsg(struct kiocb *iocb, struct socket *sock,
 	if (flags&MSG_OOB)
 		goto out;
 
-	mutex_lock(&u->readlock);
+	err = mutex_lock_interruptible(&u->readlock);
+	if (unlikely(err)) {
+		/* recvmsg() in non blocking mode is supposed to return -EAGAIN
+		 * sk_rcvtimeo is not honored by mutex_lock_interruptible()
+		 */
+		err = noblock ? -EAGAIN : -ERESTARTSYS;
+		goto out;
+	}
 
 	skip = sk_peek_offset(sk, flags);
 
@@ -2083,14 +2090,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		memset(&tmp_scm, 0, sizeof(tmp_scm));
 	}
 
-	err = mutex_lock_interruptible(&u->readlock);
-	if (unlikely(err)) {
-		/* recvmsg() in non blocking mode is supposed to return -EAGAIN
-		 * sk_rcvtimeo is not honored by mutex_lock_interruptible()
-		 */
-		err = noblock ? -EAGAIN : -ERESTARTSYS;
-		goto out;
-	}
+	mutex_lock(&u->readlock);
 
 	do {
 		int chunk;
-- 
1.7.12.2.21.g234cd45.dirty


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: Mis-backport in af_unix patch for Linux 3.10.95
  2016-01-24  8:31 ` Willy Tarreau
@ 2016-01-24 16:16   ` Sultan Qasim
  2016-01-24 21:40     ` Greg KH
  0 siblings, 1 reply; 4+ messages in thread
From: Sultan Qasim @ 2016-01-24 16:16 UTC (permalink / raw)
  To: Willy Tarreau
  Cc: stable, linux-kernel, Rainer Weikusat, Hannes Frederic Sowa,
	David S. Miller, netdev

Hello,

Thank you very much for the warm welcome :-)

You're right, the noblock variable is used elsewhere in the stream
receive function, so nothing is needed there after the interruptible
logic is restored for the dgram function.

Your patch looks good to me. I had picked the Linux 3.12.52 version of
the patch (where the interruptible locking was removed from the right
place in the stream receive function) onto my 3.10 kernel branch a
couple weeks ago and it has been working fine for me.

Thanks,
Sultan

On Sun, Jan 24, 2016 at 3:31 AM, Willy Tarreau <w@1wt.eu> wrote:
> Hello,
>
> On Sun, Jan 24, 2016 at 12:10:35AM -0500, Sultan Qasim wrote:
>> Hello all,
>>
>> I'm an outsider to the Linux kernel community, so I apologize if this
>> is not the right channel to mention this.
>
> The simple fact that you participate, inspect the code and report bugs
> makes you part of this community :-)  It's indeed the right place.
> Usually when reporting an issue with a commit, we also CC the whole
> signed-off-by / CC chain of that commit (which I'm doing now). For
> bugs related to networking, we usually CC the netdev list as well.
>
>> I noticed that the
>> backported version of the patch "af_unix: Revert 'lock_interruptible'
>> in stream receive code" in Linux 3.10.95 seems to have removed the
>> mutex_lock_interruptible from the wrong function.
>>
>> Here is the backported patch:
>> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=3a57e783016bf43ab9326172217f564941b85b17
>>
>> Here is the original:
>> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/net/unix/af_unix.c?id=3822b5c2fc62e3de8a0f33806ff279fb7df92432
>>
>> Was it not meant to be removed from unix_stream_recvmsg instead of
>> unix_dgram_recvmsg?
>
> You're absolutely right, good catch! Similar controls were added to
> both functions resulting in the same code appearing there, which
> confused the patch process, causing the change to be applied to the
> wrong location. This happens from time to time in such circumstances
> when backporting to older kernels.
>
>> Also, the variable called "noblock" needs to be
>> removed from the function being changed to prevent unused variable
>> warnings.
>
> If you mean this variable in function unix_dgram_recvmsg(), it would
> indeed report a warning but only due to the patch being mis-applied.
> In unix_stream_recvmsg(), it's still used as well.
>
> Does the attached patch seem better to you (not compile-tested) ?
>
> Greg/Ben, both 3.2.76 and 3.14.59 are OK regarding this, it seems
> like only 3.10.95 was affected.
>
> Thanks,
> Willy
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Mis-backport in af_unix patch for Linux 3.10.95
  2016-01-24 16:16   ` Sultan Qasim
@ 2016-01-24 21:40     ` Greg KH
  0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2016-01-24 21:40 UTC (permalink / raw)
  To: Sultan Qasim
  Cc: Willy Tarreau, stable, linux-kernel, Rainer Weikusat,
	Hannes Frederic Sowa, David S. Miller, netdev

On Sun, Jan 24, 2016 at 11:16:25AM -0500, Sultan Qasim wrote:
> Hello,
> 
> Thank you very much for the warm welcome :-)
> 
> You're right, the noblock variable is used elsewhere in the stream
> receive function, so nothing is needed there after the interruptible
> logic is restored for the dgram function.
> 
> Your patch looks good to me. I had picked the Linux 3.12.52 version of
> the patch (where the interruptible locking was removed from the right
> place in the stream receive function) onto my 3.10 kernel branch a
> couple weeks ago and it has been working fine for me.

Great, thanks for reviewing and letting us know about this, I've queued
Willy's patch up now for inclusion in the next 3.10-stable release.

greg k-h

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-01-24 21:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-24  5:10 Mis-backport in af_unix patch for Linux 3.10.95 Sultan Qasim
2016-01-24  8:31 ` Willy Tarreau
2016-01-24 16:16   ` Sultan Qasim
2016-01-24 21:40     ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).