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

[Alexandra Winter <wintera@linux.ibm.com>]

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??

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