Re: [PATCH net 2/2] net/smc: fix LGR and link use-after-free issue

Wen Gu
2024-11-27T07:03:51Z

On 2024/11/25 21:02, Alexandra Winter wrote:
>
>
> On 25.11.24 11:00, Wen Gu wrote:
>>> I wonder if this can deadlock, when you take lock_sock so far down in the callchain.
>>> example:
>>>   smc_connect will first take lock_sock(sk) and then mutex_lock(&smc_server_lgr_pending);  (e.g. in smc_connect_ism())
>>> wheras
>>> smc_listen_work() will take mutex_lock(&smc_server_lgr_pending); and then lock_sock(sk) (in your __smc_conn_abort(,,false))
>>>
>>> I am not sure whether this can be called on the same socket, but it looks suspicious to me.
>>>
>>
>> IMHO this two paths can not occur on the same sk.
>>
>>>
>>> All callers of smc_conn_abort() without socklock seem to originate from smc_listen_work().
>>> That makes me think whether smc_listen_work() should do lock_sock(sk) on a higher level.
>>>
>>
>> Yes, I also think about this question, I guess it is because the new smc sock will be
>> accepted by userspace only after smc_listen_work() is completed. Before that, no userspace
>> operation occurs synchronously with it, so it is not protected by sock lock. But I am not
>> sure if there are other reasons, so I did not aggressively protect the entire smc_listen_work
>> with sock lock, but chose a conservative approach.
>>
>>> Do you have an example which function could collide with smc_listen_work()?
>>> i.e. have you found a way to reproduce this?
>>>
>>
>> We discovered this during our fault injection testing where the rdma driver was rmmod/insmod
>> sporadically during the nginx/wrk 1K connections test.
>>
>> e.g.
>>
>>    __smc_lgr_terminate            | smc_listen_decline
>>    (caused by rmmod mlx5_ib)      | (caused by e.g. reg mr fail)
>>    --------------------------------------------------------------
>>    lock_sock                      |
>>    smc_conn_kill                  | smc_conn_abort
>>     \- smc_conn_free              |  \- smc_conn_free
>>    release_sock                   |
>
>
> Thank you for the explanations. So the most suspicious scenario is
> smc_listen_work() colliding with
> __smc_lgr_terminate() -> smc_conn_kill() of the conn and smc socket that is just under
> construction by smc_listen_work() (without socklock).
>
> I am wondering, if other parts of smc_listen_work() are allowed to run in parallel
> with smc_conn_kill() of this smc socket??
>
Ideally, smc_listen_work should be all covered by new_smc's sock lock, mutually
exclusive with other conn operations.

But I need to look deeper into the smc_listen_work() implementation to see if
all-covered by sock lock is feasible. At least some of the places already protected
by new_smc's sock lock need to be excluded or handled.

e.g.

smc_listen_work()
\- smc_listen_out_xxx()
\- smc_listen_out()
\- smc_close_non_accepted() -> take the new_smc's sock lock.

> My impression would be that the whole smc_listen_work() should be protected against
> smc_conn_kill(), not only smc_conn_free.
>

Thanks!