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

["D. Wythe" <alibuda@linux.alibaba.com>]

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