[PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[D. Wythe]

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

Consider the following scenarios:

smc_release
	smc_close_active
		write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
		smc->clcsock->sk->sk_user_data = NULL;
		write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);

smc_tcp_syn_recv_sock
	smc = smc_clcsock_user_data(sk);
	/* now */
	/* smc == NULL */

Hence, we may read the a NULL value in smc_tcp_syn_recv_sock(). And
since we only unset sk_user_data during smc_release, it's safe to
drop the incoming tcp reqsock.

Fixes:  ("net/smc: net/smc: Limit backlog connections"
Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/af_smc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index bacdd97..b4acf47 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -125,6 +125,8 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
 	struct sock *child;
 
 	smc = smc_clcsock_user_data(sk);
+	if (unlikely(!smc))
+		goto drop;
 
 	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
 				sk->sk_max_ack_backlog)
-- 
1.8.3.1

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Dust Li]

On Wed, Sep 20, 2023 at 08:08:34PM +0800, D. Wythe wrote:
>From: "D. Wythe" <alibuda@linux.alibaba.com>
>
>Consider the following scenarios:
>
>smc_release
>	smc_close_active
>		write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
>		smc->clcsock->sk->sk_user_data = NULL;
>		write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
>
>smc_tcp_syn_recv_sock
>	smc = smc_clcsock_user_data(sk);
>	/* now */
>	/* smc == NULL */
>
>Hence, we may read the a NULL value in smc_tcp_syn_recv_sock(). And
>since we only unset sk_user_data during smc_release, it's safe to
>drop the incoming tcp reqsock.
>
>Fixes:  ("net/smc: net/smc: Limit backlog connections"
>Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>---
> net/smc/af_smc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>index bacdd97..b4acf47 100644
>--- a/net/smc/af_smc.c
>+++ b/net/smc/af_smc.c
>@@ -125,6 +125,8 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> 	struct sock *child;
> 
> 	smc = smc_clcsock_user_data(sk);
>+	if (unlikely(!smc))
>+		goto drop;

Is it possible smc != NULL here
> 
> 	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
But later turns to NULL in 'atomic_read(&smc->queue_smc_hs)'
> 				sk->sk_max_ack_backlog)

Seems there is still a race ?

>-- 
>1.8.3.1

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Simon Horman]

On Wed, Sep 20, 2023 at 08:08:34PM +0800, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> Consider the following scenarios:
> 
> smc_release
> 	smc_close_active
> 		write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
> 		smc->clcsock->sk->sk_user_data = NULL;
> 		write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
> 
> smc_tcp_syn_recv_sock
> 	smc = smc_clcsock_user_data(sk);
> 	/* now */
> 	/* smc == NULL */
> 
> Hence, we may read the a NULL value in smc_tcp_syn_recv_sock(). And
> since we only unset sk_user_data during smc_release, it's safe to
> drop the incoming tcp reqsock.
> 
> Fixes:  ("net/smc: net/smc: Limit backlog connections"

The tag above is malformed. The correct form is:

Fixes: 8270d9c21041 ("net/smc: Limit backlog connections")

> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> ---
>  net/smc/af_smc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index bacdd97..b4acf47 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -125,6 +125,8 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
>  	struct sock *child;
>  
>  	smc = smc_clcsock_user_data(sk);
> +	if (unlikely(!smc))
> +		goto drop;
>  
>  	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
>  				sk->sk_max_ack_backlog)
> -- 
> 1.8.3.1
> 
> 

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Wenjia Zhang]

