Re: [PATCH net v2 1/2] net/smc: add connection lifetime infrastructure for diag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Setting `conn->lnk = NULL` and `conn->lgr = NULL` in `smc_conn_free()` introduces fatal NULL pointer dereferences in concurrent and deferred readers.
- [Medium] `conn->lnk` is modified in `smc_cdc_msg_validate()` without holding the newly introduced `lgr_lnk_lock`, violating the stated synchronization invariants.
--

commit 340542b4422ee1313d9e6622b416469a93adf1be
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>

net/smc: add connection lifetime infrastructure for diag

Add the basic lifetime synchronization needed by the diag dump path.
Introduce a per-connection lock that serializes connection-owned
conn->lgr/conn->lnk updates against readers. In smc_conn_free(), NULL out
conn->lgr and conn->lnk under lgr_lnk_lock before releasing references to
close the window where a concurrent diag reader could observe freed memory.

> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 427b6d63b9931..f5ed51d8eb939 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -232,6 +232,7 @@ struct smc_connection {
>  	atomic_t		sndbuf_space;	/* remaining space in sndbuf */
>  	u16			tx_cdc_seq;	/* sequence # for CDC send */
>  	u16			tx_cdc_seq_fin;	/* sequence # - tx completed */
> +	spinlock_t		lgr_lnk_lock;	/* protect conn owned lgr/lnk refs */

[Severity: Medium]
If this lock is required for all updates to conn->lnk as stated in the
commit message, should we also take it in
net/smc/smc_cdc.c:smc_cdc_msg_validate()?

net/smc/smc_cdc.c:smc_cdc_msg_validate() {
        ...
        conn->out_of_sync = 1;
        spin_lock_bh(&conn->send_lock);
        conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
        conn->lnk = link;
        spin_unlock_bh(&conn->send_lock);
        ...
}

Without lgr_lnk_lock here, concurrent diag readers could observe an
inconsistent link state.

>  	spinlock_t		send_lock;	/* protect wr_sends */
>  	atomic_t		cdc_pend_tx_wr; /* number of pending tx CDC wqe
>  						 * - inc when post wqe,

[ ... ]

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 04aedd957543a..5af55abece577 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
[ ... ]
> @@ -1287,8 +1290,13 @@ void smc_conn_free(struct smc_connection *conn)
>  	if (!lgr->conns_num)
>  		smc_lgr_schedule_free_work(lgr);
>  lgr_put:
> +	spin_lock_bh(&conn->lgr_lnk_lock);
> +	lnk = conn->lnk;
> +	conn->lnk = NULL;
> +	conn->lgr = NULL;

[Severity: Critical]
Does setting these to NULL here introduce NULL pointer dereferences in
existing paths that don't take lgr_lnk_lock?

For example, in net/smc/smc_core.c:smc_conn_kill(), if smc_conn_free()
sets conn->lgr to NULL, and a concurrent smc_lgr_terminate_work() is
waiting for the socket lock:

smc_conn_free()
  conn->lgr = NULL;

smc_lgr_terminate_work()
  smc_conn_kill()
    if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)

Similarly, a deferred release callback in net/smc/smc_tx.c can race:

smc_release_cb()
  smc_tx_sndbuf_nonempty()
    if (conn->lgr->is_smcd)

Also, concurrent threads calling smc_switch_conns() in
net/smc/smc_core.c can race and dereference the newly NULL conn->lnk:

smc_switch_conns()
  smc_switch_link_and_count()
    atomic_dec(&conn->lnk->conn_cnt);

> +	spin_unlock_bh(&conn->lgr_lnk_lock);
>  	if (!lgr->is_smcd)
> -		smcr_link_put(conn->lnk); /* link_hold in smc_conn_create() */
> +		smcr_link_put(lnk); /* link_hold in smc_conn_create() */
>  	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828065439.3582783-1-mjambigi@linux.ibm.com?part=1