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

Groups > linux.kernel > #1281055 > unrolled thread

Re: [RFC PATCH] af_unix: fix entry locking in unix_dgram_recvmsg

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] af_unix: fix entry locking in unix_dgram_recvmsg Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-12-01 18:10 +0100
    Re: [RFC PATCH] af_unix: fix entry locking in unix_dgram_recvmsg David Miller <davem@davemloft.net> - 2015-12-02 19:10 +0100
      Re: [RFC PATCH] af_unix: fix entry locking in unix_dgram_recvmsg Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2015-12-03 22:30 +0100
        Re: [RFC PATCH] af_unix: fix entry locking in unix_dgram_recvmsg Eric Dumazet <eric.dumazet@gmail.com> - 2015-12-03 22:50 +0100
        Re: [RFC PATCH] af_unix: fix entry locking in unix_dgram_recvmsg David Miller <davem@davemloft.net> - 2015-12-04 00:10 +0100

#1281055 — Re: [RFC PATCH] af_unix: fix entry locking in unix_dgram_recvmsg

Rainer Weikusat <rw@doppelsaurus.mobileactivedefense.com> writes:

[...]

> Insofar I understand the comment in this code block correctly,
>
>         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;
>         }
>
> setting a receive timeout for an AF_UNIX datagram socket also doesn't
> work as intended because of this: In case of n readers with the same
> timeout, the nth reader will end up blocking n times the timeout.

Test program which confirms this. It starts four concurrent reads on the
same socket with a receive timeout of 3s. This means the whole program
should take a little more than 3s to execute as each read should time
out at about the same time. But it takes 12s instead as the reads
pile up on the readlock mutex and each then gets its own timeout once it
could enter the receive loop.

-------
#include <stdio.h>
#include <sys/signal.h>
#include <sys/socket.h>
#include <sys/time.h>
#include <sys/un.h>
#include <sys/wait.h>
#include <unistd.h>

#define SERVER_ADDR	"\0multi-timeout"
#define RCV_TIMEO	3

static void set_rcv_timeo(int sk)
{
    struct timeval tv;

    tv.tv_sec = RCV_TIMEO;
    tv.tv_usec = 0;
    setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
}

int main(void)
{
    struct sockaddr_un sun;
    struct timeval tv_start, tv_end;
    int sk, dummy;
    
    sun.sun_family = AF_UNIX;
    memcpy(sun.sun_path, SERVER_ADDR, sizeof(SERVER_ADDR));
    sk = socket(AF_UNIX, SOCK_DGRAM, 0);
    bind(sk, (struct sockaddr *)&sun, sizeof(sun));
    set_rcv_timeo(sk);

    gettimeofday(&tv_start, NULL);
    
    if (fork() == 0) {
	read(sk, &dummy, sizeof(dummy));
	_exit(0);
    }
    
    if (fork() == 0) {
	read(sk, &dummy, sizeof(dummy));
	_exit(0);
    }
    
    if (fork() == 0) {
	read(sk, &dummy, sizeof(dummy));
	_exit(0);
    }

    read(sk, &dummy, sizeof(dummy));
    
    while (waitpid(-1, NULL, 0) > 0);

    gettimeofday(&tv_end, NULL);
    printf("Waited for %u timeouts\n",
	   (unsigned)((tv_end.tv_sec - tv_start.tv_sec) / RCV_TIMEO));
    
    return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1282242

From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
Date: Tue, 01 Dec 2015 17:02:33 +0000

> Rainer Weikusat <rw@doppelsaurus.mobileactivedefense.com> writes:
> 
> [...]
> 
>> Insofar I understand the comment in this code block correctly,
>>
>>         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;
>>         }
>>
>> setting a receive timeout for an AF_UNIX datagram socket also doesn't
>> work as intended because of this: In case of n readers with the same
>> timeout, the nth reader will end up blocking n times the timeout.
> 
> Test program which confirms this. It starts four concurrent reads on the
> same socket with a receive timeout of 3s. This means the whole program
> should take a little more than 3s to execute as each read should time
> out at about the same time. But it takes 12s instead as the reads
> pile up on the readlock mutex and each then gets its own timeout once it
> could enter the receive loop.

