[PATCH net] net/smc: Serialize CLC preparation with link teardown

[PATCH net] net/smc: Serialize CLC preparation with link teardown

[Chengfeng Ye]

smcr_clc_prep_confirm_accept() reads the RMB memory region and scatterlist
without holding llc_conf_mutex. The connection's link reference keeps the
link object alive, but does not prevent smcr_link_clear() from unmapping
its buffers and destroying its RDMA resources.

The CLC handshake can load the scatterlist pointer, then an LLC delete-link
worker can acquire llc_conf_mutex for writing and free the scatterlist in
smcr_buf_unmap_link(). When the handshake resumes, sg_dma_address() reads
freed memory. The memory-region rkey read has the same lifetime problem.

KASAN reported:

  BUG: KASAN: slab-use-after-free in smc_clc_send_confirm_accept
  Read of size 8 at addr ffff88810efcdfd0 by task poc/94
  Call Trace:
   smc_clc_send_confirm_accept
   smc_clc_send_confirm
   __smc_connect
   smc_connect
   __sys_connect
  Allocated by task 94:
   __sg_alloc_table
   sg_alloc_table
   smcr_buf_map_link
   __smc_buf_create
   smc_buf_create
   __smc_connect
  Freed by task 11:
   kfree
   sg_free_table
   smcr_buf_unmap_link
   smcr_link_clear
   smc_llc_delete_link_work

Hold llc_conf_mutex for reading while preparing the SMC-R message,
including the QP accesses. Reject unusable or cleared links under the
lock so that teardown completing before preparation is also handled.
Keep activating links valid for first contact and release the lock
before sending over TCP.

Preserve the preparation error in both CLC send wrappers when the TCP
socket has no error recorded. Otherwise the new -ENOLINK return is
converted to success. Existing TCP errors and short-write handling
retain priority.

Fixes: 541afa10c126 ("net/smc: add smcr_port_err() and smcr_link_down() processing")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/smc/smc_clc.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 014d527d5462..6b9e64a5b9d0 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -1169,14 +1169,21 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 	clc->hdr.version = version;	/* SMC version */
 	if (first_contact)
 		clc->hdr.typev2 |= SMC_FIRST_CONTACT_MASK;
-	if (conn->lgr->is_smcd)
+	if (conn->lgr->is_smcd) {
 		smcd_clc_prep_confirm_accept(conn, clc, first_contact,
 					     version, eid, ini, &fce_len,
 					     &fce_v2x, &trl);
-	else
+	} else {
+		down_read(&conn->lgr->llc_conf_mutex);
+		if (!smc_link_usable(conn->lnk) || conn->lnk->clearing) {
+			up_read(&conn->lgr->llc_conf_mutex);
+			return -ENOLINK;
+		}
 		smcr_clc_prep_confirm_accept(conn, clc, first_contact,
 					     version, eid, ini, &fce_len,
 					     &fce_v2x, &gle, &trl);
+		up_read(&conn->lgr->llc_conf_mutex);
+	}
 	memset(&msg, 0, sizeof(msg));
 	i = 0;
 	vec[i].iov_base = clc;
@@ -1227,7 +1234,7 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
 			reason_code = -ENETUNREACH;
 			smc->sk.sk_err = -reason_code;
 		} else {
-			smc->sk.sk_err = smc->clcsock->sk->sk_err;
+			smc->sk.sk_err = smc->clcsock->sk->sk_err ?: -len;
 			reason_code = -smc->sk.sk_err;
 		}
 	}
@@ -1246,7 +1253,8 @@ int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
 	len = smc_clc_send_confirm_accept(new_smc, &aclc, srv_first_contact,
 					  version, negotiated_eid, ini);
 	if (len < ntohs(aclc.hdr.length))
-		len = len >= 0 ? -EPROTO : -new_smc->clcsock->sk->sk_err;
+		len = len >= 0 ? -EPROTO :
+			-(new_smc->clcsock->sk->sk_err ?: -len);
 
 	return len > 0 ? 0 : len;
 }
-- 
2.43.0

Re: [PATCH net] net/smc: Serialize CLC preparation with link teardown

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (2) · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When smc_clc_send_confirm_accept() (net/smc/smc_clc.c) finds
  that conn->lnk is unusable or being cleared under llc_conf_mutex, it…
