Re: [PATCH net 02/14] mptcp: subflow: no need to copy thmac during ulp_clone

[Matthieu Baerts]

Hi Jakub,

On 27/08/2026 21:07, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.

Thank you for having forwarded this, I didn't manage to look at it before.

> ---
> mptcp: subflow: no need to copy thmac during ulp_clone
> 
> This drops the copy of subflow_req->thmac into the new subflow context in
> subflow_ulp_clone(), since the truncated HMAC is no longer needed once the
> passive-side request is over.  The commit message notes that thmac is left
> uninitialised on the SYN cookie path, so removing the copy also avoids a
> KMSAN uninitialised-read report.
> 
> (no Link: tags in the commit header)
> 
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index af81ad5e699d1..01db7edce18a6 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -2084,7 +2084,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
>>  		new_ctx->request_bkup = subflow_req->request_bkup;
>               ^^^^
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but while looking at the
> neighbouring store: does request_bkup have exactly the same
> uninitialised-on-SYN-cookies problem that motivated removing the thmac
> copy here?
> 
> subflow_init_req() only clears a subset of the fields:
> 
> net/mptcp/subflow.c:subflow_init_req() {
> 	subflow_req->mp_capable = 0;
> 	subflow_req->mp_join = 0;
> 	subflow_req->csum_reqd = mptcp_is_checksum_enabled(sock_net(sk_listener));
> 	subflow_req->allow_join_id0 = mptcp_allow_join_id0(sock_net(sk_listener));
> 	subflow_req->msk = NULL;
> 	mptcp_token_init_request(req);
> }
> 
> and the only assignment of request_bkup for a request sock happens on the
> SYN, in subflow_token_join_request():
> 
> 	subflow_req->local_id = local_id;
> 	subflow_req->request_bkup = mptcp_pm_is_backup(msk, (struct sock_common *)req);
> 
> On the cookie ACK path that function is not called, and the state restore
> does not cover request_bkup either:
> 
> net/mptcp/syncookies.c:mptcp_token_join_cookie_init_state() {
> 	subflow_req->remote_nonce = e->remote_nonce;
> 	subflow_req->local_nonce = e->local_nonce;
> 	subflow_req->backup = e->backup;
> 	subflow_req->remote_id = e->join_id;
> 	subflow_req->local_id = e->local_id;
> 	subflow_req->token = e->token;
> 	subflow_req->msk = msk;
> }
> 
> struct join_entry has no request_bkup member at this commit, so the bit
> carries whatever was in the freshly allocated request sock.
> 
> Unlike thmac, the copied value is consumed afterwards, for instance in
> mptcp_subflow_get_send() and mptcp_subflow_get_retrans():
> 
> net/mptcp/protocol.c:mptcp_subflow_get_send() {
> 	bool backup = subflow->backup || subflow->request_bkup;
> 
> It is also put on the wire by mptcp_established_options_mp_prio() and
> exported to userspace by the mptcp diag code.
> 
> So when a remote MP_JOIN is accepted while tcp_syncookies is active, can a
> joined subflow end up randomly flagged as backup and reported that way in
> diag, along with the same class of KMSAN uninit-value branch report that
> this patch removes for thmac?
> 
> For the record, the next patch in this series, "mptcp: syncookies: remember
> the request backup flag", adds request_bkup:1 to struct join_entry and
> stores/restores it, and the store happens after subflow_token_join_request()
> has set the field (subflow_token_join_request() runs before
> subflow_init_req_cookie_join_save() in subflow_check_req()), so the field is
> properly initialised at the end of the series.

All of this to say that it is fixed in the next patch...

> Would it be worth mentioning
> that dependency here, given this patch carries a Cc: stable tag?

Do we really need to mention that?? This report says "there is a
pre-existing bug in the code around", then there is no direct
dependence. Or should we add for each patch: other fixes follow this one?

Note that there is an issue on Sashiko's side to improve that:

https://github.com/sashiko-dev/sashiko/issues/392

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