On 20.09.23 14:08, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> Consider the following scenarios:
> 
> smc_release
> 	smc_close_active
> 		write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
> 		smc->clcsock->sk->sk_user_data = NULL;
> 		write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
> 
> smc_tcp_syn_recv_sock
> 	smc = smc_clcsock_user_data(sk);
> 	/* now */
> 	/* smc == NULL */
> 
> Hence, we may read the a NULL value in smc_tcp_syn_recv_sock(). And
> since we only unset sk_user_data during smc_release, it's safe to
> drop the incoming tcp reqsock.
> 
> Fixes:  ("net/smc: net/smc: Limit backlog connections"
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> ---
>   net/smc/af_smc.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index bacdd97..b4acf47 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -125,6 +125,8 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
>   	struct sock *child;
>   
>   	smc = smc_clcsock_user_data(sk);
> +	if (unlikely(!smc))
> +		goto drop;
>   
>   	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
>   				sk->sk_max_ack_backlog)

Hi D.Wythe,

this is unfortunately not sufficient for this fix. You have to make sure 
that is not a life-time problem. Even so, READ_ONCE() is also needed in 
this case.

Thanks,
Wenjia

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[D. Wythe]

On 9/22/23 7:59 AM, Wenjia Zhang wrote:
>
>
> On 20.09.23 14:08, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> Consider the following scenarios:
>>
>> smc_release
>>     smc_close_active
>> write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
>>         smc->clcsock->sk->sk_user_data = NULL;
>> write_unlock_bh(&smc->clcsock->sk->sk_callback_lock);
>>
>> smc_tcp_syn_recv_sock
>>     smc = smc_clcsock_user_data(sk);
>>     /* now */
>>     /* smc == NULL */
>>
>> Hence, we may read the a NULL value in smc_tcp_syn_recv_sock(). And
>> since we only unset sk_user_data during smc_release, it's safe to
>> drop the incoming tcp reqsock.
>>
>> Fixes:  ("net/smc: net/smc: Limit backlog connections"
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>>   net/smc/af_smc.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index bacdd97..b4acf47 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -125,6 +125,8 @@ static struct sock *smc_tcp_syn_recv_sock(const 
>> struct sock *sk,
>>       struct sock *child;
>>         smc = smc_clcsock_user_data(sk);
>> +    if (unlikely(!smc))
>> +        goto drop;
>>         if (READ_ONCE(sk->sk_ack_backlog) + 
>> atomic_read(&smc->queued_smc_hs) >
>>                   sk->sk_max_ack_backlog)

Hi Wenjia,

>
> this is unfortunately not sufficient for this fix. You have to make 
> sure that is not a life-time problem. Even so, READ_ONCE() is also 
> needed in this case.
>

Life-time problem? If you means the smc will still be NULL in the 
future,  I don't really think so, smc is a local variable assigned by 
smc_clcsock_user_data.
it's either NULL or a valid and unchanged value.

And READ_ONCE() is needed indeed, considering not make too much change, 
maybe we can protected following

smc = smc_clcsock_user_data(sk);

with sk_callback_lock， which solves the same problem. What do you think?

Best Wishes
D. Wythe

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Alexandra Winter]

On 25.09.23 10:29, D. Wythe wrote:
> Hi Wenjia,
> 
>>
>> this is unfortunately not sufficient for this fix. You have to make sure that is not a life-time problem. Even so, READ_ONCE() is also needed in this case.
>>
> 
> Life-time problem? If you means the smc will still be NULL in the future,  I don't really think so, smc is a local variable assigned by smc_clcsock_user_data.
> it's either NULL or a valid and unchanged value.
> 
> And READ_ONCE() is needed indeed, considering not make too much change, maybe we can protected following

The local variable smc is a pointer to the smc_sock structure, so the question is whether you can just do a READ_ONCE
and then continue to use the content of the smc_sock structure, even though e.g. a smc_close_active() may be going on in 
parallel. 

> 
> smc = smc_clcsock_user_data(sk);
> 
> with sk_callback_lock， which solves the same problem. What do you think?

In af_ops.syn_recv_sock() and thus also in smc_tcp_syn_recv_sock() 
sk is defined as const. So you cannot simply do take sk_callback_lock, that will create compiler errors.
 (same for smc_hs_congested() BTW)

If you are sure the contents of *smc are always valid, then READ_ONCE is all you need.

Maybe it is better to take a step back and consider what needs to be protected when (lifetime).
Just some thoughts (there may be ramifications that I am not aware of):
Maybe clcsock->sk->sk_user_data could be set to point to smc_sock as soon as the clc socket is created?
Isn't the smc socket always valid as long as the clc socket exists? 
Then sk_user_data would no longer indicate whether the callback functions were set to smc values, but would that matter?
Are there scenarios where it matters whether the old or the new callback function is called?
Why are the values restored in smc_close_active() if the clc socket is released shortly after anyhow?

You see I am wondering whether adding more locking, there is a way to make sure structures are safe to use.

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[D. Wythe]

On 9/25/23 5:43 PM, Alexandra Winter wrote:
> On 25.09.23 10:29, D. Wythe wrote:
>> Hi Wenjia,
>>
>>> this is unfortunately not sufficient for this fix. You have to make sure that is not a life-time problem. Even so, READ_ONCE() is also needed in this case.
>>>
>> Life-time problem? If you means the smc will still be NULL in the future,  I don't really think so, smc is a local variable assigned by smc_clcsock_user_data.
>> it's either NULL or a valid and unchanged value.
>>
>> And READ_ONCE() is needed indeed, considering not make too much change, maybe we can protected following
> The local variable smc is a pointer to the smc_sock structure, so the question is whether you can just do a READ_ONCE
> and then continue to use the content of the smc_sock structure, even though e.g. a smc_close_active() may be going on in
> parallel.
>
>> smc = smc_clcsock_user_data(sk);
>>
>> with sk_callback_lock， which solves the same problem. What do you think?
> In af_ops.syn_recv_sock() and thus also in smc_tcp_syn_recv_sock()
> sk is defined as const. So you cannot simply do take sk_callback_lock, that will create compiler errors.
>   (same for smc_hs_congested() BTW)
>
> If you are sure the contents of *smc are always valid, then READ_ONCE is all you need.


Hi Alexandra,

You are right. The key point is how to ensure the valid of smc sock 
during the life time of clc sock, If so, READ_ONCE is good
enough. Unfortunately, I found  that there are no such guarantee, so 
it's still a life-time problem.  Considering the const, maybe
we need to do :

1. hold a refcnt of smc_sock for syn_recv_sock to keep smc sock valid 
during life time of clc sock
2. put the refcnt of smc_sock in sk_destruct in tcp_sock to release the 
very smc sock .

In that way, we can always make sure the valid of smc sock during the 
life time of clc sock. Then we can use READ_ONCE rather
than lock.  What do you think ?

> Maybe it is better to take a step back and consider what needs to be protected when (lifetime).
> Just some thoughts (there may be ramifications that I am not aware of):
> Maybe clcsock->sk->sk_user_data could be set to point to smc_sock as soon as the clc socket is created?
> Isn't the smc socket always valid as long as the clc socket exists?
> Then sk_user_data would no longer indicate whether the callback functions were set to smc values, but would that matter?
> Are there scenarios where it matters whether the old or the new callback function is called?
> Why are the values restored in smc_close_active() if the clc socket is released shortly after anyhow?

That's a good question, We have discussed internally and found that this 
is indeed possible. We can completely not to unset sk_user_data,
which can reduce many unnecessary judgments and locks, and no side 
effects found. We will try this approach internally and conduct multiple
rounds of testing. However, in any case, returning to the initial issue, 
the prerequisite for everything is to ensure the valid of smc sock
during the life time of clc sock. So we must have a mechanism to work it 
out. and holding referenced solutions might be a good try, what do you
think?

Best Wishes,
D. Wythe

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Alexandra Winter]

