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]

Message-ID: <a0c568ca298714e04da75c879f28cb6e3836d813.camel@redhat.com>
Date:   Wed, 27 Oct 2021 10:45:20 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        netdev@...r.kernel.org
Cc:     Peter Krystad <peter.krystad@...ux.intel.com>,
        Florian Westphal <fw@...len.de>,
        Matthieu Baerts <matthieu.baerts@...sares.net>
Subject: Re: [PATCH net-next v3 04/17] mptcp: Add handling of outgoing
 MP_JOIN requests

On Tue, 2021-10-26 at 17:54 -0700, Eric Dumazet wrote:
> 
> 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);
>  }

Thanks Eric for catching this! We need better packetdrill coverage
here. I would be courious to learn how did you get it? 

The patch LGTM, would you formally submit it, or do you prefer we cope
with the process?

Thanks!

Paolo