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

[Chengfeng Ye]

On Wed, Sep 30, 2026 at 8:48 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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

These pre-existing issues are variant of the data race reported by
this patch, now they are also addressed in v2
https://lore.kernel.org/netdev/20261003183325.2289707-1-nicoyip.dev@xxxxxxxxx/T/#u.

Best regards,
Chengfeng