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

From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: <pabeni@redhat.com>
Cc: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
	<kuni1840@gmail.com>, <kuniyu@amazon.com>,
	<netdev@vger.kernel.org>, <rweikusat@mobileactivedefense.com>
Subject: Re: [PATCH net] af_unix: Fix a data-race in unix_dgram_peer_wake_me().
Date: Tue, 7 Jun 2022 07:14:53 -0700	[thread overview]
Message-ID: <20220607141453.48225-1-kuniyu@amazon.com> (raw)
In-Reply-To: <67666593dced5eca946ac1639f214133191ebd39.camel@redhat.com>

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!