Re: [MPTCP next v3 09/12] mptcp: cleanup fallback dummy mapping generation.

[Geliang Tang <geliang@kernel.org>]

On Sat, 2025-09-20 at 08:06 +0800, Geliang Tang wrote:
> On Fri, 2025-09-19 at 17:53 +0200, Paolo Abeni wrote:
> > MPTCP currently access ack_seq outside the msk socket log scope to
> > generate the dummy mapping for fallback socket. Soon we are going
> > to introduce backlog usage and even for fallback socket the ack_seq
> > value will be significantly off outside of the msk socket lock
> > scope.
> > 
> > Avoid relying on ack_seq for dummy mapping generation, using
> > instead
> > the subflow sequence number. Note that in case of disconnect() and
> > (re)connect() we must ensure that any previous state is re-set.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v2 -> v3:
> >  - reordered before the backlog introduction to avoid transiently
> >    break the fallback
> >  - explicitly reset ack_seq
> > ---
> >  net/mptcp/protocol.c | 3 +++
> >  net/mptcp/subflow.c  | 8 +++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index fce70cdad2a7f..c8b02048126a9 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -3224,6 +3224,9 @@ static int mptcp_disconnect(struct sock *sk,
> > int flags)
> >  	msk->bytes_retrans = 0;
> >  	msk->rcvspace_init = 0;
> >  
> > +	/* for fallback's sake */
> > +	WRITE_ONCE(msk->ack_seq, 0);
> > +
> >  	WRITE_ONCE(sk->sk_shutdown, 0);
> >  	sk_error_report(sk);
> >  	return 0;
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index b9455c04e8a46..ac8616e7521e8 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -491,6 +491,9 @@ static void subflow_set_remote_key(struct
> > mptcp_sock *msk,
> >  	mptcp_crypto_key_sha(subflow->remote_key, NULL, &subflow-
> > > iasn);
> >  	subflow->iasn++;
> >  
> > +	/* for fallback's sake */
> > +	subflow->map_seq = subflow->iasn;
> > +
> >  	WRITE_ONCE(msk->remote_key, subflow->remote_key);
> >  	WRITE_ONCE(msk->ack_seq, subflow->iasn);
> >  	WRITE_ONCE(msk->can_ack, true);
> > @@ -1435,9 +1438,12 @@ static bool subflow_check_data_avail(struct
> > sock *ssk)
> >  
> >  	skb = skb_peek(&ssk->sk_receive_queue);
> >  	subflow->map_valid = 1;
> > -	subflow->map_seq = READ_ONCE(msk->ack_seq);
> 
> nit:
> 
> How about replacing this line in place? I mean, do not move it to a
> later position:
> 
> +	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
> +					      subflow->iasn +
> +					      TCP_SKB_CB(skb)->seq -
> +					      subflow->ssn_offset -
> 1);
> 
> Reviewed-by: Geliang Tang <geliang@kernel.org>
> Tested-by: Geliang Tang <geliang@kernel.org>

Please delete the sentence in the subject of this patch too.

Thanks,
-Geliang

> 
> Thanks,
> -Geliang
> 
> >  	subflow->map_data_len = skb->len;
> >  	subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq -
> > subflow->ssn_offset;
> > +	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
> > +					      subflow->iasn +
> > +					      TCP_SKB_CB(skb)->seq
> > -
> > +					      subflow->ssn_offset
> > -
> > 1);
> >  	WRITE_ONCE(subflow->data_avail, true);
> >  	return true;
> >  }