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

[Jinjie Ruan <ruanjinjie@huawei.com>]

在 2026/8/25 19:46, Oliver Hartkopp 写道:
> 
> 
> 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,
> 

Hi Oliver,

> 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).

Right!

> 
> 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

Indeed, that's true.

> so->tx.state, introducing a race condition with the concurrent readers.

My rough understanding is as follows:

All lock-free readers fall into two disjoint sets:

- `isotp_tx_timeout()` and `isotp_tx_gen_done()`: read only `tx.state`
(acquire) and `tx_gen`

- `isotp_txfr_timer_handler()` the timer path of `isotp_send_cframe()`,
and the post-claim path of `isotp_sendmsg()` touch `cfecho` but never
`tx_gen`.

So no lock-free reader observes both `tx_gen` and `cfecho`.

`isotp_rcv_echo()` is the only function reading both, and it runs under
`so->rx_lock`, which serializes it with the claim.

So the ordering the `smp_wmb()` provided on top of the new release store
`tx_gen` before `cfecho` — is unobservable to every reader.

Moreover, `tx.state` and `cfecho` were never ordered against each other
by the original barrier: both followed the `smp_wmb()`, so the `(state,
cfecho)` visibility seen by the lock-free timer readers is bit-for-bit
identical before and after this change.

So the release store preserves the one ordering that matters: a reader
observing `ISOTP_SENDING` sees the new `tx_gen`.

Best regards,
Jinjie

> 
> Best regards,
> Oliver
> 
>