Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220319110422.8261-1-zhouzhouyi@gmail.com> <CANn89iK46rw910CUJV3Kgf=M=HA32_ctd0xragwcRnHCV_VhmQ@mail.gmail.com>
 <CAABZP2yK2vCJcReJ_VvcqbkuEekvBpBJCyZ2geG=f83fv_RC=Q@mail.gmail.com>
In-Reply-To: <CAABZP2yK2vCJcReJ_VvcqbkuEekvBpBJCyZ2geG=f83fv_RC=Q@mail.gmail.com>
From:   Neal Cardwell <ncardwell@google.com>
Date:   Sat, 19 Mar 2022 09:57:28 -0400
Message-ID: <CADVnQy=shHKbvf4OZjX5-3CnFPOm3zyexbaH9XTLZBMk6pxeew@mail.gmail.com>
Subject: Re: [PATCH v2] net:ipv4: send an ack when seg.ack > snd.nxt
To:     Zhouyi Zhou <zhouzhouyi@gmail.com>
Cc:     Eric Dumazet <edumazet@google.com>,
        Florian Westphal <fw@strlen.de>,
        David Miller <davem@davemloft.net>, yoshfuji@linux-ipv6.org,
        dsahern@kernel.org, Jakub Kicinski <kuba@kernel.org>,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Wei Xu <xuweihf@ustc.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Mar 19, 2022 at 7:34 AM Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
>
> Thanks for reviewing my patch
>
> On Sat, Mar 19, 2022 at 7:14 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Sat, Mar 19, 2022 at 4:04 AM <zhouzhouyi@gmail.com> wrote:
> > >
> > > From: Zhouyi Zhou <zhouzhouyi@gmail.com>
> > >
> > > In RFC 793, page 72: "If the ACK acks something not yet sent
> > > (SEG.ACK > SND.NXT) then send an ACK, drop the segment,
> > > and return."
> > >
> > > Fix Linux's behavior according to RFC 793.
> > >
> > > Reported-by: Wei Xu <xuweihf@ustc.edu.cn>
> > > Signed-off-by: Wei Xu <xuweihf@ustc.edu.cn>
> > > Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> > > ---
> > > Thank Florian Westphal for pointing out
> > > the potential duplicated ack bug in patch version 1.
> >
> > I am travelling this week, but I think your patch is not necessary and
> > might actually be bad.
> >
> > Please provide more details of why nobody complained of this until today.
> >
> > Also I doubt you actually fully tested this patch, sending a V2 30
> > minutes after V1.
> >
> > If yes, please provide a packetdrill test.
> I am a beginner to TCP, although I have submitted once a patch to
> netdev in 2013 (aaa0c23cb90141309f5076ba5e3bfbd39544b985), this is
> first time I learned packetdrill test.
> I think I should do the packetdrill test in the coming days, and
> provide more details of how this (RFC793 related) can happen.

In addition to a packetdrill test and a more detailed analysis of how
this can happen, and the implications, I think there are at least a
few other issues that need to be considered:

(1) AFAICT, adding an unconditional ACK if (after(ack, tp->snd_nxt))
seems to open the potential for attackers to cause DoS attacks with
something like the following:

 (a) attacker injects one data packet in the A->B direction and one
data packet in the B->A direction

 (b) endpoint A sends an ACK for the forged data sent to it, which
will have an ACK beyond B's snd_nxt

 (c) endpoint B sends an ACK for the forged data sent to it, which
will have an ACK beyond A's snd_nxt

 (d) endpoint B receives the ACK sent by A, causing B to send another
ACK beyond A's snd_nxt

 (e) endpoint A receives the ACK sent by B, causing A to send another
ACK beyond B's snd_nxt

 (f) repeat (d) and (e) ad infinitum

So AFAICT an attacker could send two data packets with 1 byte of data
and cause the two endpoints to use up an unbounded amount of CPU and
bandwidth sending ACKs in an "infinite loop".

To avoid this "infinite loop" of packets, if we really need to add an
ACK in this case then the code should use the tcp_oow_rate_limited()
helper to ensure that such ACKs are rate-limited. For more context on
tcp_oow_rate_limited(), see:

f06535c599354 Merge branch 'tcp_ack_loops'
4fb17a6091674 tcp: mitigate ACK loops for connections as tcp_timewait_sock
f2b2c582e8242 tcp: mitigate ACK loops for connections as tcp_sock
a9b2c06dbef48 tcp: mitigate ACK loops for connections as tcp_request_sock
032ee4236954e tcp: helpers to mitigate ACK loops by rate-limiting
out-of-window dupacks

Note that f06535c599354 in particular mentions the case discussed in this patch:

    (2) RFC 793 (section 3.9, page 72) says: "If the ACK acknowledges
        something not yet sent (SEG.ACK > SND.NXT) then send an ACK".

(2) Please consider the potential that adding a new ACK in this
scenario may introduce new, unanticipated side channels. For more on
side channels, see:

  https://lwn.net/Articles/696868/
  The TCP "challenge ACK" side channel

  Principled Unearthing of TCP Side Channel Vulnerabilities
  https://dl.acm.org/doi/10.1145/3319535.3354250

best regards,
neal