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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241025095548.GM1202098@kernel.org>
Date: Fri, 25 Oct 2024 10:55:48 +0100
From: Simon Horman <horms@...nel.org>
To: "Matthieu Baerts (NGI0)" <matttbe@...nel.org>
Cc: mptcp@...ts.linux.dev, Mat Martineau <martineau@...nel.org>,
	Geliang Tang <geliang@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Gang Yan <yangang@...inos.cn>
Subject: Re: [PATCH net-next 2/4] mptcp: annotate data-races around
 subflow->fully_established

On Mon, Oct 21, 2024 at 05:14:04PM +0200, Matthieu Baerts (NGI0) wrote:
> From: Gang Yan <yangang@...inos.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@...inos.cn>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.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.

[1] https://lore.kernel.org/netdev/CANn89i+8myPgn61bn7DBqcnK5kXX2XvPo2oc2TfzntPUkeqQ6w@mail.gmail.com/