Re: [PATCH net] net/smc: avoid recursive sk_callback_lock in listen data_ready

[Mahanta Jambigi]

On 18/06/26 7:46 pm, Runyu Xiao wrote:
> Hi,
> 
> Thanks for taking a look.
> 
> The exact Lockdep stack I have is from the grounded reproducer, not from
> a production SMC setup.  The reproducer keeps the same callback shape:
> the close/flush side holds sk_callback_lock and invokes the installed
> sk_data_ready callback, which re-enters smc_clcsock_data_ready() and tries
> to take sk_callback_lock again.
> 
> The relevant Lockdep report is:
> 
>   WARNING: possible recursive locking detected
>   kworker/u4:3/39 is trying to acquire lock:
>     (sk_callback_lock) at smc_clcsock_data_ready+0xa/0x4d
> 
>   but task is already holding lock:
>     (sk_callback_lock) at smc_close_flush_work+0xc/0x30
> 
>   Possible unsafe locking scenario:
> 
>         CPU0
>         ----
>         lock(sk_callback_lock);
>         lock(sk_callback_lock);
> 
>   *** DEADLOCK ***
> 
>   Workqueue: smc_close_wq smc_close_flush_work
> 
>   Call Trace:
>     dump_stack_lvl
>     __lock_acquire
>     lock_acquire
>     _raw_read_lock_bh
>     smc_clcsock_data_ready+0xa/0x4d
>     smc_close_flush_work+0x1f/0x30
>     process_one_work
>     worker_thread
>     kthread
>     ret_from_fork

Thank you for addressing the feedback. My suggestion would be to reply
to the original email thread where the review comments were given, so
that the maintainers can follow the conversation.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#respond-to-review-comments

Please include above call stack in your next version.

> 
> The nvmet change I referred to is:
> 
>   2fa8961d3a6a ("nvmet-tcp: fixup hang in nvmet_tcp_listen_data_ready()")

Please include this info in your next version.

> 
> The stable/backport patch I originally used as the reference is:
> 
>   1c90f930e7b4 ("nvmet-tcp: fixup hang in nvmet_tcp_listen_data_ready()")
> 
> Its commit message says that when the socket is closed while in
> TCP_LISTEN, the flush callback can call nvmet_tcp_listen_data_ready()
> with sk_callback_lock already held, so nvmet moved the TCP_LISTEN check
> before taking sk_callback_lock.
> 
> For the TCP_LISTEN check: my reasoning was that smc_clcsock_data_ready()
> is installed by smc_listen() on the underlying TCP listen socket and only
> queues smc_tcp_listen_work() for the SMC listen/accept path.  Once that
> underlying socket is no longer in TCP_LISTEN, there should be no SMC
> listen accept work to queue from this callback.  TCP_SYN_RECV and
> TCP_ESTABLISHED are not listen-socket states for this callback path, so I
> did not intend the callback to queue listen work for those states.

I understand. Please include this info in your next version.

> 
> That said, if SMC expects smc_clcsock_data_ready() to handle a non-LISTEN
> state during fallback or another transition, then the proposed check is
> too strict and I should rework the fix.
> 
> Thanks,
> Runyu