Re: [PATCH next mptcp-net 3/3] mptcp: remove thmac from subflow ctx

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Tue, 2026-08-18 at 19:55 +0200, Matthieu Baerts (NGI0) wrote:
> This entry is only used in subflow_finish_connect().
> 
> Instead, use the original value from mp_opt, and pass it to
> subflow_thmac_valid() to do the validation with the given truncated
> hmac.
> 
> While at it, rename the variables in subflow_thmac_valid() to avoid
> confusions about the received one vs the expected one.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.h |  1 -
>  net/mptcp/subflow.c  | 18 +++++++++---------
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 7e168e450fb0..d414065d1966 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -592,7 +592,6 @@ struct mptcp_subflow_context {
>  	bool	fully_established;  /* path validated */
>  	u32	lent_mem_frag;
>  	u32	remote_nonce;
> -	u64	thmac;
>  	u32	local_nonce;
>  	u32	remote_token;
>  	union {
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 01db7edce18a..2d7ccb01d234 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -408,20 +408,21 @@ static struct dst_entry
> *subflow_v6_route_req(const struct sock *sk,
>  #endif
>  
>  /* validate received truncated hmac and create hmac for third ACK */
> -static bool subflow_thmac_valid(struct mptcp_subflow_context
> *subflow)
> +static bool subflow_thmac_valid(struct mptcp_subflow_context
> *subflow,
> +				u64 thmac)

Perhaps we could rename this parameter, say to "subflow_thmac", so that
we don't need to rename the local variable "thmac" to "expected_thmac".
What do you think?

Thanks,
-Geliang

>  {
>  	u8 hmac[SHA256_DIGEST_SIZE];
> -	u64 thmac;
> +	u64 expected_thmac;
>  
>  	subflow_generate_hmac(subflow->remote_key, subflow-
> >local_key,
>  			      subflow->remote_nonce, subflow-
> >local_nonce,
>  			      hmac);
>  
> -	thmac = get_unaligned_be64(hmac);
> -	pr_debug("subflow=%p, token=%u, thmac=%llu, subflow-
> >thmac=%llu\n",
> -		 subflow, subflow->token, thmac, subflow->thmac);
> +	expected_thmac = get_unaligned_be64(hmac);
> +	pr_debug("subflow=%p, token=%u, expected_thmac=%llu,
> thmac=%llu\n",
> +		 subflow, subflow->token, expected_thmac, thmac);
>  
> -	return thmac == subflow->thmac;
> +	return expected_thmac == thmac;
>  }
>  
>  void mptcp_subflow_reset(struct sock *ssk)
> @@ -571,14 +572,13 @@ static void subflow_finish_connect(struct sock
> *sk, const struct sk_buff *skb)
>  		}
>  
>  		subflow->backup = mp_opt.backup;
> -		subflow->thmac = mp_opt.thmac;
>  		subflow->remote_nonce = mp_opt.nonce;
>  		WRITE_ONCE(subflow->remote_id, mp_opt.join_id);
>  		pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u
> backup=%d\n",
> -			 subflow, subflow->thmac, subflow-
> >remote_nonce,
> +			 subflow, mp_opt.thmac, subflow-
> >remote_nonce,
>  			 subflow->backup);
>  
> -		if (!subflow_thmac_valid(subflow)) {
> +		if (!subflow_thmac_valid(subflow, mp_opt.thmac)) {
>  			MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNACKMAC);
>  			subflow->reset_reason = MPTCP_RST_EMPTCP;
>  			goto do_reset;