Re: [PATCH mptcp-next v2 1/4] mptcp: give MPTCP sockets a distinct lockdep class

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: race on sk_lock reinit and unlocked clone with SLAB_TYPESAFE_BY_RCU
--

commit 1535a4c9bef6857111a0222f084ae0666d8e7461
Author: Geliang Tang <geliang@kernel.org>

mptcp: give MPTCP sockets a distinct lockdep class

This commit introduces mptcp_reclassify_socket() to assign distinct lockdep
classes to MPTCP sockets based on address family and kernel vs user socket
type. It calls this helper from mptcp_init_sock() and from
mptcp_sk_clone_init() after switching from sk_clone_lock() to unlocked
sk_clone().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..d4f42dfc227e2 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3267,11 +3267,43 @@ static void mptcp_ca_reset(struct sock *sk)
>  	icsk->icsk_ca_ops = NULL;
>  }
>  
> +/* The subflow socket lock is acquired in a nested to the msk one
> + * in several places, even by the TCP stack: lockdep complains when
> + * both are kernel sockets. Instead of propagating the _nested modifiers
> + * in several places, re-init the lock class for the msk socket to an
> + * mptcp specific one. Separate lock classes are used for kernel and
> + * user sockets to prevent lockdep from conflating their usage patterns.
> + */
> +static struct lock_class_key mptcp_slock_keys[2][2];
> +static struct lock_class_key mptcp_keys[2][2];
> +
> +static void mptcp_reclassify_socket(struct sock *sk)
> +{
> +	bool is_ipv6 = sk->sk_family == AF_INET6;
> +	const char *slock_name, *lock_name;
> +	bool is_kern = !!sk->sk_kern_sock;
> +
> +	if (is_kern) {
> +		slock_name = is_ipv6 ? "kmlock-AF_INET6" : "kmlock-AF_INET";
> +		lock_name = is_ipv6 ? "kmsk_lock-AF_INET6" :
> +				      "kmsk_lock-AF_INET";
> +	} else {
> +		slock_name = is_ipv6 ? "mlock-AF_INET6" : "mlock-AF_INET";
> +		lock_name = is_ipv6 ? "msk_lock-AF_INET6" : "msk_lock-AF_INET";
> +	}
> +
> +	sock_lock_init_class_and_name(sk, slock_name,
> +				      &mptcp_slock_keys[is_kern][is_ipv6],
> +				      lock_name, &mptcp_keys[is_kern][is_ipv6]);
> +}
> +
>  static int mptcp_init_sock(struct sock *sk)
>  {
>  	struct net *net = sock_net(sk);
>  	int ret;
>  
> +	mptcp_reclassify_socket(sk);
> +
>  	__mptcp_init_sock(sk);
>  
>  	if (!mptcp_is_enabled(net))
> @@ -3772,13 +3804,16 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
>  				 struct request_sock *req)
>  {
>  	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
> -	struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
> +	struct sock *nsk = sk_clone(sk, GFP_ATOMIC, false);
>  	struct mptcp_subflow_context *subflow;
>  	struct mptcp_sock *msk;
>  
>  	if (!nsk)
>  		return NULL;
>  
> +	mptcp_reclassify_socket(nsk);
> +
> +	bh_lock_sock(nsk);

[Severity: High]
Can calling mptcp_reclassify_socket() and unlocked sk_clone() race with
lockless RCU readers on SLAB_TYPESAFE_BY_RCU MPTCP sockets?

Both mptcp_prot and mptcp_v6_prot use SLAB_TYPESAFE_BY_RCU, and accepted
MPTCP sockets clear SOCK_RCU_FREE in mptcp_sk_clone_init(), so a closed
mptcp_sock can be immediately recycled from the slab cache while a
concurrent RCU reader in mptcp_token_iter_next() or mptcp_token_get_sock()
is walking bucket->msk_chain:

net/mptcp/token.c:mptcp_token_iter_next() {
	...
	rcu_read_lock();
	sk_nulls_for_each_rcu(sk, pos, &bucket->msk_chain) {
		++num;
		if (!net_eq(sock_net(sk), net))
			continue;

		if (num <= *s_num)
			continue;

		if (!refcount_inc_not_zero(&sk->sk_refcnt))
			continue;
	...
}

Once refcount_inc_not_zero(&sk->sk_refcnt) succeeds on the recycled socket,
callers of mptcp_token_iter_next() (such as mptcp_diag_dump(),
mptcp_nl_remove_subflow_and_signal_addr(), mptcp_nl_remove_id_zero_address(),
mptcp_nl_flush_addrs_list(), and mptcp_nl_set_flags()) immediately call
lock_sock(sk) or lock_sock_fast(sk).

In mptcp_sk_clone_init(), sk_clone_lock() previously initialized newsk's
lock and acquired bh_lock_sock(newsk) before setting newsk->sk_refcnt to 2:

net/core/sock.c:sk_clone() {
	...
	sock_lock_init(newsk);

	if (lock)
		bh_lock_sock(newsk);
	...
	smp_wmb();
	refcount_set(&newsk->sk_refcnt, 2);
}

Holding bh_lock_sock(newsk) before sk_refcnt became non-zero serialized any
concurrent lock_sock() or lock_sock_fast() reader until __mptcp_init_sock()
completed and bh_unlock_sock(nsk) was called.

With sk_clone(sk, GFP_ATOMIC, false), nsk->sk_refcnt becomes 2 while nsk is
unlocked. Can a concurrent RCU reader grab a reference on nsk and call
lock_sock(nsk) or lock_sock_fast(nsk):

1. Concurrently with mptcp_reclassify_socket(nsk) (and similarly in
   mptcp_init_sock() after sock_init_data() has set sk->sk_refcnt to 1),
   where sock_lock_init_class_and_name() zeroes sk_lock.owned and
   re-initializes sk_lock.wq, sk_lock.slock, and the lockdep maps while
   another CPU is acquiring or holding sk_lock?

2. In mptcp_sk_clone_init(), after mptcp_reclassify_socket(nsk) finishes
   and before bh_lock_sock(nsk) is called? If the reader acquires
   lock_sock(nsk) first, it sets nsk->sk_lock.owned = 1 and drops
   nsk->sk_lock.slock, which does not prevent bh_lock_sock(nsk) from
   proceeding into __mptcp_init_sock(nsk) concurrently, and the reader can
   iterate msk->conn_list while it still contains the raw copy of the
   listening socket's list head from sock_copy()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793010744.git.tanggeliang@kylinos.cn?part=1