Re: [PATCH net] net/smc: drop the abort_work reference when the work is cancelled

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

Thanks for the review.

All three issues are real, and none of them is fixed by anything 
currently in flight.
I will include this patch in a series against net that fixes all four.

On 11/08/26 4:49 am, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "*shiko finds more issues with the work termination here.
> Can you please TAL and fix these all in one series?"
> ---
> net/smc: drop the abort_work reference when the work is cancelled
>
> The patch makes smc_conn_free() drop the socket reference handed to
> conn->abort_work when cancel_work_sync() reports that the work was still
> pending, so the sock_hold() taken by the scheduler of the work is not
> leaked.  It follows the pattern smc_close_cancel_work() already uses for
> conn->close_work.
>
> A few questions about the surrounding cancel logic follow.
>
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index b4208cb186c5e..3d4a86d99dd6b 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -1252,6 +1252,7 @@ static void smc_buf_unuse(struct smc_connection *conn,
>>   /* remove a finished connection from its link group */
>>   void smc_conn_free(struct smc_connection *conn)
>>   {
>> +	struct smc_sock *smc = container_of(conn, struct smc_sock, conn);
>>   	struct smc_link_group *lgr = conn->lgr;
>>   
>>   	if (!lgr || conn->freed)
>> @@ -1275,8 +1276,13 @@ static void smc_buf_unuse(struct smc_connection *conn,
>>   		tasklet_kill(&conn->rx_tsklet);
>>   	} else {
>>   		smc_cdc_wait_pend_tx_wr(conn);
>> -		if (current_work() != &conn->abort_work)
>> -			cancel_work_sync(&conn->abort_work);
>> +		/* If the work was pending (cancel returns true) it never ran,
>> +		 * so the sock_hold taken by its scheduler was never released.
>> +		 */
>> +		if (current_work() != &conn->abort_work) {
>> +			if (cancel_work_sync(&conn->abort_work))
>> +				sock_put(&smc->sk);
>> +		}
> [Severity: High]
> This isn't a bug introduced by this patch, but can this cancel_work_sync()
> deadlock against a already-running abort_work?
Yes, smc_conn_free() holds the socket lock and
smc_conn_abort_work() takes the same lock, so once the work has started
the cancel waits for the work and the work waits for the caller. The
current_work() test only stops the work cancelling itself, so it does not
help when the two are on different CPUs.  I will drop the lock around the
cancel, the way smc_close_cancel_work() already does for close_work.
>
> The worker takes the socket lock as its first action:
>
> net/smc/smc_core.c:smc_conn_abort_work() {
> 	lock_sock(&smc->sk);
> 	smc_conn_kill(conn, true);
> 	release_sock(&smc->sk);
> 	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
> }
>
> and the callers of smc_conn_free() reach it while holding that same lock:
>
> net/smc/af_smc.c:__smc_release()
> 	lock_sock(sk);
> 	...
> 	if (!smc->use_fallback)
> 		smc_conn_free(&smc->conn);
>
> net/smc/smc_close.c:smc_close_passive_work()  (inside lock_sock(sk))
> 	if ((sk->sk_state == SMC_CLOSED) &&
> 	    (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) {
> 		smc_conn_free(conn);
>
> net/smc/smc_close.c:smc_close_active_abort()  (SMC_PEERCLOSEWAIT1/2,
> SMC_PEERFINCLOSEWAIT, SMC_PROCESSABORT, SMC_APPFINCLOSEWAIT)
> 		smc_conn_free(&smc->conn);
>
> So if abort_work has already started on another CPU and is blocked in
> lock_sock(), the closing task holds the socket lock and waits in
> cancel_work_sync() for that worker, which cannot proceed.  The
> current_work() != &conn->abort_work test only covers the case where the
> same task is running abort_work itself.
>
> Note both abort_work and close_work are queued on smc_close_wq, so
> close_work running smc_close_passive_work() can be the blocking canceller.
>
> The commit message cites smc_close_cancel_work() as the model:
>
> 	if (cancel_work_sync(&smc->conn.close_work))
> 		sock_put(sk);
>
> but that helper drops the socket lock around the cancel:
>
> net/smc/smc_close.c:smc_close_cancel_work() {
> 	release_sock(sk);
> 	if (cancel_work_sync(&smc->conn.close_work))
> 		sock_put(sk);
> 	cancel_delayed_work_sync(&smc->conn.tx_work);
> 	lock_sock(sk);
> }
>
> Should the abort_work cancel do the same, and would that change where the
> new sock_put() has to live?
Yes to both.  I will move the cancel into a small helper that drops the
socket lock first, and the sock_put() goes inside that helper, after the
lock is dropped, exactly as in smc_close_cancel_work().
>
> [Severity: High]
> This is a pre-existing ordering issue rather than something the patch adds,
> but does cancel_work_sync() here actually keep abort_work from being
> re-armed?
>
> The cancel happens before smc_lgr_unregister_conn(conn), so the receive
> path can still find the connection:
>
> net/smc/smc_cdc.c:smc_cdc_rx_handler() {
> 	read_lock_bh(&lgr->conns_lock);
> 	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> 	if (!conn || conn->out_of_sync) {
> 		read_unlock_bh(&lgr->conns_lock);
> 		return;
> 	}
> 	smc = container_of(conn, struct smc_sock, conn);
> 	sock_hold(&smc->sk);
> 	read_unlock_bh(&lgr->conns_lock);
>
> and then queue the work again:
>
> net/smc/smc_cdc.c:smc_cdc_msg_validate() {
> 		sock_hold(&smc->sk); /* sock_put in abort_work */
> 		if (!queue_work(smc_close_wq, &conn->abort_work))
> 			sock_put(&smc->sk);
>
> cancel_work_sync() passes cflags 0, so it re-enables the work before
> returning:
>
> kernel/workqueue.c:__cancel_work_sync() {
> 	ret = __cancel_work(work, cflags | WORK_CANCEL_DISABLE);
> 	...
> 	if (wq_online)
> 		__flush_work(work, true);
>
> 	if (!(cflags & WORK_CANCEL_DISABLE))
> 		enable_work(work);
>
> which means a queue_work() issued after the cancel returns succeeds.  The
> freshly queued worker then runs smc_conn_abort_work() -> smc_conn_kill() on
> a connection with conn->freed == 1 whose lgr reference smc_conn_free()
> already dropped, and dereferences conn->lgr:
>
> net/smc/smc_core.c:smc_conn_kill() {
> 	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
>
> Would unregistering the connection from lgr->conns_all before cancelling,
> or gating the queueing on conn->freed under conns_lock, close that window?
The first one.  Gating the queue site would need smc_cdc_msg_validate() to
take conns_lock, which it does not do today, and it would not help with the
buffers.

So I will unregister first, then cancel.  The buffer release has to move as
well: cancel_work_sync() waits for a work item that may still start during
the wait, and that item would then run against buffers smc_buf_unuse() had
already returned.  The order becomes unregister, cancel, release.
>
>>   	}
>>   	if (!list_empty(&lgr->list)) {
>>   		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> [Severity: High]
> This is also a pre-existing problem and not something introduced here, but
> the patch documents smc_conn_free() as the place that disposes of a pending
> abort_work, and one exit path of the function skips that disposal:
>
> net/smc/smc_core.c:smc_conn_free() {
> 	if (!smc_conn_lgr_valid(conn))
> 		/* Connection has already unregistered from
> 		 * link group.
> 		 */
> 		goto lgr_put;
> 	...
> lgr_put:
> 	if (!lgr->is_smcd)
> 		smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */
> 	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
> }
>
> On that path neither the cancel nor the new sock_put() runs, yet the
> connection's link and link-group references are still dropped, and
> conn->lgr is not reset.  A pending abort_work then dereferences it:
>
> net/smc/smc_core.c:smc_conn_kill() {
> 	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
>
> Concretely, a peer sends a CDC message with prod_flags.failover_validation
> and a stale seqno, so smc_cdc_msg_validate() queues abort_work.  Meanwhile
> __smc_lgr_terminate() calls smc_conn_kill(), which does
> smc_lgr_unregister_conn() (clearing alert_token_local) and then
> smc_close_active_abort() -> smc_conn_free(); smc_conn_lgr_valid(conn) is
> now false, so the goto lgr_put path is taken.  __smc_lgr_terminate() then
> calls smc_lgr_free(), whose smc_lgr_put() drops the last reference and
> kfree()s the lgr while abort_work is still queued.
>
> Can the queued abort_work read a freed struct smc_link_group here, and
> should this exit path also cancel the work and return its reference?
Yes to both.  smc_conn_kill() clears alert_token_local through 
smc_lgr_unregister_conn(),
so smc_conn_lgr_valid() is already false when smc_close_active_abort() 
reaches
smc_conn_free(), and the goto lgr_put path skips both the cancel and the
reference.  __smc_lgr_terminate() then frees the link group underneath the
queued work.  I will cancel on that path as well.

I will send the series shortly.