- [Medium] smc_clc_send_confirm() (net/smc/smc_clc.c) now does
  `smc->sk.sk_err = smc->clcsock->sk->sk_err ?: -len; reason_code =…
- [Low] The `sk_err ?: -len` changes in smc_clc_send_confirm() and
  smc_clc_send_accept() (net/smc/smc_clc.c) do more than keep the new…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it concerns
  the locking contract the fix relies on.
- [Medium] This problem wasn't introduced by this patch, but the patch
  makes the CLC send helpers inconsistent.
- [Low] This problem wasn't introduced by this patch (it dates from
  2020).

--

net/smc: Serialize CLC preparation with link teardown

This commit makes smc_clc_send_confirm_accept() hold llc_conf_mutex for
reading while it prepares the SMC-R CONFIRM/ACCEPT message. If the link
is unusable or being cleared, it returns -ENOLINK. It also changes
smc_clc_send_confirm() and smc_clc_send_accept() to use the
kernel_sendmsg() return value when the TCP socket has no sk_err set.

> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
> index 014d527d5462d..6b9e64a5b9d0a 100644
> --- a/net/smc/smc_clc.c
> +++ b/net/smc/smc_clc.c
> @@ -1169,14 +1169,21 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
>  	clc->hdr.version = version;	/* SMC version */
>  	if (first_contact)
>  		clc->hdr.typev2 |= SMC_FIRST_CONTACT_MASK;
> -	if (conn->lgr->is_smcd)
> +	if (conn->lgr->is_smcd) {
>  		smcd_clc_prep_confirm_accept(conn, clc, first_contact,
>  					     version, eid, ini, &fce_len,
>  					     &fce_v2x, &trl);
> -	else
> +	} else {
> +		down_read(&conn->lgr->llc_conf_mutex);
> +		if (!smc_link_usable(conn->lnk) || conn->lnk->clearing) {
> +			up_read(&conn->lgr->llc_conf_mutex);
> +			return -ENOLINK;

[Severity: Medium]
Should this return a positive CLC decline code instead of -ENOLINK?

Both handshake paths read a negative reason code as "fallback is not
possible". On the client, smc_clc_send_confirm() returns -ENOLINK,
smc_connect_rdma() jumps to connect_abort, and then
smc_connect_decline_fallback() does:

net/smc/af_smc.c:smc_connect_decline_fallback() {
    ...
	if (reason_code < 0) { /* error, fallback is not possible */
    ...
}

As a result, connect() fails with ENOLINK.

On the server, smc_listen_work()->smc_listen_decline() does:

	if (reason_code < 0 ||
	    smc_switch_to_fallback(new_smc, reason_code)) {
		/* error, no fallback possible */
		smc_listen_out_err(new_smc);

and the accepted connection is torn down.

Other SMC-R link and buffer failures at the same stage return positive
decline codes, send a DECLINE and fall back to TCP. Examples in
smc_connect_rdma() are SMC_CLC_DECL_ERR_RDYLNK and
SMC_CLC_DECL_ERR_REGBUF. Neither CONFIRM nor ACCEPT has been sent yet,
so a DECLINE is still valid here.

A matching code already exists and smcr_lgr_conn_assign_link() uses
it:

#define SMC_CLC_DECL_NOACTLINK	0x030a0000  /* no active smc-r link in lgr    */

In multi-link groups, smc_switch_conns() skips SMC_INIT connections. A
connection that is still handshaking therefore keeps the dying link
even when another link is available.

Could SMC_CLC_DECL_NOACTLINK be reported here so the connection falls
back to TCP? The callers of smc_clc_send_confirm_accept() treat positive
return values as byte counts, so the wrappers would need a small
restructure to pass it through.

The commit message also doesn't mention that this case now fails
instead of falling back to TCP.

> +		}
>  		smcr_clc_prep_confirm_accept(conn, clc, first_contact,
>  					     version, eid, ini, &fce_len,
>  					     &fce_v2x, &gle, &trl);
> +		up_read(&conn->lgr->llc_conf_mutex);
> +	}

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch, but it
affects the locking contract this down_read() relies on.
smcr_link_clear() is documented as:

/* must be called under lgr->llc_conf_mutex lock */

smcr_clnt_conf_first_link() in af_smc.c calls smc_llc_cli_add_link()
without taking llc_conf_mutex:

		smc_llc_flow_qentry_clr(&link->lgr->llc_flow_lcl);
		smc_llc_cli_add_link(link, qentry);

smc_llc_cli_add_link() calls smcr_link_init() and
smcr_buf_map_lgr(lnk_new). On its out_clear_lnk error path it also
calls smcr_link_clear(lnk_new).

The other callers take the write side. smc_llc_process_cli_add_link()
wraps the same function in down_write(), and smcr_serv_conf_first_link()
and smc_llc_process_srv_add_link() do the same around
smc_llc_srv_add_link().

The new reader here doesn't seem able to see the cleared lnk_new. That
slot isn't any connection's conn->lnk, and smc_client_lgr_pending
serializes other client connections on this link group.

Should smcr_clnt_conf_first_link() still hold llc_conf_mutex for
writing around smc_llc_cli_add_link()? The new down_read() assumes
that all link setup and teardown runs under the write side.

>  	memset(&msg, 0, sizeof(msg));
>  	i = 0;
>  	vec[i].iov_base = clc;

[Severity: Low]
This isn't a bug introduced by this patch (it dates from 2020), but
further down, smc_clc_send_confirm_accept() builds up to five kvecs
(header, fce_v2x, gle, gid list, trailer) and then sends them with num
set to 1:

	return kernel_sendmsg(smc->clcsock, &msg, vec, 1,
			      ntohs(clc->hdr.length));

kernel_sendmsg() passes num to iov_iter_kvec() as nr_segs. This only
works because iterate_kvec() walks the segments by remaining length
rather than by nr_segs. Afterwards, iter->nr_segs underflows:

	iter->nr_segs -= p - iter->kvec;

Should this pass i instead of 1?

[ ... ]

> @@ -1227,7 +1234,7 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
>  			reason_code = -ENETUNREACH;
>  			smc->sk.sk_err = -reason_code;
>  		} else {
> -			smc->sk.sk_err = smc->clcsock->sk->sk_err;
> +			smc->sk.sk_err = smc->clcsock->sk->sk_err ?: -len;
>  			reason_code = -smc->sk.sk_err;

[Severity: Medium]
Can this copy -ERESTARTSYS into sk_err and into the connect() return
value?

On a blocking connect(), __smc_connect() runs in the caller's context,
and the clcsock sndtimeo defaults to MAX_SCHEDULE_TIMEOUT. Suppose
tcp_sendmsg_locked() waits in sk_stream_wait_memory(), for example
under TCP memory pressure, while a signal is pending. It then returns
sock_intr_errno(MAX_SCHEDULE_TIMEOUT), which is -ERESTARTSYS.

With this change, smc->sk.sk_err is set to 512, which
getsockopt(SO_ERROR) can expose. smc_connect() also returns
-ERESTARTSYS, so the syscall may be restarted, for example after
SIGSTOP/SIGCONT or with an SA_RESTART handler.

smc_connect_decline_fallback() leaves sk_state at SMC_INIT and
sock->state at SS_CONNECTING. On restart, kernel_connect() on the
already connected clcsock returns -EISCONN. The application may read
that as success, even though the SMC connection was aborted.

The receive side in smc_clc_wait_msg() handles this case explicitly:

	if (signal_pending(current)) {
		reason_code = -EINTR;
		clc_sk->sk_err = EINTR;
		smc->sk.sk_err = EINTR;

Should the send side map signal errors to EINTR the same way? Before
this patch the same case was reported as success, so the change helps,
but the errno exposed here looks wrong.

[Severity: Medium]
This is a pre-existing issue, but after this change the CLC send
helpers handle the same error differently. smc_clc_send_proposal()
still does:

	len = kernel_sendmsg(smc->clcsock, &msg, vec, i, plen);
	if (len < 0) {
		smc->sk.sk_err = smc->clcsock->sk->sk_err;
		reason_code = -smc->sk.sk_err;

sk_err can be 0 here: after sk_stream_error()->sock_error() clears it
on EPIPE, or when the send fails with -EAGAIN, -EINTR or -ENOMEM. In
that case this returns 0 even though the send failed. The client then
waits in smc_clc_wait_msg() and is likely to hit an error or EOF there.

Should smc_clc_send_proposal() use the same sk_err ?: -len fallback?

>  		}
>  	}
> @@ -1246,7 +1253,8 @@ int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
>  	len = smc_clc_send_confirm_accept(new_smc, &aclc, srv_first_contact,
>  					  version, negotiated_eid, ini);
>  	if (len < ntohs(aclc.hdr.length))
> -		len = len >= 0 ? -EPROTO : -new_smc->clcsock->sk->sk_err;
> +		len = len >= 0 ? -EPROTO :
> +			-(new_smc->clcsock->sk->sk_err ?: -len);
>  
>  	return len > 0 ? 0 : len;
>  }

[Severity: Low]
The commit message describes these two hunks as:

    Preserve the preparation error in both CLC send wrappers when the TCP
    socket has no error recorded. Otherwise the new -ENOLINK return is
    converted to success.

Is that the whole effect? The change also covers every failed
kernel_sendmsg() where clcsock->sk->sk_err is 0.

That case is common. When tcp_sendmsg_locked() fails with EPIPE,
sk_stream_error() calls sock_error(), which clears sk_err. Timeouts and
signals (-EAGAIN, -EINTR, -ERESTARTSYS) also leave sk_err at 0.

Before this patch, those failures became reason_code 0 in
smc_clc_send_confirm() and a return value of 0 in smc_clc_send_accept(),
which callers treat as success.

Could the commit message say that ordinary TCP send failures during
CONFIRM/ACCEPT are no longer reported as success?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927074600.3694802-1-nicoyip.dev%40gmail.com