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

[Wen Gu <guwen@linux.alibaba.com>]

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!