[PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Some applications didn't expect recvmsg() on a non blocking socket
could return -EINTR. This possibility was added as a side effect
of commit b3ca9b02b00704 ("net: fix multithreaded signal handling in
unix recv routines").

To hit this bug, you need to be a bit unlucky, as the u->readlock
mutex is usually held for very small periods.

Fixes: b3ca9b02b00704 ("net: fix multithreaded signal handling in unix recv routines")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---
 net/unix/af_unix.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ce6ec6c2f4de..94404f19f9de 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1787,8 +1787,11 @@ static int unix_dgram_recvmsg(struct kiocb *iocb, struct socket *sock,
 		goto out;
 
 	err = mutex_lock_interruptible(&u->readlock);
-	if (err) {
-		err = sock_intr_errno(sock_rcvtimeo(sk, noblock));
+	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;
 	}
 
@@ -1913,6 +1916,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 	struct unix_sock *u = unix_sk(sk);
 	DECLARE_SOCKADDR(struct sockaddr_un *, sunaddr, msg->msg_name);
 	int copied = 0;
+	int noblock = flags & MSG_DONTWAIT;
 	int check_creds = 0;
 	int target;
 	int err = 0;
@@ -1928,7 +1932,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 		goto out;
 
 	target = sock_rcvlowat(sk, flags&MSG_WAITALL, size);
-	timeo = sock_rcvtimeo(sk, flags&MSG_DONTWAIT);
+	timeo = sock_rcvtimeo(sk, noblock);
 
 	/* Lock the socket to prevent queue disordering
 	 * while sleeps in memcpy_tomsg
@@ -1940,8 +1944,11 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
 	}
 
 	err = mutex_lock_interruptible(&u->readlock);
-	if (err) {
-		err = sock_intr_errno(timeo);
+	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;
 	}
 

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> From: Eric Dumazet <edumazet@google.com>
>
> Some applications didn't expect recvmsg() on a non blocking socket
> could return -EINTR. This possibility was added as a side effect
> of commit b3ca9b02b00704 ("net: fix multithreaded signal handling in
> unix recv routines").
>
> To hit this bug, you need to be a bit unlucky, as the u->readlock
> mutex is usually held for very small periods.

This would mean that 'some applications' are broken, cf

[EAGAIN] or [EWOULDBLOCK]
    The socket's file descriptor is marked O_NONBLOCK and no data is
    waiting to be received; or MSG_OOB is set and no out-of-band data is
    available and either the socket's file descriptor is marked
    O_NONBLOCK or the socket does not support blocking to await
    out-of-band data.

[EINTR]
    This function was interrupted by a signal before any data was
    available.
    
http://pubs.opengroup.org/onlinepubs/9699919799/functions/recvmsg.html

and

 EAGAIN or EWOULDBLOCK
              The socket is marked nonblocking and the receive operation
              would block, or a receive timeout had been set and the
              timeout expired before data was received.

 EINTR  The receive was interrupted by delivery of a signal before any
        data were available; see signal(7).

[3.27 Linux recvmsg(2)]

since the function was interrupted before any data was available and it
is unknown if the condition supposed to be signalled by EAGAIN had
otherwise occurred.

A correct 'fix'/ workaround would seem to be using mutex_trylock and
abort execution immediately with -EAGAIN in case the operation had to
wait for the lock, although this is inconsistent with the usual
semantics of 'blocking' which implies that the operation may take an
indefinite amount of time because it waits for an external event which
might never occur.
 
              
>
> Fixes: b3ca9b02b00704 ("net: fix multithreaded signal handling in unix recv routines")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> ---
>  net/unix/af_unix.c |   17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index ce6ec6c2f4de..94404f19f9de 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1787,8 +1787,11 @@ static int unix_dgram_recvmsg(struct kiocb *iocb, struct socket *sock,
>  		goto out;
>  
>  	err = mutex_lock_interruptible(&u->readlock);
> -	if (err) {
> -		err = sock_intr_errno(sock_rcvtimeo(sk, noblock));
> +	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;
>  	}
>  
> @@ -1913,6 +1916,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	struct unix_sock *u = unix_sk(sk);
>  	DECLARE_SOCKADDR(struct sockaddr_un *, sunaddr, msg->msg_name);
>  	int copied = 0;
> +	int noblock = flags & MSG_DONTWAIT;
>  	int check_creds = 0;
>  	int target;
>  	int err = 0;
> @@ -1928,7 +1932,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  		goto out;
>  
>  	target = sock_rcvlowat(sk, flags&MSG_WAITALL, size);
> -	timeo = sock_rcvtimeo(sk, flags&MSG_DONTWAIT);
> +	timeo = sock_rcvtimeo(sk, noblock);
>  
>  	/* Lock the socket to prevent queue disordering
>  	 * while sleeps in memcpy_tomsg
> @@ -1940,8 +1944,11 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>  	}
>  
>  	err = mutex_lock_interruptible(&u->readlock);
> -	if (err) {
> -		err = sock_intr_errno(timeo);
> +	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;
>  	}
>  

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Rainer Weikusat <rw@sable> writes:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> Some applications didn't expect recvmsg() on a non blocking socket
>> could return -EINTR. This possibility was added as a side effect
>> of commit b3ca9b02b00704 ("net: fix multithreaded signal handling in
>> unix recv routines").
>>
>> To hit this bug, you need to be a bit unlucky, as the u->readlock
>> mutex is usually held for very small periods.

[...]

> the function was interrupted before any data was available and it
> is unknown if the condition supposed to be signalled by EAGAIN had
> otherwise occurred.

Further explanation of the difference:

do
	rc = recv(...);
while (rc == -1 && errno == EINTR);

is 'usually sensible code' while

do
	rc = recv(...);
while (rc == -1 && errno == EAGAIN);

usually isn't: One is a one-off event not expected to occur again
'soon', the other implies that "do something else for a while" would be
advisable. In particular, EAGAIN usually shouldn't happen after 'data is
available now' was signalled by select/ poll/ $you_name_it and while it
would be prudent to handle it nevertheless (especially considering that
Linux doesn't necessarily behave in this way), there are doubtlessly
applications 'unprepared' for that, too.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Eric Dumazet]

On Wed, 2014-03-26 at 13:17 +0000, Rainer Weikusat wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Some applications didn't expect recvmsg() on a non blocking socket
> > could return -EINTR. This possibility was added as a side effect
> > of commit b3ca9b02b00704 ("net: fix multithreaded signal handling in
> > unix recv routines").
> >
> > To hit this bug, you need to be a bit unlucky, as the u->readlock
> > mutex is usually held for very small periods.
> 
> This would mean that 'some applications' are broken, cf
> 
> [EAGAIN] or [EWOULDBLOCK]
>     The socket's file descriptor is marked O_NONBLOCK and no data is
>     waiting to be received; or MSG_OOB is set and no out-of-band data is
>     available and either the socket's file descriptor is marked
>     O_NONBLOCK or the socket does not support blocking to await
>     out-of-band data.
> 
> [EINTR]
>     This function was interrupted by a signal before any data was
>     available.
>     
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/recvmsg.html
> 
> and
> 
>  EAGAIN or EWOULDBLOCK
>               The socket is marked nonblocking and the receive operation
>               would block, or a receive timeout had been set and the
>               timeout expired before data was received.
> 
>  EINTR  The receive was interrupted by delivery of a signal before any
>         data were available; see signal(7).
> 
> [3.27 Linux recvmsg(2)]
> 
> since the function was interrupted before any data was available and it
> is unknown if the condition supposed to be signalled by EAGAIN had
> otherwise occurred.
> 
> A correct 'fix'/ workaround would seem to be using mutex_trylock and
> abort execution immediately with -EAGAIN in case the operation had to
> wait for the lock, although this is inconsistent with the usual
> semantics of 'blocking' which implies that the operation may take an
> indefinite amount of time because it waits for an external event which
> might never occur.

Before your patch, -EAGAIN was delivered, -EINTR was _never_ delivered.

Thats a fact. If you read the manpage, its in this order :

When using a nonblocking socket, and no data is available, EAGAIN or
EWOULDBLOCK is delivered.

-EINTR is not expected from a non blocking system call. Period.

After your patch, -EINTR might be delivered, and it breaks legacy
applications.




Please send a fix if you think you have a better one, thanks

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2014-03-26 at 13:17 +0000, Rainer Weikusat wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> > From: Eric Dumazet <edumazet@google.com>
>> >
>> > Some applications didn't expect recvmsg() on a non blocking socket
>> > could return -EINTR. This possibility was added as a side effect
>> > of commit b3ca9b02b00704 ("net: fix multithreaded signal handling in
>> > unix recv routines").
>> >
>> > To hit this bug, you need to be a bit unlucky, as the u->readlock
>> > mutex is usually held for very small periods.
>> 
>> This would mean that 'some applications' are broken, cf
>> 
>> [EAGAIN] or [EWOULDBLOCK]
>>     The socket's file descriptor is marked O_NONBLOCK and no data is
>>     waiting to be received; or MSG_OOB is set and no out-of-band data is
>>     available and either the socket's file descriptor is marked
>>     O_NONBLOCK or the socket does not support blocking to await
>>     out-of-band data.
>> 
>> [EINTR]
>>     This function was interrupted by a signal before any data was
>>     available.
>>     
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/recvmsg.html
>> 
>> and
>> 
>>  EAGAIN or EWOULDBLOCK
>>               The socket is marked nonblocking and the receive operation
>>               would block, or a receive timeout had been set and the
>>               timeout expired before data was received.
>> 
>>  EINTR  The receive was interrupted by delivery of a signal before any
>>         data were available; see signal(7).
>> 
>> [3.27 Linux recvmsg(2)]
>> 
>> since the function was interrupted before any data was available and it
>> is unknown if the condition supposed to be signalled by EAGAIN had
>> otherwise occurred.
>> 
>> A correct 'fix'/ workaround would seem to be using mutex_trylock and
>> abort execution immediately with -EAGAIN in case the operation had to
>> wait for the lock, although this is inconsistent with the usual
>> semantics of 'blocking' which implies that the operation may take an
>> indefinite amount of time because it waits for an external event which
>> might never occur.
>
> Before your patch, -EAGAIN was delivered, -EINTR was _never_ delivered.
>
> Thats a fact.

Indeed. Before this change, a signal could silently get lost for reasons
I outlined in the original mail. This is now no longer the case.

> If you read the manpage, its in this order :
>
> When using a nonblocking socket, and no data is available, EAGAIN or
> EWOULDBLOCK is delivered.
>
> -EINTR is not expected from a non blocking system call. Period.

That's your interpretation of a text which doesn't say so unambigiously
(actually, it pretty much says the exact opposite) and I think this
interpretation is not correct for the reasons I gave (in particular,
that it is unknown if data is available and that EAGAIN implies that it
is known that data is not available).

This is a somewhat interesting problem from a theoretical standpoint,
because there is really no totally satisfactory solution, however, I'm
presently 'over-the-top' buried below a heap of seriously grotty and
misbehaving Java code. Consequently, I suggest to end this discussion
here.

RE: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[David Laight]

From: Eric Dumazet
> From: Eric Dumazet <edumazet@google.com>
> 
> Some applications didn't expect recvmsg() on a non blocking socket
> could return -EINTR. This possibility was added as a side effect
> of commit b3ca9b02b00704 ("net: fix multithreaded signal handling in
> unix recv routines").
> 
> To hit this bug, you need to be a bit unlucky, as the u->readlock
> mutex is usually held for very small periods.

The commit message for b3ca9b02b00704 looks very strange.
Maybe something else is wrong.

If we assume that u->readlock is only held for a short period
why should it matter than the kernel decided to give the
signal to that thread?

There is the hint that if there are multiple readers, all
but one are blocked waiting for u->readlock.
But I should be able to have on thread/process doing
a blocking read and a second doing a non-blocking read
(through a different fd).

If we assume that all readers are either blocking or non-blocking
then maybe u->readlock should be acquired non-interruptibly
in the non-block case.

Then you don't have to worry about the return code.

	David

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

David Laight <David.Laight@ACULAB.COM> writes:
> From: Eric Dumazet
>> From: Eric Dumazet <edumazet@google.com>
>> 
>> Some applications didn't expect recvmsg() on a non blocking socket
>> could return -EINTR. This possibility was added as a side effect
>> of commit b3ca9b02b00704 ("net: fix multithreaded signal handling in
>> unix recv routines").
>> 
>> To hit this bug, you need to be a bit unlucky, as the u->readlock
>> mutex is usually held for very small periods.
>
> The commit message for b3ca9b02b00704 looks very strange.
> Maybe something else is wrong.
>
> If we assume that u->readlock is only held for a short period
> why should it matter than the kernel decided to give the
> signal to that thread?

(This is from memory) If there's a thread blocked in recv and another
blocked on the lock and the kernel selects the thread blocked on the lock
for handling the signal, the signal won't be handled until some data is
received on the socket, ie, possibly never.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Eric Dumazet]

On Wed, 2014-03-26 at 15:13 +0000, Rainer Weikusat wrote:

> (This is from memory) If there's a thread blocked in recv and another
> blocked on the lock and the kernel selects the thread blocked on the lock
> for handling the signal, the signal won't be handled until some data is
> received on the socket, ie, possibly never.

Look at what we do for AF_INET. We handle this the proper way.

If we are 'interrupted' by a signal while sleeping in lock_sock(),
recvmsg() on a non blocking socket, we return -EAGAIN properly, not
-EINTR.

Fact that we potentially sleep to get the socket lock is hidden for the
user, its an implementation detail of the kernel.

We never return -EINTR, as stated in manpage for non blocking sockets.

Only AF_UNIX in recent kernels introduced this API change.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Eric Dumazet]

On Wed, 2014-03-26 at 08:25 -0700, Eric Dumazet wrote:

> Look at what we do for AF_INET. We handle this the proper way.
> 
> If we are 'interrupted' by a signal while sleeping in lock_sock(),
> recvmsg() on a non blocking socket, we return -EAGAIN properly, not
> -EINTR.
> 
> Fact that we potentially sleep to get the socket lock is hidden for the
> user, its an implementation detail of the kernel.

And this is done pretty much the same way I did in my patch :

if (signal_pending(current)) {
    copied = timeo ? sock_intr_errno(timeo) : -EAGAIN;
    break;
}

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2014-03-26 at 15:13 +0000, Rainer Weikusat wrote:
>
>> (This is from memory) If there's a thread blocked in recv and another
>> blocked on the lock and the kernel selects the thread blocked on the lock
>> for handling the signal, the signal won't be handled until some data is
>> received on the socket, ie, possibly never.
>
> Look at what we do for AF_INET. We handle this the proper way.
>
> If we are 'interrupted' by a signal while sleeping in lock_sock(),
> recvmsg() on a non blocking socket, we return -EAGAIN properly, not
> -EINTR.
>
> Fact that we potentially sleep to get the socket lock is hidden for the
> user, its an implementation detail of the kernel.
>
> We never return -EINTR, as stated in manpage for non blocking sockets.

There is (to the best of my knowledge) no man page which specifically
states that "socket is non-blocking" means "receive operation can't be
interrupted by a signal before any data was received" and telling
userspace "no data is available right now" when the operation was
actually interrupted by a signal before checking is still inaccurate.
The underlying problem would seem to be that a O_NONBLOCK call might
actually block forever in case a blocking receiver sits on the lock and
no data is ever received. Hence my original suggestion that it should
use a trylock-operation. But there are presumably downsides to this as
well, I didn't really think through it.

As I already wrote, this is a theoretical problem with no really
satisfactory solution minus noting that both 'nonblocking sockets' and
'multithreaded kernels' are somewhat alien concepts when considering the
system where EINTR originated (AFAIK). Also, it doesn't really matter:
If it was commonly assumed that O_NONBLOCK would imply 'no EINTR', the
kernel should behave accordingly for pragmatic reasons, not because
"commonly assumed" is the same as "obviously correct" (although it is
not uncommon for people to assume that common assumption are
must be obviously correct because they're common assumptions :-).

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Rainer Weikusat <rw@sable> writes:

[...]

> The underlying problem would seem to be that a O_NONBLOCK call might
> actually block forever in case a blocking receiver sits on the lock and
> no data is ever received.

... except that this probably cannot happen because O_NONBLOCK is a file
status flag and not a file descriptor flag.

NB: I've neither tested nor checked this.

RE: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[David Laight]

From: Rainer Weikusat 
> Rainer Weikusat <rw@sable> writes:
> 
> [...]
> 
> > The underlying problem would seem to be that a O_NONBLOCK call might
> > actually block forever in case a blocking receiver sits on the lock and
> > no data is ever received.
> 
> ... except that this probably cannot happen because O_NONBLOCK is a file
> status flag and not a file descriptor flag.
> 
> NB: I've neither tested nor checked this.

While dup() gives a second fd referring to the same kernel 'file'
doing open("/dev/fd/4", ...) traditionally gives you an additional
'file' referring to the same vnode.
For real files the file offset is in the 'file' structure, and
I think the O_NONBLOCK flag is in the same place.
Which means that is possible (but maybe not that usual or sensible)
for a process to try a non-blocking read on a socket while another
process is blocked in the read code.

The same would be true for writes, and for writes to a datagram
socket it might even make sense.

In any case I expect EAGAIN to mean 'there is no data to read'
not 'something happened and I didn't bother to look for data'.

As the code stands (or with the suggested change) if a datagram
socket has an indefinite supply of data, and several processes
are doing nonblocking reads, and some are also receiving signals
then some of the reads will fail with EINTR/EAGAIN/EWOULDBLOCK.
That doesn't seem right.

	David

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

David Laight <David.Laight@ACULAB.COM> writes:
> From: Rainer Weikusat 
>> Rainer Weikusat <rw@sable> writes:
>> 
>> [...]
>> 
>> > The underlying problem would seem to be that a O_NONBLOCK call might
>> > actually block forever in case a blocking receiver sits on the lock and
>> > no data is ever received.
>> 
>> ... except that this probably cannot happen because O_NONBLOCK is a file
>> status flag and not a file descriptor flag.
>> 
>> NB: I've neither tested nor checked this.
>
> While dup() gives a second fd referring to the same kernel 'file'
> doing open("/dev/fd/4", ...) traditionally gives you an additional
> 'file' referring to the same vnode.
> For real files the file offset is in the 'file' structure, and
> I think the O_NONBLOCK flag is in the same place.
> Which means that is possible (but maybe not that usual or sensible)
> for a process to try a non-blocking read on a socket while another
> process is blocked in the read code.
>
> The same would be true for writes, and for writes to a datagram
> socket it might even make sense.
>
> In any case I expect EAGAIN to mean 'there is no data to read'
> not 'something happened and I didn't bother to look for data'.

The problem is really that the non-blocking thread shouldn't be
interruptible and hence, should never return an EINTR error because of
this. As shown elsewhere, this is not only a theoretical concern but
actually real bug, as a read-call made while the socket is non-blocking
may actually stop executing forever if a prior, blocking read call is
already blocked. When assuming that the non-blocking call should execute
in favour of a blocking call which is actually blocked, this could be
regarded as a 'priority inversion' problem. OTOH, there is not 'perfect'
solution, IOW, one which doesn't involve the non-blocking read to give
up without trying 'immediately before' it had succeeded had it tried.

The 'return EAGAIN in an EINTR situation' is really a lame attempt at
hiding the real problem. A slightly better idea would be that the
non-blocking call should use trylock and return EAGAIN if this didn't
succeed. This would at least prevent it from becoming blocked for an
indefinite time.

A possible improvement would be to record if the thread currently
holding the lock made a blocking or a non-blocking call and use a
non-interruptible wait for the latter case since the lock ought to
become free soon. Problem with this: Another blocking reader could
appear while the non-blocking one is waiting an grab the lock instead.
This could presumably be preventend, but I doubt it'll be worth the
effort for something which seems to be a corner case.

Lastly, the non-blocking read could wait for a bounded time and give up
afterwards. Which turns this into a 'tuning' problem because there's no
good way to determine the 'right' bounded time.

Considering all of this, the trylock-approach seems best to me. OTOH,
I'm find with any behaviour which does not restore the original 'lost
wakeup' bug and considering that "it is a standard procedure to harras
people who are so careless to contribute bug fixes to Linux until the
cow comes home and they'd better be quiet about that!", as Mr Eric
"/dev/null" Dumasomething explained, I certainly don't plan to turn this
conviction of mine into a proper patch.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Jamal Hadi Salim]

On 03/27/14 08:40, Rainer Weikusat wrote:
> David Laight <David.Laight@ACULAB.COM> writes:
>
> I'm find with any behaviour which does not restore the original 'lost
> wakeup' bug and considering that "it is a standard procedure to harras
> people who are so careless to contribute bug fixes to Linux until the
> cow comes home and they'd better be quiet about that!", as Mr Eric
> "/dev/null" Dumasomething explained, I certainly don't plan to turn this
> conviction of mine into a proper patch.

I think anything you say past this point nobody is listening.
You may actually have something of value to say but the passive
aggressiveness is going to bite your ass.

cheers,
jamal

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Jamal Hadi Salim <jhs@mojatatu.com> writes:
> On 03/27/14 08:40, Rainer Weikusat wrote:
>> David Laight <David.Laight@ACULAB.COM> writes:
>>
>> I'm find with any behaviour which does not restore the original 'lost
>> wakeup' bug and considering that "it is a standard procedure to harras
>> people who are so careless to contribute bug fixes to Linux until the
>> cow comes home and they'd better be quiet about that!", as Mr Eric
>> "/dev/null" Dumasomething explained, I certainly don't plan to turn this
>> conviction of mine into a proper patch.
>
> I think anything you say past this point nobody is listening.

Why would I care if someone like you 'listens' to me?

RE: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[David Laight]

From: Rainer Weikusat 
> Considering all of this, the trylock-approach seems best to me. OTOH,

No - using trylock() is definitely wrong.
If there are two messages queued and two processes/threads try to
read them, you don't want one of them being given EAGAIN.

If we ignore the case where there are some blocking and some
non-blocking readers (which doesn't work for other reasons)
then it would actually make sense for the non-blocking
code to wait uninterruptibly (possibly with a timeout that
should never expire) and for the blocking code to wait
interruptibly (as currently).

Then problem of which error code to return is moot.

	David

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

David Laight <David.Laight@ACULAB.COM> writes:
> From: Rainer Weikusat 
>> Considering all of this, the trylock-approach seems best to me. OTOH,
>
> No - using trylock() is definitely wrong.
> If there are two messages queued and two processes/threads try to
> read them, you don't want one of them being given EAGAIN.

I'd consider this acceptable because a second message could also come in
at the very moment the 2nd reader gives up. This is the 'next simplest
approach' after 'returning EAGAIN instead of EINTR' and I think it is a
little less non-desirable because it avoids the mixed blocking/
non-blocking problem (presently, I don't see why this shouldn't work,
ie, a process which inherited a socket ought to be able to switch that
to non-blocking mode and then call recv without risking to stop
executing for 'a long time'), and it is still reasonably simple (although
the problem is likely rather contrived).

Anything beyond that would involve priorising non-blocking readers over
blocking ones and let them wait uninterruptedly in case the lock is
presently held by another non-blocking reader. But this would be a whole
lot more complicated and even waiting for a blocking reader might work
just fine if a message arrives soon enough. Which then again means "it
should wait some time uninterruptedly in any case" but how much time?

The whole issue would neatly "not exist" if only one process could ever
be executing kernel code at any given time, but since is not the case
anymore, some kind of 'heuristic solution based on a tradeoff' is
necessary.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[David Miller]

From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
Date: Wed, 26 Mar 2014 19:46:05 +0000

> As I already wrote, this is a theoretical problem with no really
> satisfactory solution minus noting that both 'nonblocking sockets' and
> 'multithreaded kernels' are somewhat alien concepts when considering the
> system where EINTR originated (AFAIK).

I completely disagree.

EINTR always means that you asked for a blocking operation, and a
signal arrived meanwhile.

Once you invert the "blocking" part of that set of conditrions, EINTR
becomes an impossible event.

I don't have time to check BSD, but I'm very confident that these are
the rules codified in their socket syscall layer too.

I'm going to apply Eric's patch.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

David Miller <davem@davemloft.net> writes:
> From: Rainer Weikusat <rweikusat@mobileactivedefense.com>:
>> As I already wrote, this is a theoretical problem with no really
>> satisfactory solution minus noting that both 'nonblocking sockets' and
>> 'multithreaded kernels' are somewhat alien concepts when considering the
>> system where EINTR originated (AFAIK).
>
> I completely disagree.

And I "completely disagree" with having written the text above, that is
minus the "reporting EAGAIN when the call was really interrupted by a
signal is inaccurate" and "for pragmatic reasons, the kernel should
behave in line with common expectations about its behaviour" parts which
means

a) This is an 'interesting theoretical problem to me' (but maybe not to
   you)

b) my suggestion would be to apply the patch regardless of it.

BTW, here's the program with the O_NONBLOCK read call which blocks until
the end of electricity, at least on 3.2.9:

---------
#include <fcntl.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/un.h>

int main(void)
{
    struct sockaddr_un sun;
    int fd;

    fd = socket(AF_UNIX, SOCK_DGRAM, 0);
    sun.sun_family = AF_UNIX;
    strncpy(sun.sun_path, "/tmp/bla", sizeof(sun.sun_path));
    bind(fd, (struct sockaddr *)&sun, sizeof(sun));

    if (fork() == 0) read(fd, &fd, sizeof(fd));

    sleep(1);

    fcntl(fd, F_SETFL, O_NONBLOCK);
    read(fd, &fd, sizeof(fd));

    return 0;
}
--------

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Eric Dumazet]

On Wed, 2014-03-26 at 21:21 +0000, Rainer Weikusat wrote:

> BTW, here's the program with the O_NONBLOCK read call which blocks until
> the end of electricity, at least on 3.2.9:
> 
> ---------
> #include <fcntl.h>
> #include <string.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> 
> int main(void)
> {
>     struct sockaddr_un sun;
>     int fd;
> 
>     fd = socket(AF_UNIX, SOCK_DGRAM, 0);
>     sun.sun_family = AF_UNIX;
>     strncpy(sun.sun_path, "/tmp/bla", sizeof(sun.sun_path));
>     bind(fd, (struct sockaddr *)&sun, sizeof(sun));
> 
>     if (fork() == 0) read(fd, &fd, sizeof(fd));
> 
>     sleep(1);
> 
>     fcntl(fd, F_SETFL, O_NONBLOCK);
>     read(fd, &fd, sizeof(fd));
> 
>     return 0;
> }

Sure, O_NONBLOCK is translated into MSG_NOWAIT one time per socket
syscall. 

Once we block, we don't automatically unblock if another thread change
the file flag.

grep -n MSG_DONTWAIT net/socket.c

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2014-03-26 at 21:21 +0000, Rainer Weikusat wrote:
>> BTW, here's the program with the O_NONBLOCK read call which blocks until
>> the end of electricity, at least on 3.2.9:
>> 
>> ---------
>> #include <fcntl.h>
>> #include <string.h>
>> #include <sys/socket.h>
>> #include <sys/un.h>
>> 
>> int main(void)
>> {
>>     struct sockaddr_un sun;
>>     int fd;
>> 
>>     fd = socket(AF_UNIX, SOCK_DGRAM, 0);
>>     sun.sun_family = AF_UNIX;
>>     strncpy(sun.sun_path, "/tmp/bla", sizeof(sun.sun_path));
>>     bind(fd, (struct sockaddr *)&sun, sizeof(sun));
>> 
>>     if (fork() == 0) read(fd, &fd, sizeof(fd));
>> 
>>     sleep(1);
>> 
>>     fcntl(fd, F_SETFL, O_NONBLOCK);
>>     read(fd, &fd, sizeof(fd));
>> 
>>     return 0;
>> }
>
> Sure, O_NONBLOCK is translated into MSG_NOWAIT one time per socket
> syscall. 
>
> Once we block, we don't automatically unblock if another thread change
> the file flag.

That would be a seriously bizarre idea. The thread of execution which
does the supposed-to-be-non-blocking call shouldn't become blocked for
an indefinite time. Which means it should not wait indefinitely for a
thread which - in turn - waits indefinitely for an external event (and
hence, the original problem should never have existed to begin with as
there would neither be an opportunity nor a reason to interrupt in the
non-blocking case).

BTW, while these are things which are of some interest to me (even of
some professional interest), I don't exactly get paid for these kind of
discussions, have some real work to do, and the way people interact on
LKML is seriously more aggressive then I can stand for a prolonged time,
so can we please end this discussion here while it - if only for a
change - hasn't yet degraded into an all out mutual extermination fight?

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Eric Dumazet]

On Wed, 2014-03-26 at 22:06 +0000, Rainer Weikusat wrote:

> That would be a seriously bizarre idea. The thread of execution which
> does the supposed-to-be-non-blocking call shouldn't become blocked for
> an indefinite time. Which means it should not wait indefinitely for a
> thread which - in turn - waits indefinitely for an external event (and
> hence, the original problem should never have existed to begin with as
> there would neither be an opportunity nor a reason to interrupt in the
> non-blocking case).


This is not what your program do.

Your program does a read() on a blocking fd.

If another thread does a close() on this fd or change non blocking flag,
it does or it doesn't impact first thread.

It is not part of any specification. So nobody would rely on this.


> BTW, while these are things which are of some interest to me (even of
> some professional interest), I don't exactly get paid for these kind of
> discussions, have some real work to do, and the way people interact on
> LKML is seriously more aggressive then I can stand for a prolonged time,
> so can we please end this discussion here while it - if only for a
> change - hasn't yet degraded into an all out mutual extermination fight?

...

I was not particularly aggressive, I tried to explain my points after
you said my patch was not the right way, and that thousands of
applications had to be identified for potential problems and patched
instead.

If you do not have time, I suggest you simply not spend time on this.

I CCed you because you were the patch author, not because I wanted your
approval or any comment, and not because I wanted to hurt you in any
way.

Thats the standard procedure for all patches, even ones coming from you.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Wed, 2014-03-26 at 22:06 +0000, Rainer Weikusat wrote:
>
>> That would be a seriously bizarre idea. The thread of execution which
>> does the supposed-to-be-non-blocking call shouldn't become blocked for
>> an indefinite time. Which means it should not wait indefinitely for a
>> thread which - in turn - waits indefinitely for an external event (and
>> hence, the original problem should never have existed to begin with as
>> there would neither be an opportunity nor a reason to interrupt in the
>> non-blocking case).
>
>
> This is not what your program do.
>
> Your program does a read() on a blocking fd.

It does both, actually: The forked process calls read while the socket
is still blocking, the other process calls read after it was switched to
non-blocking. This can easily be determined with strace.

Again, please stop dumping your infiltered rage onto me just because I
happen to disagree with you. You won't change that.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Eric Dumazet]

On Wed, 2014-03-26 at 22:51 +0000, Rainer Weikusat wrote:


> 
> Again, please stop dumping your infiltered rage onto me just because I
> happen to disagree with you. You won't change that.

Read again all my mails, I was factual.

It seems you are pretty upset, not me.

This will be my last mail, since it obviously bothers you very much.

Re: [PATCH] net: unix: non blocking recvmsg() should not return -EINTR

[Rainer Weikusat]

Rainer Weikusat <rw@sable> writes:

[...]

> ---------
> #include <fcntl.h>
> #include <string.h>
> #include <sys/socket.h>
> #include <sys/un.h>
>
> int main(void)
> {
>     struct sockaddr_un sun;
>     int fd;
>
>     fd = socket(AF_UNIX, SOCK_DGRAM, 0);
>     sun.sun_family = AF_UNIX;
>     strncpy(sun.sun_path, "/tmp/bla", sizeof(sun.sun_path));
>     bind(fd, (struct sockaddr *)&sun, sizeof(sun));
>
>     if (fork() == 0) read(fd, &fd, sizeof(fd));
>
>     sleep(1);
>
>     fcntl(fd, F_SETFL, O_NONBLOCK);
>     read(fd, &fd, sizeof(fd));
>
>     return 0;
> }
> --------

In the interest of fairness, here's a more realistic test case (tested
on 3.2.9 and 3.2.54):

--------------
#include <fcntl.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/un.h>

int main(void)
{
    struct sockaddr_un sun;
    int fd;

    fd = socket(AF_UNIX, SOCK_DGRAM, 0);
    sun.sun_family = AF_UNIX;
    strncpy(sun.sun_path, "/tmp/bla", sizeof(sun.sun_path));
    bind(fd, (struct sockaddr *)&sun, sizeof(sun));

    if (fork() == 0) recv(fd, &fd, sizeof(fd), 0);

    sleep(1);

    recv(fd, &fd, sizeof(fd), MSG_DONTWAIT);

    return 0;
}