netdev - Re: [PATCH net-next 1/2] tcp: notify when a delayed ack is sent

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: <DF94F008-2AF5-4B5B-A282-C73B73DD2147@fb.com>
Date:   Mon, 2 Jul 2018 21:24:19 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Neal Cardwell <ncardwell@...gle.com>
CC:     Netdev <netdev@...r.kernel.org>, Kernel Team <Kernel-team@...com>,
        "Blake Matheny" <bmatheny@...com>, Alexei Starovoitov <ast@...com>,
        Yuchung Cheng <ycheng@...gle.com>,
        Steve Ibanez <sibanez@...nford.edu>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next 1/2] tcp: notify when a delayed ack is sent

On 7/2/18, 8:18 AM, "netdev-owner@...r.kernel.org on behalf of Neal Cardwell" <netdev-owner@...r.kernel.org on behalf of ncardwell@...gle.com> wrote:

    On Fri, Jun 29, 2018 at 9:48 PM Lawrence Brakmo <brakmo@...com> wrote:
    >
    > DCTCP depends on the CA_EVENT_NON_DELAYED_ACK and CA_EVENT_DELAYED_ACK
    > notifications to keep track if it needs to send an ACK for packets that
    > were received with a particular ECN state but whose ACK was delayed.
    >
    > Under some circumstances, for example when a delayed ACK is sent with a
    > data packet, DCTCP state was not being updated due to a lack of
    > notification that the previously delayed ACK was sent. As a result, it
    > would sometimes send a duplicate ACK when a new data packet arrived.
    >
    > This patch insures that DCTCP's state is correctly updated so it will
    > not send the duplicate ACK.
    >
    > Signed-off-by: Lawrence Brakmo <brakmo@...com>
    > ---
    >  net/ipv4/tcp_output.c | 2 ++
    >  1 file changed, 2 insertions(+)
    >
    > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
    > index f8f6129160dd..41f6ad7a21e4 100644
    > --- a/net/ipv4/tcp_output.c
    > +++ b/net/ipv4/tcp_output.c
    > @@ -172,6 +172,8 @@ static inline void tcp_event_ack_sent(struct sock *sk, unsigned int pkts)
    >                         __sock_put(sk);
    >         }
    >         tcp_dec_quickack_mode(sk, pkts);
    > +       if (inet_csk_ack_scheduled(sk))
    > +               tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
    >         inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
    >  }
    
    Thanks for this fix! Seems like this would work, but if I am reading
    this correctly then it seems like this would cause a duplicate call to
    tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK) when we are sending a pure
    ACK (delayed or non-delayed):
    
    (1) once from tcp_send_ack() before we send the ACK:
    
    tcp_send_ack(struct sock *sk)
     ->        tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
    
    (2) then again from tcp_event_ack_sent() after we have sent the ACK:
    
    tcp_event_ack_sent()
        ->   if (inet_csk_ack_scheduled(sk))
                     tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
    
    What if we remove the original CA_EVENT_NON_DELAYED_ACK call and just
    replace it with your new one? (not compiled, not tested):
    
    diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
    index 3889dcd4868d4..bddb49617d9be 100644
    --- a/net/ipv4/tcp_output.c
    +++ b/net/ipv4/tcp_output.c
    @@ -184,6 +184,8 @@ static inline void tcp_event_ack_sent(struct sock
    *sk, unsigned int pkts)
                            __sock_put(sk);
            }
            tcp_dec_quickack_mode(sk, pkts);
    +       if (inet_csk_ack_scheduled(sk))
    +               tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
            inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
     }
    
    @@ -3836,8 +3838,6 @@ void tcp_send_ack(struct sock *sk)
            if (sk->sk_state == TCP_CLOSE)
                    return;
    
    -       tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
    -
            /* We are not putting this on the write queue, so
             * tcp_transmit_skb() will set the ownership to this
             * sock.
    
    Aside from lower CPU overhead, one nice benefit of that is that we
    then only call tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK) in one
    place, which might be a little easier to reason about.
    
    Does that work?
    
    neal

Thanks Neal, good catch!  I will resubmit the patch.