[PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().

[PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().

[Kuniyuki Iwashima]

unix_dgram_poll() calls unix_dgram_peer_wake_me() without `other`'s
lock held and check if its receive queue is full.  Here we need to
use unix_recvq_full_lockless() instead of unix_recvq_full(), otherwise
KCSAN will report a data-race.

Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
As Eric noted in commit 04f08eb44b501, I think rest of unix_recvq_full()
can be turned into the lockless version.  After this merge window, I can
send a follow-up patch if there is no objection.
---
 net/unix/af_unix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 654dcef7cfb3..2206e6f8902d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -490,7 +490,7 @@ static int unix_dgram_peer_wake_me(struct sock *sk, struct sock *other)
 	 * -ECONNREFUSED. Otherwise, if we haven't queued any skbs
 	 * to other and its full, we will hang waiting for POLLOUT.
 	 */
-	if (unix_recvq_full(other) && !sock_flag(other, SOCK_DEAD))
+	if (unix_recvq_full_lockless(other) && !sock_flag(other, SOCK_DEAD))
 		return 1;
 
 	if (connected)
-- 
2.30.2

Re: [PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().

[Paolo Abeni]

On Sun, 2022-06-05 at 16:23 -0700, Kuniyuki Iwashima wrote:
> unix_dgram_poll() calls unix_dgram_peer_wake_me() without `other`'s
> lock held and check if its receive queue is full.  Here we need to
> use unix_recvq_full_lockless() instead of unix_recvq_full(), otherwise
> KCSAN will report a data-race.
> 
> Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> As Eric noted in commit 04f08eb44b501, I think rest of unix_recvq_full()
> can be turned into the lockless version.  After this merge window, I can
> send a follow-up patch if there is no objection.

It looks like replacing the remaining instances of unix_recvq_full()
with unix_recvq_full_lockless() should be safe, but I'm wondering if
doing that while retaining the current state lock scope it's worthy?!? 

It may trick later readers of the relevant code to think that such code
may be reached without a lock. Or are you suggesting to additionally
shrink the state lock scope? that latter part looks much more tricky,
IMHO.

Cheers,

Paolo

Re: [PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Sun, 5 Jun 2022 16:23:25 -0700 you wrote:
> unix_dgram_poll() calls unix_dgram_peer_wake_me() without `other`'s
> lock held and check if its receive queue is full.  Here we need to
> use unix_recvq_full_lockless() instead of unix_recvq_full(), otherwise
> KCSAN will report a data-race.
> 
> Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> 
> [...]

Here is the summary with links:
  - [net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().
    https://git.kernel.org/netdev/net/c/662a80946ce1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().

[Kuniyuki Iwashima]

From:   Paolo Abeni <pabeni@redhat.com>
Date:   Tue, 07 Jun 2022 12:35:13 +0200
> On Sun, 2022-06-05 at 16:23 -0700, Kuniyuki Iwashima wrote:
>> unix_dgram_poll() calls unix_dgram_peer_wake_me() without `other`'s
>> lock held and check if its receive queue is full.  Here we need to
>> use unix_recvq_full_lockless() instead of unix_recvq_full(), otherwise
>> KCSAN will report a data-race.
>> 
>> Fixes: 7d267278a9ec ("unix: avoid use-after-free in ep_remove_wait_queue")
>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
>> ---
>> As Eric noted in commit 04f08eb44b501, I think rest of unix_recvq_full()
>> can be turned into the lockless version.  After this merge window, I can
>> send a follow-up patch if there is no objection.
> 
> It looks like replacing the remaining instances of unix_recvq_full()
> with unix_recvq_full_lockless() should be safe, but I'm wondering if
> doing that while retaining the current state lock scope it's worthy?!? 
> 
> It may trick later readers of the relevant code to think that such code
> may be reached without a lock. Or are you suggesting to additionally
> shrink the state lock scope? that latter part looks much more tricky,
> IMHO.

I thought removing unix_recvq_full() will prevent the same mistakes, but
I agree that it is confusing for later readers.

Thank you!