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!
