Re: [RFC PATCH net-next] net/smc: avoid conflict with sockmap after fallback

[Dust Li <dust.li@linux.alibaba.com>]

On 2025-03-12 21:30:14, D. Wythe wrote:
>Currently, after fallback, SMC will occupy the sk_user_data of the TCP sock(clcsk) to
>forward events. As a result, we cannot use sockmap after that, since sockmap also
>requires the use of the sk_user_data. Even more, in some cases, this may result in
>abnormal panic.

Looks like this is a bugfix more then a feature.

>
>To enable sockmap after SMC fallback , we need to avoid using sk_user_data and
>instead introduce an additional smc_ctx in tcp_sock to index from TCP sock to SMC sock.
>
>Additionally, we bind the lifecycle of the SMC sock to that of the TCP sock, ensuring
>that the indexing to the SMC sock remains valid throughout the lifetime of the TCP sock.
>
>One key reason is that SMC overrides inet_connection_sock_af_ops, which introduces
>potential dependencies. We must ensure that the af_ops remain visible throughout the
>lifecycle of the TCP socket. In addition, this also resolves potential issues in some
>scenarios where the SMC sock might be invalid.
>
>Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>---
> include/linux/tcp.h |  1 +
> net/smc/af_smc.c    | 53 ++++++++++++++++++++++-----------------------
> net/smc/smc.h       |  8 +++----
> net/smc/smc_close.c |  1 -
> 4 files changed, 30 insertions(+), 33 deletions(-)
>
>diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>index f88daaa76d83..f2223b1cc0d0 100644
>--- a/include/linux/tcp.h
>+++ b/include/linux/tcp.h
>@@ -478,6 +478,7 @@ struct tcp_sock {
> #if IS_ENABLED(CONFIG_SMC)
> 	bool	syn_smc;	/* SYN includes SMC */
> 	bool	(*smc_hs_congested)(const struct sock *sk);
>+	void	*smc_ctx;
> #endif
> 
> #if defined(CONFIG_TCP_MD5SIG) || defined(CONFIG_TCP_AO)
>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>index bc356f77ff1d..d434105639c1 100644
>--- a/net/smc/af_smc.c
>+++ b/net/smc/af_smc.c
>@@ -127,7 +127,7 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> 	struct smc_sock *smc;
> 	struct sock *child;
> 
>-	smc = smc_clcsock_user_data(sk);
>+	smc = smc_sk_from_clcsk(sk);
> 
> 	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
> 				sk->sk_max_ack_backlog)
>@@ -143,8 +143,6 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> 					       own_req);
> 	/* child must not inherit smc or its ops */
> 	if (child) {
>-		rcu_assign_sk_user_data(child, NULL);
>-
> 		/* v4-mapped sockets don't inherit parent ops. Don't restore. */
> 		if (inet_csk(child)->icsk_af_ops == inet_csk(sk)->icsk_af_ops)
> 			inet_csk(child)->icsk_af_ops = smc->ori_af_ops;
>@@ -161,10 +159,7 @@ static bool smc_hs_congested(const struct sock *sk)
> {
> 	const struct smc_sock *smc;
> 
>-	smc = smc_clcsock_user_data(sk);
>-
>-	if (!smc)
>-		return true;
>+	smc = smc_sk_from_clcsk(sk);

I don't see any users of smc in this function. Seems it is redundant here.

> 
> 	if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
> 		return true;
>@@ -250,7 +245,6 @@ static void smc_fback_restore_callbacks(struct smc_sock *smc)
> 	struct sock *clcsk = smc->clcsock->sk;
> 
> 	write_lock_bh(&clcsk->sk_callback_lock);
>-	clcsk->sk_user_data = NULL;
> 
> 	smc_clcsock_restore_cb(&clcsk->sk_state_change, &smc->clcsk_state_change);
> 	smc_clcsock_restore_cb(&clcsk->sk_data_ready, &smc->clcsk_data_ready);
>@@ -832,11 +826,10 @@ static void smc_fback_forward_wakeup(struct smc_sock *smc, struct sock *clcsk,
> 
> static void smc_fback_state_change(struct sock *clcsk)
> {
>-	struct smc_sock *smc;
>+	struct smc_sock *smc = smc_sk_from_clcsk(clcsk);
> 
> 	read_lock_bh(&clcsk->sk_callback_lock);
>-	smc = smc_clcsock_user_data(clcsk);
>-	if (smc)
>+	if (smc->clcsk_state_change)
> 		smc_fback_forward_wakeup(smc, clcsk,
> 					 smc->clcsk_state_change);

Since we checked the clcsock_callback everywhere, why not put it into
smc_fback_forward_wakeup() ?


Best regards,
Dust