On 26.09.23 05:00, D. Wythe wrote:
> You are right. The key point is how to ensure the valid of smc sock during the life time of clc sock, If so, READ_ONCE is good
> enough. Unfortunately, I found  that there are no such guarantee, so it's still a life-time problem.  

Did you discover a scenario, where clc sock could live longer than smc sock? 
Wouldn't that be a dangerous scenario in itself? I still have some hope that the lifetime of an smc socket is by design longer
than that of the corresponding tcp socket.

Considering the const, maybe
> we need to do :
> 
> 1. hold a refcnt of smc_sock for syn_recv_sock to keep smc sock valid during life time of clc sock
> 2. put the refcnt of smc_sock in sk_destruct in tcp_sock to release the very smc sock .
> 
> In that way, we can always make sure the valid of smc sock during the life time of clc sock. Then we can use READ_ONCE rather
> than lock.  What do you think ?

I am not sure I fully understand the details what you propose to do. And it is not only syn_recv_sock(), right?
You need to consider all relations between smc socks and tcp socks; fallback to tcp, initial creation, children of listen sockets, variants of shutdown, ... Preferrably a single simple mechanism covers all situations. Maybe there is such a mechanism already today?
(I don't think clcsock->sk->sk_user_data or sk_callback_lock provide this general coverage)
If we really have a gap, a general refcnt'ing on smc sock could be a solution, but needs to be designed carefully.

Many thanks to you and the team to help make smc more stable and robust.

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[D. Wythe]

On 9/26/23 3:18 PM, Alexandra Winter wrote:
>
> On 26.09.23 05:00, D. Wythe wrote:
>> You are right. The key point is how to ensure the valid of smc sock during the life time of clc sock, If so, READ_ONCE is good
>> enough. Unfortunately, I found  that there are no such guarantee, so it's still a life-time problem.
> Did you discover a scenario, where clc sock could live longer than smc sock?
> Wouldn't that be a dangerous scenario in itself? I still have some hope that the lifetime of an smc socket is by design longer
> than that of the corresponding tcp socket.


Hi Alexandra,

Yes there is. Considering scenario:

tcp_v4_rcv(skb)

/* req sock */
reqsk = _inet_lookup_skb(skb)

/* listen sock */
sk = reqsk(reqsk)->rsk_listener;
sock_hold(sk);
tcp_check_req(sk)


                                                 smc_release /* release 
smc listen sock */
                                                 __smc_release
smc_close_active()         /*  smc_sk->sk_state = SMC_CLOSED; */
                                                     if 
(smc_sk->sk_state == SMC_CLOSED)
smc_clcsock_release();
sock_release(clcsk);        /* close clcsock */
     sock_put(sk);              /* might not  the final refcnt */

sock_put(smc_sk)    /* might be the final refcnt of smc_sock  */

syn_recv_sock(sk...)
/* might be the final refcnt of tcp listen sock */
sock_put(sk);

Fortunately, this scenario only affects smc_syn_recv_sock and 
smc_hs_congested, as other callbacks already have locks to protect smc,
which can guarantee that the sk_user_data is either NULL (set in 
smc_close_active) or valid under the lock.

> Considering the const, maybe
>> we need to do :
>>
>> 1. hold a refcnt of smc_sock for syn_recv_sock to keep smc sock valid during life time of clc sock
>> 2. put the refcnt of smc_sock in sk_destruct in tcp_sock to release the very smc sock .
>>
>> In that way, we can always make sure the valid of smc sock during the life time of clc sock. Then we can use READ_ONCE rather
>> than lock.  What do you think ?
> I am not sure I fully understand the details what you propose to do. And it is not only syn_recv_sock(), right?
> You need to consider all relations between smc socks and tcp socks; fallback to tcp, initial creation, children of listen sockets, variants of shutdown, ... Preferrably a single simple mechanism covers all situations. Maybe there is such a mechanism already today?
> (I don't think clcsock->sk->sk_user_data or sk_callback_lock provide this general coverage)
> If we really have a gap, a general refcnt'ing on smc sock could be a solution, but needs to be designed carefully.

You are right , we need designed it with care, we will try the 
referenced solutions internally first, and I will also send some RFCs so 
that everyone can track the latest progress
and make it can be all agreed.
> Many thanks to you and the team to help make smc more stable and robust.

Our pleasure 😁.  The stability of smc is important to us too.

Best wishes,
D. Wythe

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Alexandra Winter]

On 26.09.23 11:06, D. Wythe wrote:
>> Considering the const, maybe
>>> we need to do :
>>>
>>> 1. hold a refcnt of smc_sock for syn_recv_sock to keep smc sock valid during life time of clc sock
>>> 2. put the refcnt of smc_sock in sk_destruct in tcp_sock to release the very smc sock .
>>>
>>> In that way, we can always make sure the valid of smc sock during the life time of clc sock. Then we can use READ_ONCE rather
>>> than lock.  What do you think ?
>> I am not sure I fully understand the details what you propose to do. And it is not only syn_recv_sock(), right?
>> You need to consider all relations between smc socks and tcp socks; fallback to tcp, initial creation, children of listen sockets, variants of shutdown, ... Preferrably a single simple mechanism covers all situations. Maybe there is such a mechanism already today?
>> (I don't think clcsock->sk->sk_user_data or sk_callback_lock provide this general coverage)
>> If we really have a gap, a general refcnt'ing on smc sock could be a solution, but needs to be designed carefully.
> 
> You are right , we need designed it with care, we will try the referenced solutions internally first, and I will also send some RFCs so that everyone can track the latest progress
> and make it can be all agreed.
>> Many thanks to you and the team to help make smc more stable and robust.
> 
> Our pleasure 😁.  The stability of smc is important to us too.
> 
> Best wishes,
> D. Wythe


Just one more thought: I noticed that 
9744d2bf1976 ("smc: Fix use-after-free in tcp_write_timer_handler().")
states that unlike MPTCP, smc_clcsock_release() does not call __tcp_close().
(which matches your explanation). 
Maybe we something similar to the MPTCP approach could also solve this issue?

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Wenjia Zhang]

On 26.09.23 11:06, D. Wythe wrote:
> 
> 
> On 9/26/23 3:18 PM, Alexandra Winter wrote:
>>
>> On 26.09.23 05:00, D. Wythe wrote:
>>> You are right. The key point is how to ensure the valid of smc sock 
>>> during the life time of clc sock, If so, READ_ONCE is good
>>> enough. Unfortunately, I found  that there are no such guarantee, so 
>>> it's still a life-time problem.
>> Did you discover a scenario, where clc sock could live longer than smc 
>> sock?
>> Wouldn't that be a dangerous scenario in itself? I still have some 
>> hope that the lifetime of an smc socket is by design longer
>> than that of the corresponding tcp socket.
> 
> 
> Hi Alexandra,
> 
> Yes there is. Considering scenario:
> 
> tcp_v4_rcv(skb)
> 
> /* req sock */
> reqsk = _inet_lookup_skb(skb)
> 
> /* listen sock */
> sk = reqsk(reqsk)->rsk_listener;
> sock_hold(sk);
> tcp_check_req(sk)
> 
> 
>                                                  smc_release /* release 
> smc listen sock */
>                                                  __smc_release
> smc_close_active()         /*  smc_sk->sk_state = SMC_CLOSED; */
>                                                      if 
> (smc_sk->sk_state == SMC_CLOSED)
> smc_clcsock_release();
> sock_release(clcsk);        /* close clcsock */
>      sock_put(sk);              /* might not  the final refcnt */
> 
> sock_put(smc_sk)    /* might be the final refcnt of smc_sock  */
> 
> syn_recv_sock(sk...)
> /* might be the final refcnt of tcp listen sock */
> sock_put(sk);
> 
> Fortunately, this scenario only affects smc_syn_recv_sock and 
> smc_hs_congested, as other callbacks already have locks to protect smc,
> which can guarantee that the sk_user_data is either NULL (set in 
> smc_close_active) or valid under the lock.
> I'm kind of confused with this scenario. How could the 
smc_clcsock_release()->sock_release(clcsk) happen?
Because the syn_recv_sock happens short prior to accept(), that means 
that the &smc->tcp_listen_work is already triggered but the real 
accept() is still not happening. At this moment, the incoming connection 
is being added into the accept queue. Thus, if the sk->sk_state is 
changed from SMC_LISTEN to SMC_CLOSED in smc_close_active(), there is 
still "flush_work(&smc->tcp_listen_work);" after that. That ensures the 
smc_clcsock_release() should not happen, if smc_clcsock_accept() is not 
finished. Do you think that the execution of the &smc->tcp_listen_work 
is already done? Or am I missing something?

>> Considering the const, maybe
>>> we need to do :
>>>
>>> 1. hold a refcnt of smc_sock for syn_recv_sock to keep smc sock valid 
>>> during life time of clc sock
>>> 2. put the refcnt of smc_sock in sk_destruct in tcp_sock to release 
>>> the very smc sock .
>>>
>>> In that way, we can always make sure the valid of smc sock during the 
>>> life time of clc sock. Then we can use READ_ONCE rather
>>> than lock.  What do you think ?
>> I am not sure I fully understand the details what you propose to do. 
>> And it is not only syn_recv_sock(), right?
>> You need to consider all relations between smc socks and tcp socks; 
>> fallback to tcp, initial creation, children of listen sockets, 
>> variants of shutdown, ... Preferrably a single simple mechanism covers 
>> all situations. Maybe there is such a mechanism already today?
>> (I don't think clcsock->sk->sk_user_data or sk_callback_lock provide 
>> this general coverage)
>> If we really have a gap, a general refcnt'ing on smc sock could be a 
>> solution, but needs to be designed carefully.
> 
> You are right , we need designed it with care, we will try the 
> referenced solutions internally first, and I will also send some RFCs so 
> that everyone can track the latest progress
> and make it can be all agreed.
>> Many thanks to you and the team to help make smc more stable and robust.
> 
> Our pleasure 😁.  The stability of smc is important to us too.
> 
> Best wishes,
> D. Wythe
> 
> 

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[D. Wythe]

On 10/6/23 2:14 AM, Wenjia Zhang wrote:
>
>
> On 26.09.23 11:06, D. Wythe wrote:
>>
>>
>> On 9/26/23 3:18 PM, Alexandra Winter wrote:
>>>
>>> On 26.09.23 05:00, D. Wythe wrote:
>>>> You are right. The key point is how to ensure the valid of smc sock 
>>>> during the life time of clc sock, If so, READ_ONCE is good
>>>> enough. Unfortunately, I found  that there are no such guarantee, 
>>>> so it's still a life-time problem.
>>> Did you discover a scenario, where clc sock could live longer than 
>>> smc sock?
>>> Wouldn't that be a dangerous scenario in itself? I still have some 
>>> hope that the lifetime of an smc socket is by design longer
>>> than that of the corresponding tcp socket.
>>
>>
>> Hi Alexandra,
>>
>> Yes there is. Considering scenario:
>>
>> tcp_v4_rcv(skb)
>>
>> /* req sock */
>> reqsk = _inet_lookup_skb(skb)
>>
>> /* listen sock */
>> sk = reqsk(reqsk)->rsk_listener;
>> sock_hold(sk);
>> tcp_check_req(sk)
>>
>>
>>                                                  smc_release /* 
>> release smc listen sock */
>>                                                  __smc_release
>> smc_close_active()         /*  smc_sk->sk_state = SMC_CLOSED; */
>>                                                      if 
>> (smc_sk->sk_state == SMC_CLOSED)
>> smc_clcsock_release();
>> sock_release(clcsk);        /* close clcsock */
>>      sock_put(sk);              /* might not  the final refcnt */
>>
>> sock_put(smc_sk)    /* might be the final refcnt of smc_sock  */
>>
>> syn_recv_sock(sk...)
>> /* might be the final refcnt of tcp listen sock */
>> sock_put(sk);
>>
>> Fortunately, this scenario only affects smc_syn_recv_sock and 
>> smc_hs_congested, as other callbacks already have locks to protect smc,
>> which can guarantee that the sk_user_data is either NULL (set in 
>> smc_close_active) or valid under the lock.
>> I'm kind of confused with this scenario. How could the 
> smc_clcsock_release()->sock_release(clcsk) happen?
> Because the syn_recv_sock happens short prior to accept(), that means 
> that the &smc->tcp_listen_work is already triggered but the real 
> accept() is still not happening. At this moment, the incoming 
> connection is being added into the accept queue. Thus, if the 
> sk->sk_state is changed from SMC_LISTEN to SMC_CLOSED in 
> smc_close_active(), there is still 
> "flush_work(&smc->tcp_listen_work);" after that. That ensures the 
> smc_clcsock_release() should not happen, if smc_clcsock_accept() is 
> not finished. Do you think that the execution of the 
> &smc->tcp_listen_work is already done? Or am I missing something?
>
Hi wenjia,

Sorry for late reply, we have just returned from vacation.

The smc_clcsock_release here release the listen clcsock rather than the 
child clcsock.
So the flush_work might not be helpful for this scenario.

Best wishes,
D. Wythe


>>> Considering the const, maybe
>>>> we need to do :
>>>>
>>>> 1. hold a refcnt of smc_sock for syn_recv_sock to keep smc sock 
>>>> valid during life time of clc sock
>>>> 2. put the refcnt of smc_sock in sk_destruct in tcp_sock to release 
>>>> the very smc sock .
>>>>
>>>> In that way, we can always make sure the valid of smc sock during 
>>>> the life time of clc sock. Then we can use READ_ONCE rather
>>>> than lock.  What do you think ?
>>> I am not sure I fully understand the details what you propose to do. 
>>> And it is not only syn_recv_sock(), right?
>>> You need to consider all relations between smc socks and tcp socks; 
>>> fallback to tcp, initial creation, children of listen sockets, 
>>> variants of shutdown, ... Preferrably a single simple mechanism 
>>> covers all situations. Maybe there is such a mechanism already today?
>>> (I don't think clcsock->sk->sk_user_data or sk_callback_lock provide 
>>> this general coverage)
>>> If we really have a gap, a general refcnt'ing on smc sock could be a 
>>> solution, but needs to be designed carefully.
>>
>> You are right , we need designed it with care, we will try the 
>> referenced solutions internally first, and I will also send some RFCs 
>> so that everyone can track the latest progress
>> and make it can be all agreed.
>>> Many thanks to you and the team to help make smc more stable and 
>>> robust.
>>
>> Our pleasure 😁.  The stability of smc is important to us too.
>>
>> Best wishes,
>> D. Wythe
>>
>>

Re: [PATCH net] net/smc: fix panic smc_tcp_syn_recv_sock() while closing listen socket

[Wenjia Zhang]

On 05.10.23 20:14, Wenjia Zhang wrote:
>> 
>> 
>> On 26.09.23 11:06, D. Wythe wrote:
>>>
>>>
>>> On 9/26/23 3:18 PM, Alexandra Winter wrote:
>>>>
>>>> On 26.09.23 05:00, D. Wythe wrote:
>>>>> You are right. The key point is how to ensure the valid of smc sock 
>>>>> during the life time of clc sock, If so, READ_ONCE is good
>>>>> enough. Unfortunately, I found  that there are no such guarantee, so 
>>>>> it's still a life-time problem.
>>>> Did you discover a scenario, where clc sock could live longer than 
>>>> smc sock?
>>>> Wouldn't that be a dangerous scenario in itself? I still have some 
>>>> hope that the lifetime of an smc socket is by design longer
>>>> than that of the corresponding tcp socket.
>>>
>>>
>>> Hi Alexandra,
>>>
>>> Yes there is. Considering scenario:
>>>
>>> tcp_v4_rcv(skb)
>>>
>>> /* req sock */
>>> reqsk = _inet_lookup_skb(skb)
>>>
>>> /* listen sock */
>>> sk = reqsk(reqsk)->rsk_listener;
>>> sock_hold(sk);
>>> tcp_check_req(sk)
>>>
>>>
>>>                                                  smc_release /* 
>>> release smc listen sock */
>>>                                                  __smc_release
>>> smc_close_active()         /*  smc_sk->sk_state = SMC_CLOSED; */
>>>                                                      if 
>>> (smc_sk->sk_state == SMC_CLOSED)
>>> smc_clcsock_release();
>>> sock_release(clcsk);        /* close clcsock */
>>>      sock_put(sk);              /* might not  the final refcnt */
>>>
>>> sock_put(smc_sk)    /* might be the final refcnt of smc_sock  */
>>>
>>> syn_recv_sock(sk...)
>>> /* might be the final refcnt of tcp listen sock */
>>> sock_put(sk);
>>>
>>> Fortunately, this scenario only affects smc_syn_recv_sock and 
>>> smc_hs_congested, as other callbacks already have locks to protect smc,
>>> which can guarantee that the sk_user_data is either NULL (set in 
>>> smc_close_active) or valid under the lock.
>>> I'm kind of confused with this scenario. How could the 
>> smc_clcsock_release()->sock_release(clcsk) happen?
>> Because the syn_recv_sock happens short prior to accept(), that means 
>> that the &smc->tcp_listen_work is already triggered but the real 
>> accept() is still not happening. At this moment, the incoming connection 
>> is being added into the accept queue. Thus, if the sk->sk_state is 
>> changed from SMC_LISTEN to SMC_CLOSED in smc_close_active(), there is 
>> still "flush_work(&smc->tcp_listen_work);" after that. That ensures the 
>> smc_clcsock_release() should not happen, if smc_clcsock_accept() is not 
>> finished. Do you think that the execution of the &smc->tcp_listen_work 
>> is already done? Or am I missing something?
>>  > Hi wenjia,
 >
 > Sorry for late reply, we have just returned from vacation.
 >
 > The smc_clcsock_release here release the listen clcsock rather than
 > the child clcsock.
 > So the flush_work might not be helpful for this scenario.
 >
 > Best wishes,
 > D. Wythe

It seems like that I lost some mails these days :-( Just saw your answer.

Maybe I didn't describe my thought clearly. Following data flow is your 
scenario, right?
			–
(sk_state == SMC_LISTEN)|
tcp_check_req()		| smc_release()
			| ->__smc_release()
			|   -> smc_close_active()
			|     -> sk->sk_state = SMC_CLOSED;
			|     -> ...
			|     -> smc->clcsock->sk->sk_user_data = NULL;
			|     -> ...
			|*1)  -> flush_work(&smc->tcp_listen_work);
			|*4)
			|	-> smc_clcsock_accept()
	    		|         -> kernel_accept()
			| 	    -> inet_csk_accept()
			|*5)
			|   if (sk->sk_state == SMC_CLOSED)
			|*3)-> smc_clcsock_release()
-> syn_recv_sock()   *2)|
			|
			v
My question is how the smc_clcsock_release() could happen after the 
syn_recv_sock()?
IMO, the syn_recv_sock() should be called during the 
&smc->tcp_listen_work, which is corresponding to lsmc (listen smc). And 
in smc_clcsock_accept(), the lsmc->clcsock as the listening socket goes 
on to be used to accept a new connection. If the &smc->tcp_listen_work 
is not finished, *1) will wait for its finishing. It can only happen in 
following situation:
*4) sk_state is SMC_CLOSED, then no connection is accepted.
*5) old sk_state is SMC_LISTEN, TCP accept is successful. But current 
sk_state is SMC_CLOSED. Thus, no new smc connection.

What do you think? Please let me know if I have any lapse of thought.

Thanks,
Wenjia