Re: [PATCH 11/11] can: isotp: publish tx.state with smp_store_release()

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 25.08.26 11:54, Jinjie Ruan wrote:
> The writer already pairs with the smp_load_acquire() readers
> in isotp_tx_timeout()/isotp_tx_gen_done(); convert
> the smp_wmb() + WRITE_ONCE() into a release store.
> 
> Assisted-by: DeepSeek:DeepSeek-V3
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   net/can/isotp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 155530aedce2..11b653ba7c10 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1156,8 +1156,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>   	my_gen = isotp_inc_tx_gen(READ_ONCE(so->tx_gen));
>   	isotp_set_tx_result(so, my_gen, ECOMM); /* prevent stale slot matching */
>   	WRITE_ONCE(so->tx_gen, my_gen);
> -	smp_wmb(); /* see smp_load_acquire() in isotp_tx_[timeout|gen_done] */
> -	WRITE_ONCE(so->tx.state, ISOTP_SENDING);
> +	/* Pairs with smp_load_acquire() in isotp_tx_[timeout|gen_done] */
> +	smp_store_release(&so->tx.state, ISOTP_SENDING);
>   	WRITE_ONCE(so->cfecho, 0);
>   	spin_unlock_bh(&so->rx_lock);
>   

Hi Jinjie,

thank you for the patch, but I think this breaks the barrier logic.
The original smp_wmb() ensures that so->tx_gen is visible before both 
subsequent writes (so->tx.state and so->cfecho).

By converting only the first write into smp_store_release(), the 
WRITE_ONCE(so->cfecho, 0) is no longer protected. The compiler or CPU 
could reorder and execute the cfecho write before the release store of 
so->tx.state, introducing a race condition with the concurrent readers.

Best regards,
Oliver