[PATCH net] af_unix: Support POLLPRI for OOB.

[PATCH net] af_unix: Support POLLPRI for OOB.

[Kuniyuki Iwashima]

The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
piece.

In the selftest, normal datagrams are sent followed by OOB data, so this
commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first test
case.

Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 net/unix/af_unix.c                                  | 2 ++
 tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index c19569819866..711d21b1c3e1 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, struct socket *sock, poll_table *wa
 		mask |= EPOLLIN | EPOLLRDNORM;
 	if (sk_is_readable(sk))
 		mask |= EPOLLIN | EPOLLRDNORM;
+	if (unix_sk(sk)->oob_skb)
+		mask |= EPOLLPRI;
 
 	/* Connection-based need to check for termination and startup */
 	if ((sk->sk_type == SOCK_STREAM || sk->sk_type == SOCK_SEQPACKET) &&
diff --git a/tools/testing/selftests/net/af_unix/test_unix_oob.c b/tools/testing/selftests/net/af_unix/test_unix_oob.c
index 3dece8b29253..b57e91e1c3f2 100644
--- a/tools/testing/selftests/net/af_unix/test_unix_oob.c
+++ b/tools/testing/selftests/net/af_unix/test_unix_oob.c
@@ -218,10 +218,10 @@ main(int argc, char **argv)
 
 	/* Test 1:
 	 * veriyf that SIGURG is
-	 * delivered and 63 bytes are
-	 * read and oob is '@'
+	 * delivered, 63 bytes are
+	 * read, oob is '@', and POLLPRI works.
 	 */
-	wait_for_data(pfd, POLLIN | POLLPRI);
+	wait_for_data(pfd, POLLPRI);
 	read_oob(pfd, &oob);
 	len = read_data(pfd, buf, 1024);
 	if (!signal_recvd || len != 63 || oob != '@') {
-- 
2.30.2

Re: [PATCH net] af_unix: Support POLLPRI for OOB.

[Shoaib Rao]

On 3/13/22 22:21, Kuniyuki Iwashima wrote:
> The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
> AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
> piece.
>
> In the selftest, normal datagrams are sent followed by OOB data, so this
> commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first test
> case.
>
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> ---
>   net/unix/af_unix.c                                  | 2 ++
>   tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index c19569819866..711d21b1c3e1 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, struct socket *sock, poll_table *wa
>   		mask |= EPOLLIN | EPOLLRDNORM;
>   	if (sk_is_readable(sk))
>   		mask |= EPOLLIN | EPOLLRDNORM;
> +	if (unix_sk(sk)->oob_skb)
> +		mask |= EPOLLPRI;
>   
>   	/* Connection-based need to check for termination and startup */
>   	if ((sk->sk_type == SOCK_STREAM || sk->sk_type == SOCK_SEQPACKET) &&
> diff --git a/tools/testing/selftests/net/af_unix/test_unix_oob.c b/tools/testing/selftests/net/af_unix/test_unix_oob.c
> index 3dece8b29253..b57e91e1c3f2 100644
> --- a/tools/testing/selftests/net/af_unix/test_unix_oob.c
> +++ b/tools/testing/selftests/net/af_unix/test_unix_oob.c
> @@ -218,10 +218,10 @@ main(int argc, char **argv)
>   
>   	/* Test 1:
>   	 * veriyf that SIGURG is
> -	 * delivered and 63 bytes are
> -	 * read and oob is '@'
> +	 * delivered, 63 bytes are
> +	 * read, oob is '@', and POLLPRI works.
>   	 */
> -	wait_for_data(pfd, POLLIN | POLLPRI);
> +	wait_for_data(pfd, POLLPRI);
>   	read_oob(pfd, &oob);
>   	len = read_data(pfd, buf, 1024);
>   	if (!signal_recvd || len != 63 || oob != '@') {

Reviewed-by: Rao Shoaib <rao.shoaib@oracle.com>

Thanks for fixing this.

Shoaib

Re: [PATCH net] af_unix: Support POLLPRI for OOB.

[Eric Dumazet]

On 3/13/22 22:21, Kuniyuki Iwashima wrote:
> The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
> AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
> piece.
>
> In the selftest, normal datagrams are sent followed by OOB data, so this
> commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first test
> case.
>
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> ---
>   net/unix/af_unix.c                                  | 2 ++
>   tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index c19569819866..711d21b1c3e1 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, struct socket *sock, poll_table *wa
>   		mask |= EPOLLIN | EPOLLRDNORM;
>   	if (sk_is_readable(sk))
>   		mask |= EPOLLIN | EPOLLRDNORM;
> +	if (unix_sk(sk)->oob_skb)
> +		mask |= EPOLLPRI;


This adds another data-race, maybe add something to avoid another syzbot 
report ?


>   
>   	/* Connection-based need to check for termination and startup */
>   	if ((sk->sk_type == SOCK_STREAM || sk->sk_type == SOCK_SEQPACKET) &&
> diff --git a/tools/testing/selftests/net/af_unix/test_unix_oob.c b/tools/testing/selftests/net/af_unix/test_unix_oob.c
> index 3dece8b29253..b57e91e1c3f2 100644
> --- a/tools/testing/selftests/net/af_unix/test_unix_oob.c
> +++ b/tools/testing/selftests/net/af_unix/test_unix_oob.c
> @@ -218,10 +218,10 @@ main(int argc, char **argv)
>   
>   	/* Test 1:
>   	 * veriyf that SIGURG is
> -	 * delivered and 63 bytes are
> -	 * read and oob is '@'
> +	 * delivered, 63 bytes are
> +	 * read, oob is '@', and POLLPRI works.
>   	 */
> -	wait_for_data(pfd, POLLIN | POLLPRI);
> +	wait_for_data(pfd, POLLPRI);
>   	read_oob(pfd, &oob);
>   	len = read_data(pfd, buf, 1024);
>   	if (!signal_recvd || len != 63 || oob != '@') {

Re: [PATCH net] af_unix: Support POLLPRI for OOB.

[Shoaib Rao]

On 3/14/22 10:42, Eric Dumazet wrote:
>
> On 3/13/22 22:21, Kuniyuki Iwashima wrote:
>> The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
>> AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
>> piece.
>>
>> In the selftest, normal datagrams are sent followed by OOB data, so this
>> commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first test
>> case.
>>
>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
>> ---
>>   net/unix/af_unix.c                                  | 2 ++
>>   tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index c19569819866..711d21b1c3e1 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, 
>> struct socket *sock, poll_table *wa
>>           mask |= EPOLLIN | EPOLLRDNORM;
>>       if (sk_is_readable(sk))
>>           mask |= EPOLLIN | EPOLLRDNORM;
>> +    if (unix_sk(sk)->oob_skb)
>> +        mask |= EPOLLPRI;
>
>
> This adds another data-race, maybe add something to avoid another 
> syzbot report ?

It's not obvious to me how there would be a race as it is just a check.

Also this change should be under #if IS_ENABLED(CONFIG_AF_UNIX_OOB)

Thanks,

Shoaib

>
>
>>         /* Connection-based need to check for termination and startup */
>>       if ((sk->sk_type == SOCK_STREAM || sk->sk_type == 
>> SOCK_SEQPACKET) &&
>> diff --git a/tools/testing/selftests/net/af_unix/test_unix_oob.c 
>> b/tools/testing/selftests/net/af_unix/test_unix_oob.c
>> index 3dece8b29253..b57e91e1c3f2 100644
>> --- a/tools/testing/selftests/net/af_unix/test_unix_oob.c
>> +++ b/tools/testing/selftests/net/af_unix/test_unix_oob.c
>> @@ -218,10 +218,10 @@ main(int argc, char **argv)
>>         /* Test 1:
>>        * veriyf that SIGURG is
>> -     * delivered and 63 bytes are
>> -     * read and oob is '@'
>> +     * delivered, 63 bytes are
>> +     * read, oob is '@', and POLLPRI works.
>>        */
>> -    wait_for_data(pfd, POLLIN | POLLPRI);
>> +    wait_for_data(pfd, POLLPRI);
>>       read_oob(pfd, &oob);
>>       len = read_data(pfd, buf, 1024);
>>       if (!signal_recvd || len != 63 || oob != '@') {

Re: [PATCH net] af_unix: Support POLLPRI for OOB.

[Eric Dumazet]

On 3/14/22 11:10, Shoaib Rao wrote:
>
> On 3/14/22 10:42, Eric Dumazet wrote:
>>
>> On 3/13/22 22:21, Kuniyuki Iwashima wrote:
>>> The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
>>> AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
>>> piece.
>>>
>>> In the selftest, normal datagrams are sent followed by OOB data, so 
>>> this
>>> commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first 
>>> test
>>> case.
>>>
>>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
>>> ---
>>>   net/unix/af_unix.c                                  | 2 ++
>>>   tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>> index c19569819866..711d21b1c3e1 100644
>>> --- a/net/unix/af_unix.c
>>> +++ b/net/unix/af_unix.c
>>> @@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, 
>>> struct socket *sock, poll_table *wa
>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>       if (sk_is_readable(sk))
>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>> +    if (unix_sk(sk)->oob_skb)
>>> +        mask |= EPOLLPRI;
>>
>>
>> This adds another data-race, maybe add something to avoid another 
>> syzbot report ?
>
> It's not obvious to me how there would be a race as it is just a check.
>

KCSAN will detect that whenever unix_poll() reads oob_skb,

its value can be changed by another cpu.


unix_poll() runs without holding a lock.



> Also this change should be under #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>
> Thanks,
>
> Shoaib
>
>>
>>
>>>         /* Connection-based need to check for termination and 
>>> startup */
>>>       if ((sk->sk_type == SOCK_STREAM || sk->sk_type == 
>>> SOCK_SEQPACKET) &&
>>> diff --git a/tools/testing/selftests/net/af_unix/test_unix_oob.c 
>>> b/tools/testing/selftests/net/af_unix/test_unix_oob.c
>>> index 3dece8b29253..b57e91e1c3f2 100644
>>> --- a/tools/testing/selftests/net/af_unix/test_unix_oob.c
>>> +++ b/tools/testing/selftests/net/af_unix/test_unix_oob.c
>>> @@ -218,10 +218,10 @@ main(int argc, char **argv)
>>>         /* Test 1:
>>>        * veriyf that SIGURG is
>>> -     * delivered and 63 bytes are
>>> -     * read and oob is '@'
>>> +     * delivered, 63 bytes are
>>> +     * read, oob is '@', and POLLPRI works.
>>>        */
>>> -    wait_for_data(pfd, POLLIN | POLLPRI);
>>> +    wait_for_data(pfd, POLLPRI);
>>>       read_oob(pfd, &oob);
>>>       len = read_data(pfd, buf, 1024);
>>>       if (!signal_recvd || len != 63 || oob != '@') {

Re: [PATCH net] af_unix: Support POLLPRI for OOB.

[Kuniyuki Iwashima]

From:   Eric Dumazet <eric.dumazet@gmail.com>
Date:   Mon, 14 Mar 2022 17:26:54 -0700
> On 3/14/22 11:10, Shoaib Rao wrote:
>>
>> On 3/14/22 10:42, Eric Dumazet wrote:
>>>
>>> On 3/13/22 22:21, Kuniyuki Iwashima wrote:
>>>> The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
>>>> AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
>>>> piece.
>>>>
>>>> In the selftest, normal datagrams are sent followed by OOB data, so 
>>>> this
>>>> commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first 
>>>> test
>>>> case.
>>>>
>>>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>>>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
>>>> ---
>>>>   net/unix/af_unix.c                                  | 2 ++
>>>>   tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
>>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>>> index c19569819866..711d21b1c3e1 100644
>>>> --- a/net/unix/af_unix.c
>>>> +++ b/net/unix/af_unix.c
>>>> @@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, 
>>>> struct socket *sock, poll_table *wa
>>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>>       if (sk_is_readable(sk))
>>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>> +    if (unix_sk(sk)->oob_skb)
>>>> +        mask |= EPOLLPRI;
>>>
>>>
>>> This adds another data-race, maybe add something to avoid another 
>>> syzbot report ?
>>
>> It's not obvious to me how there would be a race as it is just a check.
>>
> 
> KCSAN will detect that whenever unix_poll() reads oob_skb,
> 
> its value can be changed by another cpu.
> 
> 
> unix_poll() runs without holding a lock.

Thanks for pointing out!
So, READ_ONCE() is necessary here, right?
oob_skb is written under the lock, so I think there is no paired
WRITE_ONCE(), but is it ok?


> 
> 
> 
>> Also this change should be under #if IS_ENABLED(CONFIG_AF_UNIX_OOB)

And thanks, Shoaib!
Will add the condition in v2.


>>
>> Thanks,
>>
>> Shoaib
>>
>>>
>>>
>>>>         /* Connection-based need to check for termination and 
>>>> startup */
>>>>       if ((sk->sk_type == SOCK_STREAM || sk->sk_type == 
>>>> SOCK_SEQPACKET) &&
>>>> diff --git a/tools/testing/selftests/net/af_unix/test_unix_oob.c 
>>>> b/tools/testing/selftests/net/af_unix/test_unix_oob.c
>>>> index 3dece8b29253..b57e91e1c3f2 100644
>>>> --- a/tools/testing/selftests/net/af_unix/test_unix_oob.c
>>>> +++ b/tools/testing/selftests/net/af_unix/test_unix_oob.c
>>>> @@ -218,10 +218,10 @@ main(int argc, char **argv)
>>>>         /* Test 1:
>>>>        * veriyf that SIGURG is
>>>> -     * delivered and 63 bytes are
>>>> -     * read and oob is '@'
>>>> +     * delivered, 63 bytes are
>>>> +     * read, oob is '@', and POLLPRI works.
>>>>        */
>>>> -    wait_for_data(pfd, POLLIN | POLLPRI);
>>>> +    wait_for_data(pfd, POLLPRI);
>>>>       read_oob(pfd, &oob);
>>>>       len = read_data(pfd, buf, 1024);
>>>>       if (!signal_recvd || len != 63 || oob != '@') {

Re: [PATCH net] af_unix: Support POLLPRI for OOB.

[Kuniyuki Iwashima]

From:   Kuniyuki Iwashima <kuniyu@amazon.co.jp>
Date:   Tue, 15 Mar 2022 09:45:03 +0900
> From:   Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Mon, 14 Mar 2022 17:26:54 -0700
>> On 3/14/22 11:10, Shoaib Rao wrote:
>>>
>>> On 3/14/22 10:42, Eric Dumazet wrote:
>>>>
>>>> On 3/13/22 22:21, Kuniyuki Iwashima wrote:
>>>>> The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
>>>>> AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
>>>>> piece.
>>>>>
>>>>> In the selftest, normal datagrams are sent followed by OOB data, so 
>>>>> this
>>>>> commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first 
>>>>> test
>>>>> case.
>>>>>
>>>>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>>>>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
>>>>> ---
>>>>>   net/unix/af_unix.c                                  | 2 ++
>>>>>   tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
>>>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>>>> index c19569819866..711d21b1c3e1 100644
>>>>> --- a/net/unix/af_unix.c
>>>>> +++ b/net/unix/af_unix.c
>>>>> @@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, 
>>>>> struct socket *sock, poll_table *wa
>>>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>>>       if (sk_is_readable(sk))
>>>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>>> +    if (unix_sk(sk)->oob_skb)
>>>>> +        mask |= EPOLLPRI;
>>>>
>>>>
>>>> This adds another data-race, maybe add something to avoid another 
>>>> syzbot report ?
>>>
>>> It's not obvious to me how there would be a race as it is just a check.
>>>
>> 
>> KCSAN will detect that whenever unix_poll() reads oob_skb,
>> 
>> its value can be changed by another cpu.
>> 
>> 
>> unix_poll() runs without holding a lock.
> 
> Thanks for pointing out!
> So, READ_ONCE() is necessary here, right?
> oob_skb is written under the lock, so I think there is no paired
> WRITE_ONCE(), but is it ok?

I've tested the prog below and KCSAN repoted the race.
Also, READ_ONCE() suppressed it.

Thank you Eric!
I'll post v2 with READ_ONCE().

---8<---
[   60.021825] ==================================================================
[   60.021999] BUG: KCSAN: data-race in unix_poll / unix_stream_sendmsg
[   60.021999] 
[   60.021999] write to 0xffff8880050d9ff0 of 8 bytes by task 175 on cpu 3:
[   60.021999]  unix_stream_sendmsg+0x9dc/0xbb0
[   60.021999]  sock_sendmsg+0x90/0xa0
[   60.021999]  __sys_sendto+0x138/0x190
[   60.021999]  __x64_sys_sendto+0x6d/0x80
[   60.021999]  do_syscall_64+0x38/0x80
[   60.021999]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   60.021999] 
[   60.021999] read to 0xffff8880050d9ff0 of 8 bytes by task 176 on cpu 2:
[   60.021999]  unix_poll+0xf4/0x1b0
[   60.021999]  sock_poll+0xa4/0x1e0
[   60.021999]  do_sys_poll+0x326/0x750
[   60.021999]  __x64_sys_poll+0x55/0x1f0
[   60.021999]  do_syscall_64+0x38/0x80
[   60.021999]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   60.021999] 
[   60.021999] value changed: 0xffff8880056bf000 -> 0xffff8880056bff00
[   60.021999] 
[   60.021999] Reported by Kernel Concurrency Sanitizer on:
[   60.021999] CPU: 2 PID: 176 Comm: unix_race_oob Not tainted 5.17.0-rc5-59531-gbdaabdfaadf8-dirty #9
[   60.021999] Hardware name: Red Hat KVM, BIOS 1.11.0-2.amzn2 04/01/2014
[   60.021999] ==================================================================
---8<---

---8<---
#include <errno.h>
#include <poll.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/un.h>
#include <sys/wait.h>

#define offsetof(type, member) ((size_t)&((type *)0)->member)
#define SUNADDR "\0test\0"
#define ADDRLEN (socklen_t)(offsetof(struct sockaddr_un, sun_path) + 6)

int setup_server(void)
{
	struct sockaddr_un addr = {
		.sun_family = AF_UNIX,
		.sun_path = SUNADDR,
	};
	int err, fd;

	fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
	if (fd == -1) {
		perror("socket");
		goto out;
	}

	err = bind(fd, (struct sockaddr *)&addr, ADDRLEN);
	if (err == -1) {
		perror("bind");
		goto err;
	}

	err = listen(fd, 32);
	if (err == -1) {
		perror("listen");
		goto err;
	}

out:
	return fd;

err:
	close(fd);
	return err;
}

int setup_client(void)
{
	struct sockaddr_un addr = {
		.sun_family = AF_UNIX,
		.sun_path = SUNADDR,
	};
	int err, fd;

	fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
	if (fd == -1) {
		perror("socket");
		goto out;
	}

	err = connect(fd, (struct sockaddr *)&addr, ADDRLEN);
	if (err == -1) {
		perror("connect");
		goto err;
	}

out:
	return fd;

err:
	close(fd);
	return err;
}

int setup_child(int server)
{
	struct sockaddr_un addr;
	socklen_t len;
	int child;

	child = accept(server, (struct sockaddr *)&addr, &len);
	if (child == -1)
		perror("accept");

	return child;
}

int sender(int client)
{
	char c = 'a';
	int ret;

	printf("start sender\n");

	while (1) {
		ret = send(client, &c, sizeof(c), MSG_OOB);
		if (ret != 1 || errno)
			perror("send");
	}

	return 0;
}


int receiver(int child)
{
	struct pollfd pfds[1];
	char buf[1024];
	int ret;

	pfds[0].fd = child;
	pfds[0].events = POLLIN | POLLPRI;

	printf("start receiver\n");

	while (1) {
		poll(pfds, 1, -1);

		ret = recv(child, buf, sizeof(buf), MSG_OOB);
		if (ret < 0 || errno)
			perror("recv (MSG_OOB)");

		ret = recv(child, buf, sizeof(buf), 0);
		if (ret < 0 || errno)
			perror("recv");
	}

	return 0;
}

int main(void)
{
	int server, client, child;
	int status;
	pid_t pid;

	server = setup_server();
	if (server == -1)
		goto out;

	client = setup_client();
	if (client == -1)
		goto close_server;

	child = setup_child(server);
	if (child == -1)
		goto close_client;

	pid = fork();
	if (pid == 0)
		return sender(client);

	pid = fork();
	if (pid == 0)
		return receiver(child);

	wait(&status);

	close(child);
close_client:
	close(client);
close_server:
	close(server);
out:
	return 0;
}
---8<---

Re: [PATCH net] af_unix: Support POLLPRI for OOB.

[Kuniyuki Iwashima]

From:   Kuniyuki Iwashima <kuniyu@amazon.co.jp>
Date:   Tue, 15 Mar 2022 14:30:40 +0900
> From:   Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> Date:   Tue, 15 Mar 2022 09:45:03 +0900
>> From:   Eric Dumazet <eric.dumazet@gmail.com>
>> Date:   Mon, 14 Mar 2022 17:26:54 -0700
>>> On 3/14/22 11:10, Shoaib Rao wrote:
>>>>
>>>> On 3/14/22 10:42, Eric Dumazet wrote:
>>>>>
>>>>> On 3/13/22 22:21, Kuniyuki Iwashima wrote:
>>>>>> The commit 314001f0bf92 ("af_unix: Add OOB support") introduced OOB for
>>>>>> AF_UNIX, but it lacks some changes for POLLPRI.  Let's add the missing
>>>>>> piece.
>>>>>>
>>>>>> In the selftest, normal datagrams are sent followed by OOB data, so 
>>>>>> this
>>>>>> commit replaces `POLLIN | POLLPRI` with just `POLLPRI` in the first 
>>>>>> test
>>>>>> case.
>>>>>>
>>>>>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>>>>>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
>>>>>> ---
>>>>>>   net/unix/af_unix.c                                  | 2 ++
>>>>>>   tools/testing/selftests/net/af_unix/test_unix_oob.c | 6 +++---
>>>>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>>>>> index c19569819866..711d21b1c3e1 100644
>>>>>> --- a/net/unix/af_unix.c
>>>>>> +++ b/net/unix/af_unix.c
>>>>>> @@ -3139,6 +3139,8 @@ static __poll_t unix_poll(struct file *file, 
>>>>>> struct socket *sock, poll_table *wa
>>>>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>>>>       if (sk_is_readable(sk))
>>>>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>>>> +    if (unix_sk(sk)->oob_skb)
>>>>>> +        mask |= EPOLLPRI;
>>>>>
>>>>>
>>>>> This adds another data-race, maybe add something to avoid another 
>>>>> syzbot report ?
>>>>
>>>> It's not obvious to me how there would be a race as it is just a check.
>>>>
>>> 
>>> KCSAN will detect that whenever unix_poll() reads oob_skb,
>>> 
>>> its value can be changed by another cpu.
>>> 
>>> 
>>> unix_poll() runs without holding a lock.
>> 
>> Thanks for pointing out!
>> So, READ_ONCE() is necessary here, right?
>> oob_skb is written under the lock, so I think there is no paired
>> WRITE_ONCE(), but is it ok?

I have misunderstood this, the lock has nothing to do with WRITE_ONCE()
this time.  The write of oob_skb can be teared by GCC, which leads to a
data race at the read side even with READ_ONCE(), so the paired one is
needed.

I will respin v3 with READ_ONCE() and WRITE_ONCE().


> 
> I've tested the prog below and KCSAN repoted the race.
> Also, READ_ONCE() suppressed it.
> 
> Thank you Eric!
> I'll post v2 with READ_ONCE().