Re: [PATCH mptcp] mptcp: sockopt: make sync_socket_options propagate SOCK_KEEPOPEN

[Krister Johansen <kjlx@templeofstupid.com>]

On Mon, Sep 08, 2025 at 07:31:43PM +0200, Matthieu Baerts wrote:
> Hi Krister,
> 
> On 08/09/2025 19:25, Krister Johansen wrote:
> > On Mon, Sep 08, 2025 at 07:13:12PM +0200, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> On 07/09/2025 02:51, Geliang Tang wrote:
> >>> Hi Matt,
> >>>
> >>> On Sat, 2025-09-06 at 15:26 +0200, Matthieu Baerts wrote:
> >>>> Hi Krister,
> >>>>
> >>>> On 06/09/2025 02:43, Krister Johansen wrote:
> >>>>> Users reported a scenario where MPTCP connections that were
> >>>>> configured
> >>>>> with SO_KEEPALIVE prior to connect would fail to enable their
> >>>>> keepalives
> >>>>> if MTPCP fell back to TCP mode.
> >>>>>
> >>>>> After investigating, this affects keepalives for any connection
> >>>>> where
> >>>>> sync_socket_options is called on a socket that is in the closed or
> >>>>> listening state.  Joins are handled properly. For connects,
> >>>>> sync_socket_options is called when the socket is still in the
> >>>>> closed
> >>>>> state.  The tcp_set_keepalive() function does not act on sockets
> >>>>> that
> >>>>> are closed or listening, hence keepalive is not immediately
> >>>>> enabled.
> >>>>> Since the SO_KEEPOPEN flag is absent, it is not enabled later in
> >>>>> the
> >>>>> connect sequence via tcp_finish_connect.  Setting the keepalive via
> >>>>> sockopt after connect does work, but would not address any
> >>>>> subsequently
> >>>>> created flows.
> >>>>>
> >>>>> Fortunately, the fix here is straight-forward: set SOCK_KEEPOPEN on
> >>>>> the
> >>>>> subflow when calling sync_socket_options.
> >>>>>
> >>>>> The fix was valdidated both by using tcpdump to observe keeplaive
> >>>>> packets not being sent before the fix, and being sent after the
> >>>>> fix.  It
> >>>>> was also possible to observe via ss that the keepalive timer was
> >>>>> not
> >>>>> enabled on these sockets before the fix, but was enabled
> >>>>> afterwards.
> >>>>
> >>>>
> >>>> Thank you for the fix! Indeed, the SOCK_KEEPOPEN flag was missing!
> >>>> This
> >>>> patch looks good to me as well:
> >>>>
> >>>> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> >>>>
> >>>>
> >>>> @Netdev Maintainers: please apply this patch in 'net' directly. But I
> >>>> can always re-send it later if preferred.
> >>>
> >>> nit:
> >>>
> >>> I just noticed his patch breaks 'Reverse X-Mas Tree' order in
> >>> sync_socket_options(). If you think any changes are needed, please
> >>> update this when you re-send it.
> >>
> >> Sure, I can do the modification and send it with other fixes we have.
> > 
> > Thanks for the reviews, Geliang and Matt.  If you'd like me to fix the
> > formatting up and send a v2, I'm happy to do that as well.  Just let me
> > know.
> 
> I was going to apply this diff:
> 
> > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> > index 13108e9f982b..2abe6f1e9940 100644
> > --- a/net/mptcp/sockopt.c
> > +++ b/net/mptcp/sockopt.c
> > @@ -1532,11 +1532,12 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
> >  {
> >         static const unsigned int tx_rx_locks = SOCK_RCVBUF_LOCK | SOCK_SNDBUF_LOCK;
> >         struct sock *sk = (struct sock *)msk;
> > -       int kaval = !!sock_flag(sk, SOCK_KEEPOPEN);
> > +       bool keep_open;
> >  
> > +       keep_open = sock_flag(sk, SOCK_KEEPOPEN);
> >         if (ssk->sk_prot->keepalive)
> > -               ssk->sk_prot->keepalive(ssk, kaval);
> > -       sock_valbool_flag(ssk, SOCK_KEEPOPEN, kaval);
> > +               ssk->sk_prot->keepalive(ssk, keep_open);
> > +       sock_valbool_flag(ssk, SOCK_KEEPOPEN, keep_open);
> >  
> >         ssk->sk_priority = sk->sk_priority;
> >         ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
> 
> (sock_flag() returns a bool, and 'keep_open' is maybe clearer)
> 
> But up to you, I really don't mind if you prefer to send the v2 by
> yourself, just let me know.

Thanks, I'll go ahead and amend as you suggest and then send a v2.

-K