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

From: Paolo Abeni <pabeni@redhat.com>
To: Kuniyuki Iwashima <kuniyu@amazon.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Rainer Weikusat <rweikusat@mobileactivedefense.com>
Cc: Kuniyuki Iwashima <kuni1840@gmail.com>, netdev@vger.kernel.org
Subject: Re: [PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().
Date: Tue, 07 Jun 2022 12:35:13 +0200	[thread overview]
Message-ID: <67666593dced5eca946ac1639f214133191ebd39.camel@redhat.com> (raw)
In-Reply-To: <20220605232325.11804-1-kuniyu@amazon.com>

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