Re: [PATCH net-next 2/4] mptcp: annotate data-races around subflow->fully_established

[Matthieu Baerts <matttbe@kernel.org>]

Hi Simon,

Thank you for the review!

On 25/10/2024 11:55, Simon Horman wrote:
> On Mon, Oct 21, 2024 at 05:14:04PM +0200, Matthieu Baerts (NGI0) wrote:
>> From: Gang Yan <yangang@kylinos.cn>
>>
>> We introduce the same handling for potential data races with the
>> 'fully_established' flag in subflow as previously done for
>> msk->fully_established.
>>
>> Additionally, we make a crucial change: convert the subflow's
>> 'fully_established' from 'bit_field' to 'bool' type. This is
>> necessary because methods for avoiding data races don't work well
>> with 'bit_field'. Specifically, the 'READ_ONCE' needs to know
>> the size of the variable being accessed, which is not supported in
>> 'bit_field'. Also, 'test_bit' expect the address of 'bit_field'.
>>
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/516
>> Signed-off-by: Gang Yan <yangang@kylinos.cn>
>> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> ...
> 
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 568a72702b080d7610425ce5c3a409c7b88da13a..a93e661ef5c435155066ce9cc109092661f0711c 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -513,7 +513,6 @@ struct mptcp_subflow_context {
>>  		request_bkup : 1,
>>  		mp_capable : 1,	    /* remote is MPTCP capable */
>>  		mp_join : 1,	    /* remote is JOINing */
>> -		fully_established : 1,	    /* path validated */
>>  		pm_notified : 1,    /* PM hook called for established status */
>>  		conn_finished : 1,
>>  		map_valid : 1,
>> @@ -532,10 +531,11 @@ struct mptcp_subflow_context {
>>  		is_mptfo : 1,	    /* subflow is doing TFO */
>>  		close_event_done : 1,       /* has done the post-closed part */
>>  		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
>> -		__unused : 8;
>> +		__unused : 9;
>>  	bool	data_avail;
>>  	bool	scheduled;
>>  	bool	pm_listener;	    /* a listener managed by the kernel PM? */
>> +	bool	fully_established;  /* path validated */
>>  	u32	remote_nonce;
>>  	u64	thmac;
>>  	u32	local_nonce;
> 
> ...
> 
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 6170f2fff71e4f9d64837f2ebf4d81bba224fafb..860903e0642255cf9efb39da9e24c39f6547481f 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -800,7 +800,7 @@ void __mptcp_subflow_fully_established(struct mptcp_sock *msk,
>>  				       const struct mptcp_options_received *mp_opt)
>>  {
>>  	subflow_set_remote_key(msk, subflow, mp_opt);
>> -	subflow->fully_established = 1;
>> +	WRITE_ONCE(subflow->fully_established, true);
>>  	WRITE_ONCE(msk->fully_established, true);
>>  
>>  	if (subflow->is_mptfo)
>> @@ -2062,7 +2062,7 @@ static void subflow_ulp_clone(const struct request_sock *req,
>>  	} else if (subflow_req->mp_join) {
>>  		new_ctx->ssn_offset = subflow_req->ssn_offset;
>>  		new_ctx->mp_join = 1;
>> -		new_ctx->fully_established = 1;
>> +		WRITE_ONCE(new_ctx->fully_established, true);
>>  		new_ctx->remote_key_valid = 1;
>>  		new_ctx->backup = subflow_req->backup;
>>  		new_ctx->request_bkup = subflow_req->request_bkup;
> 
> My understanding is that 1) fully_established is now a single byte and
> 2) WRITE_ONCE is not necessary for a single byte, as if I understand Eric's
> comment in [1] correctly, tearing is not possible in this case.

Good point, I appreciate this note, I didn't realise it was always not
necessary to use it for a single byte!

Just to be sure: is it an issue to keep them?

I mean: here, we are not in the fast path, and I think it "feels" better
to see WRITE_ONCE() being used when all the readers use READ_ONCE(). Do
you see what I mean? Not to have to think "strange, no WRITE_ONCE() here
; oh but that's fine here because it is a single byte when I look at its
definition".

Also, many other single byte variables in MPTCP structures are being
used with WRITE_ONCE(): "msk->fully_established" (used just above), but
also the other booleans declared above the new one in the subflow
context structure, and in other structures declared in protocol.h.

(Note that WRITE_ONCE() could also be a NOOP when used with a single
byte to keep the consistency, if it is always useless in this case.)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.