Re: [PATCH net v2] net/smc: fix abort_work termination in smc_conn_free()

[Hidayath Khan <hidayath@linux.ibm.com>]

On 08/09/26 5:17 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Late-queued `tx_work` can execute on a freed socket because it does not hold a socket reference, causing a use-after-free.
> --
Thanks for the Sashiko AI review. The finding is correct.

tx_work is the only one of the three connection work items with no socket
reference:

   close_work   sock_hold smc_cdc.c:420   sock_put smc_close.c:444
   abort_work   sock_hold smc_cdc.c:324   sock_put smc_core.c:1754
   tx_work        none                                  none

The link group holds a reference while the connection is registered, which
is what hides this. smc_conn_free() drops it in smc_lgr_unregister_conn()
and never cancels tx_work - it only cancels abort_work - and the link
group usually outlives the socket, so lgr->tx_wq is still there to run an
item that was queued late.

Worth noting the fix in this patch does not transfer. conn->freed works
for abort_work because abort_work holds a reference, so the socket is
alive and the flag is safe to read. tx_work holds nothing, so by the time
it runs there may be no conn to test. It needs the missing reference
rather than a guard: sock_hold() at the three queue sites, sock_put() in
smc_tx_work(), and the reference returned at the cancel sites that report
they removed a pending item.

I will send that separately, since it is a different work item and a
different fix. This patch is unchanged.

Thanks,
Hidayath
>
> commit ea8d2d358addfe3a2e07acf0170476ee1325ff01
> Author: Hidayath Khan <hidayath@linux.ibm.com>
>
> net/smc: fix abort_work termination in smc_conn_free()
>
> This commit addresses deadlocks, reference leaks, and late-queued work races
> related to abort_work in smc_conn_free(). It drops the socket lock around
> cancel_work_sync() and uses a freed flag to prevent late-queued work from
> processing a destroyed connection.
>
> Link: https://lore.kernel.org/netdev/20260806081549.595001-1-hidayath@linux.ibm.com/
>
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index 04aedd957543a..9a109eae73b92 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
> [ ... ]
>> @@ -1276,8 +1295,7 @@ void smc_conn_free(struct smc_connection *conn)
>>   			smcd_buf_detach(conn);
>>   	} else {
>>   		smc_cdc_wait_pend_tx_wr(conn);
>> -		if (current_work() != &conn->abort_work)
>> -			cancel_work_sync(&conn->abort_work);
>> +		smc_conn_cancel_abort_work(conn);
> [Severity: Critical]
> This is a pre-existing issue, but while fixing the exact same design flaw for
> abort_work, does this leave tx_work vulnerable to a similar late-queue race?
>
> If a local user closes an SMC socket, smc_close_cancel_work() cancels tx_work.
> However, before the socket is fully destroyed, a remote peer sending a CDC
> message (or a local user reading spliced data triggering
> smc_rx_pipe_buf_release()) can cause the incoming message handler
> smc_cdc_msg_recv_action() to call smc_tx_pending(). This can re-queue the work
> after it was cancelled:
>
> net/smc/smc_tx.c:smc_tx_consumer_update() {
>      ...
>      queue_delayed_work(conn->lgr->tx_wq, &conn->tx_work, SMC_TX_WORK_DELAY);
>      ...
> }
>
> Because tx_work does not hold a socket reference, it can be queued late and
> outlive the socket. If the socket destruction completes and frees the socket,
> and then tx_work expires and executes, it will dereference the freed socket
> memory:
>
> net/smc/smc_tx.c:smc_tx_work() {
>      ...
>      lock_sock(&smc->sk);
>      ...
> }
>
> Could this lead to a use-after-free if it is reliably reachable by a local
> unprivileged user via splice() or by a remote peer sending timed CDC messages
> during socket teardown?
>
>>   	}
>>   	if (!list_empty(&lgr->list)) {
>>   		smc_buf_unuse(conn, lgr); /* allow buffer reuse */