| Message ID | 20220605232325.11804-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 662a80946ce13633ae90a55379f1346c10f0c432 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] af_unix: Fix a data-race in unix_dgram_peer_wake_me(). | expand |
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
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!
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!
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)
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(-)