I'm fine with your changes.

So with your patch, the "N * timeout" behavior, where N is the number
of queues reading threads, no longer occurs?  Do they all now properly
get released at the appropriate timeout?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1283400

David Miller <davem@davemloft.net> writes:
> From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
>> Rainer Weikusat <rw@doppelsaurus.mobileactivedefense.com> writes:
>> 
>> [...]
>> 
>>> Insofar I understand the comment in this code block correctly,

[...]

>>>                 /* recvmsg() in non blocking mode is supposed to return -EAGAIN
>>>                  * sk_rcvtimeo is not honored by mutex_lock_interruptible()
>>>
>>> setting a receive timeout for an AF_UNIX datagram socket also doesn't
>>> work as intended because of this: In case of n readers with the same
>>> timeout, the nth reader will end up blocking n times the timeout.

[...]

> So with your patch, the "N * timeout" behavior, where N is the number
> of queues reading threads, no longer occurs?  Do they all now properly
> get released at the appropriate timeout?

As far as I can tell, yes. With the change, unix_dgram_recvmsg has a
read loop looking like this:

	last = NULL; /* not really necessary */
	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);

	do {
		mutex_lock(&u->readlock);

		skip = sk_peek_offset(sk, flags);
		skb = __skb_try_recv_datagram(sk, flags, &peeked, &skip, &err,
					      &last);
		if (skb)
			break;

		mutex_unlock(&u->readlock);

		if (err != -EAGAIN)
			break;
	} while (timeo &&
		 !__skb_wait_for_more_packets(sk, &err, &timeo, last));

u->readlock is only used to enforce serialized access while running code
dealing with the peek offset. If there's currently nothing to receive,
the mutex is dropped. Afterwards, non-blocking readers return with
-EAGAIN and blocking readers go to sleep waiting for 'interesting
events' via __skb_wait_for_more_packets without stuffing the mutex into
a pocket and taking it with them: All non-blocking readers of a certain
socket end up going to sleep via schedule_timeout call in the wait
function, hence, each of them will be woken up once its timeout expires.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1283409

On Thu, 2015-12-03 at 21:24 +0000, Rainer Weikusat wrote:

> As far as I can tell, yes. With the change, unix_dgram_recvmsg has a
> read loop looking like this:
> 
> 	last = NULL; /* not really necessary */


I am not sure SO_RCVTIMEO is really used for af_unix, given its poor
reaction to syscall restarts (ERESTARTSYS)

Do you really know applications relying on it ?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1283440

From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
Date: Thu, 03 Dec 2015 21:24:17 +0000

> David Miller <davem@davemloft.net> writes:
>> So with your patch, the "N * timeout" behavior, where N is the number
>> of queues reading threads, no longer occurs?  Do they all now properly
>> get released at the appropriate timeout?
> 
> As far as I can tell, yes. With the change, unix_dgram_recvmsg has a
> read loop looking like this:
> 
> 	last = NULL; /* not really necessary */
> 	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> 
> 	do {
> 		mutex_lock(&u->readlock);
> 
> 		skip = sk_peek_offset(sk, flags);
> 		skb = __skb_try_recv_datagram(sk, flags, &peeked, &skip, &err,
> 					      &last);
> 		if (skb)
> 			break;
> 
> 		mutex_unlock(&u->readlock);
> 
> 		if (err != -EAGAIN)
> 			break;
> 	} while (timeo &&
> 		 !__skb_wait_for_more_packets(sk, &err, &timeo, last));
> 
> u->readlock is only used to enforce serialized access while running code
> dealing with the peek offset. If there's currently nothing to receive,
> the mutex is dropped. Afterwards, non-blocking readers return with
> -EAGAIN and blocking readers go to sleep waiting for 'interesting
> events' via __skb_wait_for_more_packets without stuffing the mutex into
> a pocket and taking it with them: All non-blocking readers of a certain
> socket end up going to sleep via schedule_timeout call in the wait
> function, hence, each of them will be woken up once its timeout expires.

Great, thanks for the info.  I think you should submit this patch
formally.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web