Re: [PATCH net-next v1 2/2] net/smc: reduce TX slot contention with exclusive wait

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The open-coded exclusive wait loop introduces a lost wakeup vulnerability by failing to check the condition before enqueuing.

Pre-existing issues:
- [High] Returning -ERESTARTSYS from smc_wr_tx_get_free_slot causes silent data stalling because the caller does not schedule tx_work to retry, and the error is ignored by sendmsg.
--

--- Patch [2]: [PATCH net-next v1 2/2] net/smc: reduce TX slot contention with exclusive wait ---
commit bcc5b21bf03b53f401940337c34d2ab052835d45
Author: D. Wythe <alibuda@linux.alibaba.com>

net/smc: reduce TX slot contention with exclusive wait

This patch modifies smc_wr_tx_get_free_slot() to use an exclusive wait loop 
using prepare_to_wait_exclusive(). It also updates the return value on a 
pending signal from -EPIPE to -ERESTARTSYS to match standard wait semantics.

> diff --git a/net/smc/smc_wr.c b/net/smc/smc_wr.c
> index 6b5add9229937..2cdd4063c13d6 100644
> --- a/net/smc/smc_wr.c
> +++ b/net/smc/smc_wr.c
[ ... ]
> @@ -212,17 +214,31 @@ int smc_wr_tx_get_free_slot(struct smc_link *link,
>  		if (rc)
>  			return rc;
>  	} else {
> -		rc = wait_event_interruptible_timeout(
> -			link->wr_tx_wait,
> -			!smc_link_sendable(link) ||
> -			lgr->terminating ||
> -			(smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY),
> -			SMC_WR_TX_WAIT_FREE_SLOT_TIME);
> -		if (!rc) {
> -			/* timeout - terminate link */
> -			smcr_link_down_cond_sched(link);
> -			return -EPIPE;
> +		rc = 0;
> +		for (;;) {
> +			prepare_to_wait_exclusive(&link->wr_tx_wait, &wait,
> +						  TASK_INTERRUPTIBLE);
> +			if (!smc_link_sendable(link) || lgr->terminating ||
> +			    smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY)
> +				break;

[Severity: High]
Does this open-coded exclusive wait loop introduce a lost wakeup regression?

By calling prepare_to_wait_exclusive() before checking if a slot is available,
the task is unconditionally added to the waitqueue. If it then finds a free
slot immediately and breaks, it remains on the waitqueue until finish_wait() is
called.

If another slot is freed concurrently, an exclusive wake_up() could wake this
exiting task and stop iterating. This absorbs the wakeup and starves other
threads that are actually sleeping on the queue.

This could cause a 10-second stall for the starved thread, leading to a link
termination via smcr_link_down_cond_sched().

> +			timeout = schedule_timeout(timeout);
> +			/* re-check */
> +			if (!smc_link_sendable(link) || lgr->terminating ||
> +			    smc_wr_tx_get_free_slot_index(link, &idx) != -EBUSY)
> +				break;
> +			if (!timeout) {
> +				/* timeout - terminate link */
> +				smcr_link_down_cond_sched(link);
> +				break;
> +			}
> +			if (signal_pending(current)) {
> +				rc = -ERESTARTSYS;
> +				break;
> +			}

[Severity: High]
This is a pre-existing issue, but does returning -ERESTARTSYS
here cause silent data stalling if the caller does not schedule a retry?

If we look at smcr_tx_sndbuf_nonempty() in net/smc/smc_tx.c, it only schedules
tx_work if the return value is -EBUSY:

	if (rc == -EBUSY) {
		...
		mod_delayed_work(conn->lgr->tx_wq, &conn->tx_work,
                                 SMC_TX_WORK_DELAY);
	}
	return rc;

And in smc_tx_sendmsg() in net/smc/smc_tx.c, the return value is ignored:

	if (!smc_tx_should_cork(smc, msg))
		smc_tx_sndbuf_nonempty(conn);

If the syscall is interrupted by a signal, and the data was already copied to
sndbuf, smc_tx_sendmsg() ignores the return value and returns success to
userspace. Since tx_work is not scheduled for -ERESTARTSYS, the data could be
permanently stuck in the buffer.

>  		}
> +		finish_wait(&link->wr_tx_wait, &wait);
> +		if (rc)
> +			return rc;
>  		if (idx == link->wr_tx_cnt)
>  			return -EPIPE;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806064252.39180-1-alibuda@linux.alibaba.com?part=2