On 10/13/23 1:14 AM, Wenjia Zhang wrote:

On 11.10.23 09:33, D. Wythe wrote:

quoted

From: "D. Wythe" <alibuda@linux.alibaba.com>

Consider the following scenario:

                smc_close_passive_work
smc_listen_out_connected
                lock_sock()
if (state  == SMC_INIT)
                if (state  == SMC_INIT)
                    state = SMC_APPCLOSEWAIT1;
    state = SMC_ACTIVE
                release_sock()

This would cause the state machine of the connection to be corrupted.
Also, this issue can occur in smc_listen_out_err().

To solve this problem, we can protect the state transitions under
the lock of sock to avoid collision.

To this fix, I have to repeat the question from Alexandra.
Did the scenario occur in real life? Or is it just kind of potencial 
problem you found during the code review?

Hi Wenjia,

This is a real issue that occurred in our environment rather than being 
obtained from code reviews.
Unfortunately, since this patch does not cause panic, but rather 
potential reference leaks, so it is difficult for me
to provide a very intuitive error message.

If it is the former one, could you please show us the corresponding 
message, e.g. from dmesg? If it is the latter one, I'd like to deal 
with it more carefully. Going from this scenario, I noticed that there 
could also be other similar places where we need to make sure that no 
race happens. Thus, it would make more sense to find a systematic 
approach.

We agree that we should deal with it with more care, In fact, this issue 
is very complex and we may spend a lot of time discussing it. Therefore, 
I suggest that we can temporarily drop it
so that we can quickly accept the patch we have already agreed on. I 
will send those patches separately in the future.

Best Wishes,
D. Wythe

quoted

Fixes: 3b2dec2603d5 ("net/smc: restructure client and server code in 
af_smc")
Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
  net/smc/af_smc.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 5ad2a9f..3bb8265 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c

@@ -1926,8 +1926,10 @@ static void smc_listen_out_connected(struct 

smc_sock *new_smc)
  {
      struct sock *newsmcsk = &new_smc->sk;
  +    lock_sock(newsmcsk);
      if (newsmcsk->sk_state == SMC_INIT)
          newsmcsk->sk_state = SMC_ACTIVE;
+    release_sock(newsmcsk);
        smc_listen_out(new_smc);
  }

@@ -1939,9 +1941,12 @@ static void smc_listen_out_err(struct smc_sock 

*new_smc)
      struct net *net = sock_net(newsmcsk);
this_cpu_inc(net->smc.smc_stats->srv_hshake_err_cnt);
+
+    lock_sock(newsmcsk);
      if (newsmcsk->sk_state == SMC_INIT)
          sock_put(&new_smc->sk); /* passive closing */
      newsmcsk->sk_state = SMC_CLOSED;
+    release_sock(newsmcsk);
        smc_listen_out(new_smc);
  }