Re: [PATCH net v5 2/2] net: restore refcount ordering for lockless socket access

[Paolo Abeni <pabeni@redhat.com>]

On 11/2/26 23:06, netdev-bot+sashiko@kernel.org wrote:
>> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
>> index b0239d203f045..860145d810ed4 100644
>> --- a/net/ipv4/inet_hashtables.c
>> +++ b/net/ipv4/inet_hashtables.c
>> @@ -550,7 +550,7 @@ struct sock *__inet_lookup_established(const struct net *net,
>>   		if (sk->sk_hash != hash)
>>   			continue;
>>   		if (likely(inet_match(net, sk, acookie, ports, dif, sdif))) {
>> -			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
>> +			if (unlikely(!refcount_inc_not_zero_acquire(&sk->sk_refcnt)))
>>   				goto out;
>>   			if (unlikely(hlist_nulls_unhashed_lockless(&sk->sk_nulls_node) ||
>>   				     !inet_match(net, sk, acookie, ports, dif,
> 
> [Severity: High]
> The commit message says the change targets lookups "where the socket is
> validated again after taking a reference". Should the MPTCP token lookups
> be converted too? They follow the same pattern but still use plain
> refcount_inc_not_zero():
> 
> net/mptcp/token.c:mptcp_token_get_sock() {
>      ...
> 		if (!refcount_inc_not_zero(&sk->sk_refcnt))
> 			goto not_found;
> 
> 		if (READ_ONCE(msk->token) != token ||
> 		    !net_eq(sock_net(sk), net)) {
> 			sock_put(sk);
> 			goto again;
> 		}
>      ...
> }
> 
> mptcp_token_iter_next() does the same thing: it increments and then checks
> net_eq() again.
> 
> mptcp_prot uses SLAB_TYPESAFE_BY_RCU, and mptcp_sk_clone_init() clears
> SOCK_RCU_FREE on established msks:
> 
>      sock_reset_flag(nsk, SOCK_RCU_FREE);
> 
> So an msk can be freed and its slot reused while an RCU reader still holds
> the address.
> 
> On weakly ordered CPUs, can the token and sk_net loads in the re-check run
> before the increment? If so, they could see the old object's values, and
> the lookup would return a referenced msk for a different connection or
> netns.
> 
> Callers include the userspace PM netlink operations that look up by
> token, MP_JOIN handling in subflow.c and syncookies.c, and the diag and PM
> iterators. HMAC validation would probably reject most of the MP_JOIN
> cases.
> 
> mptcp_subflow_reqsk_clone() already uses refcount_inc_not_zero_acquire()
> on the same msk objects.
Even this part is better handled with a separate patch.

/P