Re: [PATCH net 4/4] net/smc: order the urgent mark across the cursor wrap rollover

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] net/smc: TOCTOU race in smc_rx_recv_urg() [1]
--

commit 474a1c81553cc3992c5253fbaae536390326e390
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: order the urgent mark across the cursor wrap rollover

This commit replaces smc_curs_comp() with smc_curs_sdiff() in
smc_rx_update_consumer() so that consumer and urgent cursors are ordered
correctly across the 16-bit wrap rollover, and removes the unused
smc_curs_comp() helper.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 63c178c10bf1a..0345167cdc0dc 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
> @@ -68,8 +68,8 @@ static int smc_rx_update_consumer(struct smc_sock *smc,
>  
>  	/* did we process urgent data? */
>  	if (conn->urg_state == SMC_URG_VALID || conn->urg_rx_skip_pend) {
> -		diff = smc_curs_comp(conn->rmb_desc->len, &cons,
> -				     &conn->urg_curs);
> +		diff = smc_curs_sdiff(conn->rmb_desc->len, &cons,
> +				      &conn->urg_curs);
>  		if (sock_flag(sk, SOCK_URGINLINE)) {
>  			if (diff == 0) {
>  				force = true;

[Severity: Medium]
This problem wasn't introduced by this patch, but can smc_rx_recv_urg()
race [1] with incoming CDC messages and duplicate an urgent byte?

net/smc/smc_rx.c:smc_rx_recv_urg() {
	...
	if (conn->urg_state == SMC_URG_VALID) {
		if (!(flags & MSG_PEEK))
			smc->conn.urg_state = SMC_URG_READ;
		msg->msg_flags |= MSG_OOB;
		if (len > 0) {
			if (!(flags & MSG_TRUNC))
				rc = memcpy_to_msg(msg, &conn->urg_rx_byte, 1);
	...
}

Although smc_rx_recv_urg() runs under lock_sock(), smc_cdc_msg_recv() in
softirq context only takes bh_lock_sock() without checking
sock_owned_by_user():

CPU0                                  CPU1 (softirq)
smc_rx_recv_urg()
  smc->conn.urg_state = SMC_URG_READ;
                                      smc_cdc_msg_recv()
                                        bh_lock_sock(&smc->sk)
                                        smc_cdc_handle_urg_data_arrival()
                                          conn->urg_rx_byte = new_byte;
                                          conn->urg_state = SMC_URG_VALID;
                                        bh_unlock_sock(&smc->sk)
  memcpy_to_msg(..., &conn->urg_rx_byte, 1)  <-- copies new_byte

The first urgent byte is lost, the current recv(MSG_OOB) returns the new
urgent byte, and because conn->urg_state was reset to SMC_URG_VALID, the
next recv(MSG_OOB) reads the same new byte a second time.

[1] https://sashiko.dev/#/bug/linux-fba17bf0-1137-4667-8969-554d43e1d736

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-disp-1a9ab9b0-v1-0-4a7c8d85d849@proton.me?part=4