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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ab3b1f2444e3a3a6b8490255e80f952196a2c00e.camel@redhat.com>
Date:   Wed, 27 Oct 2021 17:06:30 +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 Wed, 2021-10-27 at 10:45 +0200, Paolo Abeni wrote:
> 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'm experimenting with a pktdrill test, and there is also at least
another issue: the scheduled delack is cleared before taking action. So
very likely this never worked.

I'll look for a complete fix. Thanks again for head-up and suggestion!

Paolo