linux-kernel - Re: [PATCH mptcp] mptcp: sockopt: make sync_socket_options propagate SOCK

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: <23a66a02-7de9-40c5-995d-e701cb192f8b@kernel.org>
Date: Mon, 8 Sep 2025 19:31:43 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Krister Johansen <kjlx@...pleofstupid.com>
Cc: Geliang Tang <geliang@...nel.org>, Mat Martineau <martineau@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, Florian Westphal <fw@...len.de>,
 netdev@...r.kernel.org, mptcp@...ts.linux.dev, linux-kernel@...r.kernel.org,
 David Reaver <me@...idreaver.com>
Subject: Re: [PATCH mptcp] mptcp: sockopt: make sync_socket_options propagate
 SOCK_KEEPOPEN

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@...nel.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.

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