The diag dump walks the socket hash table under a read_lock and dereferences conn->lgr and conn->lnk. Four paths expose a window where a hashed socket has freed or partially freed conn/lgr/lnk state. First, smc_conn_abort() calls smc_conn_free() during early handshake aborts while the socket is still hashed in SMC_INIT, synchronously freeing the lgr on the smc_lgr_cleanup_early() path without holding the hash write_lock. A concurrent diag reader can load a non-NULL conn->lgr and dereference lgr->is_smcd after the kfree. Skip the connection, link group and DMB attributes in __smc_diag_dump() for sockets in SMC_INIT or SMC_CLOSED, and for sockets that have fallen back to TCP, and do not look at conn->lgr to determine diag_mode in SMC_INIT. This changes the sock_diag output for such sockets. SMC_DIAG_CONNINFO, SMC_DIAG_LGRINFO and SMC_DIAG_DMBINFO are not read while a socket is in SMC_INIT or SMC_CLOSED or has fallen back to TCP, and diag_mode is SMC_DIAG_MODE_SMCR for a non-fallback SMC-D socket in SMC_INIT, as it already was before smc_conn_create() had run. smcss does not use these attributes for such sockets. A fallback socket is a plain TCP socket with no SMC connection state to report; the stale conn fields left behind by smc_conn_abort() must not be read. SMC_DIAG_FALLBACK is still reported. Use smp_load_acquire() / smp_store_release() on sk_state to order these checks on weakly ordered CPUs. Second, in smc_conn_kill() on SMC-D with dmb_nocopy, smcd_buf_detach() NULLs and frees conn->sndbuf_desc while the socket is still hashed. A concurrent diag reader entering the CONNINFO block can dereference the freed descriptor. Unhash the socket right before smcd_buf_detach(). As a result, killed ACTIVE and APPCLOSEWAIT sockets that are still open are no longer visible to smcss on loopback-ism devices. Third, in the terminal teardown branches of smc_close_active_abort() (PEERCLOSEWAIT and PROCESSABORT groups) and in smc_close_passive_work(), smc_conn_free() drops lgr and lnk references while the socket is still hashed. Unhash immediately before each smc_conn_free() call at those two sites. Sockets killed in the PEERCLOSEWAIT1/2, PEERFINCLOSEWAIT, PROCESSABORT and APPFINCLOSEWAIT states are therefore unhashed, and their inuse accounting dropped, before close(), even if the fd is still open. Whether a killed socket remains visible to smcss thus depends on its close state and, for SMC-D, on the device type. Fourth, smc_listen_out_err() sets sk_state = SMC_CLOSED with a plain store after smc_conn_abort() -> smc_conn_free() has already cleared alert_token_local under write_lock_bh(). On weakly ordered CPUs a concurrent diag reader using smp_load_acquire() on sk_state can observe SMC_CLOSED while the preceding store to alert_token_local has not yet become visible, causing it to dereference an lgr whose reference was already dropped. Use smp_store_release() to order the alert_token_local clear before the sk_state = SMC_CLOSED store. Additionally, in __smc_diag_dump(), use the cached link->ibname instead of link->smcibdev->ibdev->name to avoid a NULL pointer dereference or a use-after-free of the IB device if __smcr_link_clear() zeroes the link and the device is removed during concurrent SMC-R link failover. The link data is still read without conns_lock or send_lock, so a dump racing with a link failover may transiently report zeroed or mixed link fields (ibname, ibport, link_id, GIDs). This does not affect memory safety and the next dump reports the new link. Like the SMC netlink link dump and the tracepoints, this reports the name at link creation time and does not follow a later ib_device_rename(). Fixes: f16a7dd5cf27 ("smc: netlink interface for SMC sockets") Fixes: 9dbe086c69b8 ("net/smc: fix invalid link access in dumping SMC-R connections") Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported") Cc: stable@vger.kernel.org Signed-off-by: Mahanta Jambigi --- Changes in v13: - smc_diag.c: also skip the CONNINFO/LGRINFO/DMBINFO blocks for sockets with use_fallback set; the SMC_DIAG_FALLBACK attribute is still reported. After smc_lgr_terminate_sched() emptied lgr->list, smc_conn_free() skips smc_lgr_unregister_conn(), so a socket that then falls back and reaches SMC_ACTIVE can keep a stale alert_token_local and a diag reader could dereference the freed sndbuf/rmb/lgr; a fallback socket is a plain TCP socket with no SMC connection state to report - smc_core.c: drop the v11 and v12 changes (unconditional smc_lgr_unregister_conn() in smc_conn_free() and the !lgr->terminating check in smc_lgr_schedule_free_work()); the v12 check was racy against __smc_lgr_terminate() and the changes moved the patch away from the diag reader race, so smc_core.c is back to the v10 change only; the Fixes tag for 2a0674fffb6b added in v11 is dropped with them - commit message: state that using the cached link->ibname avoids the NULL pointer dereference and the IB device use-after-free, and that link fields may be transiently zeroed or mixed in a dump racing with a link failover, without affecting memory safety - commit message: drop the claim that other kill paths leave the socket hashed; every smc_conn_kill() ends in smc_close_active_abort(), so sockets killed in the PEERCLOSEWAIT1/2, PEERFINCLOSEWAIT, PROCESSABORT and APPFINCLOSEWAIT states are unhashed before close() even if the fd is still open, and the dmb_nocopy unhash hides ACTIVE/APPCLOSEWAIT sockets on loopback-ism; smcss visibility thus depends on close state and device type; this supersedes the v10 entry below Changes in v12: - smc_core.c: guard smc_lgr_schedule_free_work() with !lgr->terminating in addition to !lgr->freeing, preventing free_work from being re-armed on a terminating LGR after smc_lgr_cleanup_early() has called __smc_lgr_terminate() (which sets terminating=1 and cancels free_work without setting freeing=1); without this a concurrent smc_conn_free() decrementing conns_num to 0 would re-arm free_work, and the subsequent kfree(lgr) in smc_lgr_free() would leave the armed work pointing at freed memory Changes in v11: - smc_core.c: unregister connection unconditionally in smc_conn_free() rather than gating it on !list_empty(&lgr->list), ensuring alert_token_local is always reset to 0 - commit message: add Fixes tag for 2a0674fffb6b and clarify that copying cached link->ibname eliminates the NULL dereference / UAF during concurrent link clearing Changes in v10: - smc_core.c: move the unhash out of the top of smc_conn_kill() into the smc_ism_support_dmb_nocopy() branch, right before smcd_buf_detach(); the other kill paths leave ACTIVE/APPCLOSEWAIT sockets open, so they must stay visible to smcss and keep their inuse accounting - commit message: document the sock_diag output changes for sockets in SMC_INIT and SMC_CLOSED (extension attributes omitted, diag_mode of SMC-D in SMC_INIT) and that ibname is the name cached at link creation Changes in v9: - af_smc.c: change plain store in smc_listen_out_err() to smp_store_release(&newsmcsk->sk_state, SMC_CLOSED) to order the preceding alert_token_local clear (done under write_lock_bh() inside smc_conn_free()) before the sk_state write; pairs with the smp_load_acquire() in __smc_diag_dump() to close a use-after-free on weakly ordered CPUs where a diag reader could observe SMC_CLOSED while conn->lgr still appeared live Changes in v8: - smc_diag.c: use smp_load_acquire(&sk->sk_state) to prevent speculative load reordering on weakly ordered CPUs (such as arm64) during handshake abort fallback - af_smc.c: pair with smp_store_release(&sk->sk_state, SMC_ACTIVE) when transitioning out of SMC_INIT - smc_diag.c: copy cached link->ibname instead of chasing link->smcibdev->ibdev->name to prevent NULL pointer dereference / UAF if __smcr_link_clear() zeroes the link during link failover Changes in v7: - no code changes; commit message only - document the SMC_CLOSED leg of the early-exit guard and the rationale that smcss already skips CLOSED sockets on the userspace side; add Fixes: ae2be35cbed2 for the smc_conn_kill() hunk since smcd_buf_detach() was introduced by that commit Changes in v6: - smc_diag.c: inline sk->sk_state != SMC_INIT into the else-if condition so conn->lgr->is_smcd is never loaded for SMC_INIT sockets; retain the SMC_INIT || SMC_CLOSED early-exit guard and out: label from v5 to defensively skip the CONNINFO/LGRINFO/DMBINFO blocks for sockets with no live connection - smc_close.c: remove the unconditional unhash from the top of smc_close_active_abort(); place it scoped immediately before each of the two smc_conn_free() calls in the PEERCLOSEWAIT and PROCESSABORT branches, and before smc_conn_free() in smc_close_passive_work(); this avoids prematurely hiding live SMC_ACTIVE/APPCLOSEWAIT sockets from smcss - smc_core.c: unhash at the top of smc_conn_kill(), before smcd_buf_detach(), so the socket is off the hash table before conn->sndbuf_desc is freed on the dmb_nocopy path Changes in v5: - guard conn/lgr/lnk blocks in __smc_diag_dump() with an (SMC_INIT || SMC_CLOSED) state check placed after the SMC_DIAG_FALLBACK nla_put; SMC_INIT can coexist with fallback (smc_sendmsg() MSG_FASTOPEN path), so the guard must not suppress the fallback reason Changes in v4: - dropped smc_conn_unhash() wrapper, conn->unhashed flag, and all changes to af_smc.c, smc.h, smc_core.c and smc_core.h; smc_unhash_sk() is already idempotent via sk_hashed(), so direct calls at the two teardown sites in smc_close.c are sufficient - dropped the __smc_release() hunk: it needs no change since the subsequent unhash there is already a safe no-op - fixed premature-unhash issue present in v3: smc_conn_free() must not unhash because smc_conn_abort() calls it before smc_switch_to_fallback() in both smc_listen_decline() and smc_connect_rdma() error paths; unhashing there would make live fallback sockets invisible to smcss - likewise, the ISM/RDMA retry loops call smc_conn_abort() on a failed attempt and then smc_conn_create() on the next device; unhashing in smc_conn_free() would permanently hide the established connection from smc_diag since smc_conn_create() does not re-hash the socket Changes in v3: - redesigned as a single patch; dropped the lgr_lnk_lock spinlock approach and the 2-patch split Changes in v2: - split into a 2-patch series; patch 1/2 adds per-connection lgr_lnk_lock infrastructure to smc_core, patch 2/2 fixes the diag dump path using it net/smc/af_smc.c | 10 +++++----- net/smc/smc_close.c | 3 +++ net/smc/smc_core.c | 4 +++- net/smc/smc_diag.c | 13 +++++++++---- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index XXXXXXX..XXXXXXX 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -976,7 +976,7 @@ static int smc_connect_fallback(struct smc_sock *smc, int reason_code) smc_copy_sock_settings_to_clc(smc); smc->connect_nonblock = 0; if (smc->sk.sk_state == SMC_INIT) - smc->sk.sk_state = SMC_ACTIVE; + smp_store_release(&smc->sk.sk_state, SMC_ACTIVE); return 0; } @@ -1381,7 +1381,7 @@ static int smc_connect_rdma(struct smc_sock *smc, smc_copy_sock_settings_to_clc(smc); smc->connect_nonblock = 0; if (smc->sk.sk_state == SMC_INIT) - smc->sk.sk_state = SMC_ACTIVE; + smp_store_release(&smc->sk.sk_state, SMC_ACTIVE); return 0; connect_abort: @@ -1484,7 +1484,7 @@ static int smc_connect_ism(struct smc_sock *smc, smc_copy_sock_settings_to_clc(smc); smc->connect_nonblock = 0; if (smc->sk.sk_state == SMC_INIT) - smc->sk.sk_state = SMC_ACTIVE; + smp_store_release(&smc->sk.sk_state, SMC_ACTIVE); return 0; connect_abort: @@ -1951,7 +1951,7 @@ static void smc_listen_out_connected(struct smc_sock *new_smc) struct sock *newsmcsk = &new_smc->sk; if (newsmcsk->sk_state == SMC_INIT) - newsmcsk->sk_state = SMC_ACTIVE; + smp_store_release(&newsmcsk->sk_state, SMC_ACTIVE); smc_listen_out(new_smc); } @@ -1965,7 +1965,7 @@ static void smc_listen_out_err(struct smc_sock *new_smc) this_cpu_inc(net->smc.smc_stats->srv_hshake_err_cnt); if (newsmcsk->sk_state == SMC_INIT) sock_put(&new_smc->sk); /* passive closing */ - newsmcsk->sk_state = SMC_CLOSED; + smp_store_release(&newsmcsk->sk_state, SMC_CLOSED); smc_listen_out(new_smc); } diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c index XXXXXXX..XXXXXXX 100644 --- a/net/smc/smc_close.c +++ b/net/smc/smc_close.c @@ -154,6 +154,7 @@ void smc_close_active_abort(struct smc_sock *smc) if (sk->sk_state != SMC_PEERABORTWAIT) break; sk->sk_state = SMC_CLOSED; + sk->sk_prot->unhash(sk); smc_conn_free(&smc->conn); release_clcsock = true; sock_put(sk); /* passive closing */ @@ -165,6 +166,7 @@ void smc_close_active_abort(struct smc_sock *smc) if (sk->sk_state != SMC_PEERABORTWAIT) break; sk->sk_state = SMC_CLOSED; + sk->sk_prot->unhash(sk); smc_conn_free(&smc->conn); release_clcsock = true; break; @@ -433,6 +435,7 @@ wakeup: sk->sk_state_change(sk); if ((sk->sk_state == SMC_CLOSED) && (sock_flag(sk, SOCK_DEAD) || !sk->sk_socket)) { + sk->sk_prot->unhash(sk); smc_conn_free(conn); if (smc->clcsock) release_clcsock = true; diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c index XXXXXXX..XXXXXXX 100644 --- a/net/smc/smc_core.c +++ b/net/smc/smc_core.c @@ -1530,8 +1530,10 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft) tasklet_kill(&conn->rx_tsklet); else tasklet_unlock_wait(&conn->rx_tsklet); - if (smc_ism_support_dmb_nocopy(conn->lgr->smcd)) + if (smc_ism_support_dmb_nocopy(conn->lgr->smcd)) { + smc->sk.sk_prot->unhash(&smc->sk); smcd_buf_detach(conn); + } } else { smc_cdc_wait_pend_tx_wr(conn); } diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c index XXXXXXX..XXXXXXX 100644 --- a/net/smc/smc_diag.c +++ b/net/smc/smc_diag.c @@ -87,10 +87,11 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, r = nlmsg_data(nlh); smc_diag_msg_common_fill(r, sk); - r->diag_state = sk->sk_state; + r->diag_state = smp_load_acquire(&sk->sk_state); if (smc->use_fallback) r->diag_mode = SMC_DIAG_MODE_FALLBACK_TCP; - else if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd) + else if (r->diag_state != SMC_INIT && + smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd) r->diag_mode = SMC_DIAG_MODE_SMCD; else r->diag_mode = SMC_DIAG_MODE_SMCR; @@ -103,6 +104,10 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, if (nla_put(skb, SMC_DIAG_FALLBACK, sizeof(fallback), &fallback) < 0) goto errout; + if (r->diag_state == SMC_INIT || r->diag_state == SMC_CLOSED || + smc->use_fallback) + goto out; + if ((req->diag_ext & (1 << (SMC_DIAG_CONNINFO - 1))) && smc->conn.alert_token_local) { struct smc_connection *conn = &smc->conn; @@ -154,8 +159,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, .lnk[0].link_id = link->link_id, }; - memcpy(linfo.lnk[0].ibname, link->smcibdev->ibdev->name, - sizeof(link->smcibdev->ibdev->name)); + memcpy(linfo.lnk[0].ibname, link->ibname, sizeof(link->ibname)); smc_gid_be16_convert(linfo.lnk[0].gid, link->gid); smc_gid_be16_convert(linfo.lnk[0].peer_gid, link->peer_gid); @@ -185,6 +189,7 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, goto errout; } +out: nlmsg_end(skb, nlh); return 0; -- 2.50.1 (Apple Git-155)