Re: [PATCH net v2 1/2] net/smc: stop killed, freed and out_of_sync sharing a byte

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

Pre-existing issues:
- [High] Concurrent Read-Modify-Write data race on `struct smc_link_group` bitfields (`sync_err`, `terminating`, `freeing`).
- [High] Read-modify-write (RMW) data race on `conn_state_flags` and `prod_flags` bitfields in `struct smc_connection`.
--

--- Patch [1]: [PATCH net v2 1/2] net/smc: stop killed, freed and out_of_sync sharing a byte ---
commit a4fa7f6a3c11e3e694e6c9235dfa2ed1bd680cdf
Author: Hidayath Khan <[email protected]>

net/smc: stop killed, freed and out_of_sync sharing a byte

The commit changes three connection state flags from single-bit bitfields to
separate u8 fields. This prevents read-modify-write data races between
process context and the receive tasklet.

> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 52145df83f6e7..427b6d63b9931 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -277,9 +277,9 @@ struct smc_connection {
>  						 * 0 for SMC-R, 32 for SMC-D
>  						 */
>  	u64			peer_token;	/* SMC-D token of peer */
> -	u8			killed : 1;	/* abnormal termination */
> -	u8			freed : 1;	/* normal termination */
> -	u8			out_of_sync : 1; /* out of sync with peer */
> +	u8			killed;		/* abnormal termination */
> +	u8			freed;		/* normal termination */
> +	u8			out_of_sync;	/* out of sync with peer */
>  };

[Severity: High]
This is a pre-existing issue, but does a similar RMW data race exist for the
conn_state_flags and prod_flags bitfields embedded in struct smc_connection?

These bitfields are defined in net/smc/smc.h:

struct smc_cdc_conn_state_flags {
#if defined(__BIG_ENDIAN_BITFIELD)
	u8	peer_done_writing : 1;
	u8	peer_conn_closed : 1;
	u8	peer_conn_abort : 1;

Process context modifies these fields holding only lock_sock(), which doesn't
disable softirqs. For instance, smc_close_wr() in net/smc/smc_close.c sets
peer_done_writing = 1:

    conn->local_tx_ctrl.conn_state_flags.peer_done_writing = 1;

And smc_tx_consumer_update() in net/smc/smc_tx.c sets write_blocked = 0:

    conn->local_rx_ctrl.prod_flags.write_blocked = 0;

Meanwhile, tasklet context modifies the same bytes. smc_cdc_msg_validate()
in net/smc/smc_cdc.c sets peer_conn_abort = 1 under send_lock:

    conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;

And smc_cdc_msg_to_host() writes the entire prod_flags byte.

Since lock_sock() does not serialize with bh_lock_sock() or send_lock, could a
softirq interrupt the process context's RMW operation, resulting in dropped
connection state changes or stalled connections when the stale byte is written
back?

[Severity: High]
This is a pre-existing issue, but are there similar data races in
struct smc_link_group in net/smc/smc_core.h?

The sync_err, terminating, and freeing flags share a single byte:

struct smc_link_group {
    ...
    u8 sync_err : 1;
    u8 terminating : 1;
    u8 freeing : 1;
    ...
};

Could a read-modify-write data race occur here since they are modified from
different contexts without a common lock?

For example, smc_clc_wait_msg() in net/smc/smc_clc.c sets sync_err = 1
holding only the socket lock:

    smc->conn.lgr->sync_err = 1;

While __smc_lgr_terminate() in net/smc/smc_core.c sets terminating = 1
without holding any link group lock:

    lgr->terminating = 1;

And smc_lgr_terminate_sched() sets freeing = 1 under smc_lgr_list.lock:

    lgr->freeing = 1;

If an RMW operation is interrupted, could the terminating flag update be lost,
potentially leading to __smc_lgr_terminate() executing twice concurrently and
causing a double-free of the link group?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1