netdev - Re: [PATCH net-next v3 04/17] mptcp: Add handling of outgoing MP

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]

Date:   Tue, 26 Oct 2021 17:54:13 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        netdev@...r.kernel.org
Cc:     Peter Krystad <peter.krystad@...ux.intel.com>,
        eric.dumazet@...il.com, Florian Westphal <fw@...len.de>,
        Paolo Abeni <pabeni@...hat.com>,
        Matthieu Baerts <matthieu.baerts@...sares.net>
Subject: Re: [PATCH net-next v3 04/17] mptcp: Add handling of outgoing MP_JOIN
 requests



On 3/27/20 2:48 PM, Mat Martineau wrote:
> From: Peter Krystad <peter.krystad@...ux.intel.com>
> 
> Subflow creation may be initiated by the path manager when
> the primary connection is fully established and a remote
> address has been received via ADD_ADDR.
> 
> Create an in-kernel sock and use kernel_connect() to
> initiate connection.
> 
> Passive sockets can't acquire the mptcp socket lock at
> subflow creation time, so an additional list protected by
> a new spinlock is used to track the MPJ subflows.
> 
> Such list is spliced into conn_list tail every time the msk
> socket lock is acquired, so that it will not interfere
> with data flow on the original connection.
> 
> Data flow and connection failover not addressed by this commit.
> 
> Co-developed-by: Florian Westphal <fw@...len.de>
> Signed-off-by: Florian Westphal <fw@...len.de>
> Co-developed-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> Co-developed-by: Matthieu Baerts <matthieu.baerts@...sares.net>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@...sares.net>
> Signed-off-by: Peter Krystad <peter.krystad@...ux.intel.com>
> Signed-off-by: Mat Martineau <mathew.j.martineau@...ux.intel.com>
> ---

...

> +/* MP_JOIN client subflow must wait for 4th ack before sending any data:
> + * TCP can't schedule delack timer before the subflow is fully established.
> + * MPTCP uses the delack timer to do 3rd ack retransmissions
> + */
> +static void schedule_3rdack_retransmission(struct sock *sk)
> +{
> +	struct inet_connection_sock *icsk = inet_csk(sk);
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	unsigned long timeout;
> +
> +	/* reschedule with a timeout above RTT, as we must look only for drop */
> +	if (tp->srtt_us)
> +		timeout = tp->srtt_us << 1;

srtt_us is in usec/8 units.

> +	else
> +		timeout = TCP_TIMEOUT_INIT;

TCP_TIMEOUT_INIT is in HZ units.


> +
> +	WARN_ON_ONCE(icsk->icsk_ack.pending & ICSK_ACK_TIMER);
> +	icsk->icsk_ack.pending |= ICSK_ACK_SCHED | ICSK_ACK_TIMER;
> +	icsk->icsk_ack.timeout = timeout;

Usually, we have to use jiffies as well...

> +	sk_reset_timer(sk, &icsk->icsk_delack_timer, timeout);
> +}
> +
> 

I wonder if this delack_timer ever worked.

What about this fix ?

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 422f4acfb3e6d6d41f6f5f820828eaa40ffaa6b9..9f5edcf562c9f98539256074b8f587c0a64a8693 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -434,12 +434,13 @@ static void schedule_3rdack_retransmission(struct sock *sk)
 
        /* reschedule with a timeout above RTT, as we must look only for drop */
        if (tp->srtt_us)
-               timeout = tp->srtt_us << 1;
+               timeout = usecs_to_jiffies(tp->srtt_us >> (3-1));
        else
                timeout = TCP_TIMEOUT_INIT;
 
        WARN_ON_ONCE(icsk->icsk_ack.pending & ICSK_ACK_TIMER);
        icsk->icsk_ack.pending |= ICSK_ACK_SCHED | ICSK_ACK_TIMER;
+       timeout += jiffies;
        icsk->icsk_ack.timeout = timeout;
        sk_reset_timer(sk, &icsk->icsk_delack_timer, timeout);
 }