netdev - Re: [PATCH net-next v2 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: <EF9FC76A-0DF8-4972-BF7D-399BD0A6D46A@fb.com>
Date:   Tue, 3 Jul 2018 15:15:46 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Yuchung Cheng <ycheng@...gle.com>
CC:     netdev <netdev@...r.kernel.org>, Kernel Team <Kernel-team@...com>,
        "Blake Matheny" <bmatheny@...com>, Alexei Starovoitov <ast@...com>,
        Neal Cardwell <ncardwell@...gle.com>,
        Steve Ibanez <sibanez@...nford.edu>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next v2 1/2] tcp: notify when a delayed ack is sent

On 7/2/18, 4:50 PM, "Yuchung Cheng" <ycheng@...gle.com> wrote:

    On Mon, Jul 2, 2018 at 2:39 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.
    Sorry to chime-in late here (lame excuse: IETF deadline)
    
    IIRC this issue would exist prior to 4.11 kernel. While it'd be good
    to fix that, it's not clear which patch introduced the regression
    between 4.11 and 4.16? I assume you tested Eric's most recent quickack
    fix.
    
    In terms of the fix itself, it seems odd the tcp_send_ack() call in
    DCTCP generates NON_DELAYED_ACK event to toggle DCTCP's
    delayed_ack_reserved bit. Shouldn't the fix to have DCTCP send the
    "prior" ACK w/o cancelling delayed-ACK and mis-recording that it's
    cancelled, because that prior-ACK really is a supplementary old ACK.
    
    But it's still unclear how this bug introduces the regression 4.11 - 4.16
   
Feedback is always appreciated! This issue is also present in 4.11 (that is where I discovered). I think the bug was introduces much earlier.

Yes, I tested with Eric's quickack fix, it did not fix either of the two issues that are fixed with this patch set.

As I mentioned earlier, the bug was introduced before 4.11. I am not sure I understand your comments. Yes, at some level it would make sense to change the delayed_ack_reserved bit directly, but we would still need to do it whenever we send the ACK, so I do not think it can be helped. Please clarify if I misunderstood your comment.
    
    >
    > Improved based on comments from Neal Cardwell <ncardwell@...gle.com>.
    >
    > Signed-off-by: Lawrence Brakmo <brakmo@...com>
    > ---
    >  net/ipv4/tcp_output.c | 4 ++--
    >  1 file changed, 2 insertions(+), 2 deletions(-)
    >
    > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
    > index f8f6129160dd..acefb64e8280 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);
    >  }
    >
    > @@ -3567,8 +3569,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.
    > --
    > 2.17.1
    >