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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220315004503.46906-1-kuniyu@amazon.co.jp>
Date:   Tue, 15 Mar 2022 09:45:03 +0900
From:   Kuniyuki Iwashima <kuniyu@...zon.co.jp>
To:     <eric.dumazet@...il.com>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <kuni1840@...il.com>,
        <kuniyu@...zon.co.jp>, <netdev@...r.kernel.org>,
        <rao.shoaib@...cle.com>
Subject: Re: [PATCH net] af_unix: Support POLLPRI for OOB.

From:   Eric Dumazet <eric.dumazet@...il.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@...zon.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 != '@') {