netdev - Re: [PATCH net-next] net: don't keep lonely packets forever in the gro hash

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] [day] [month] [year] [list]

Message-ID: <CAF=yD-KL-cObamftA8c3VaOmG3Rg6x5Tce9YHq++boT7K23bCg@mail.gmail.com>
Date:   Tue, 20 Nov 2018 20:57:47 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net-next] net: don't keep lonely packets forever in the
 gro hash

On Tue, Nov 20, 2018 at 1:19 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> Eric noted that with UDP GRO and NAPI timeout, we could keep a single
> UDP packet inside the GRO hash forever, if the related NAPI instance
> calls napi_gro_complete() at an higher frequency than the NAPI timeout.
> Willem noted that even TCP packets could be trapped there, till the
> next retransmission.
> This patch tries to address the issue, flushing the oldest packets before
> scheduling the NAPI timeout. The rationale is that such a timeout should be
> well below a jiffy and we are not flushing packets eligible for sane GRO.

Might be useful to be a bit more exact: oldest packets -> old packets,
those with a NAPI_GRO_CB age before the current jiffy. That helps
explain the "well below a jiffy" comment. I had to reread to code to
understand that this did not just flush everything, obviation the
purpose of the gro timer.

Agreed that something more fine-grained than jiffies would help. In
the case of gro_timer we know the interval, so even an age expressed
with an epoch counter would suffice? Anyway, out of scope for this patch.

> RFC -> v1:
>  - added 'Fixes tags', cleaned-up the wording.
>
> Reported-by: Eric Dumazet <eric.dumazet@...il.com>
> Fixes: 3b47d30396ba ("net: gro: add a per device gro flush timer")
> Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>

Acked on the change. Thanks, Paolo. Just a few comments and questions
about the description and target. Feel free to ignore, in which case
I'll just add my Acked-by.

> --
> Note: since one of the fixed commit is currently only on net-next,
> the other one is really old, and the affected scenario without the
> more recent commit is really a corner case, targeting net-next.

IMHO there is nothing UDP specific here. Any sender that applies GRO
and that does not use push bits can trigger this, including non-native
TCP stacks. The gro_timer is a bit of an edge case, but that makes it
fine to send it to net, too.

> ---
>  net/core/dev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5927f6a7c301..b6eb4e0bfa91 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5975,11 +5975,14 @@ bool napi_complete_done(struct napi_struct *n, int work_done)
>                 if (work_done)
>                         timeout = n->dev->gro_flush_timeout;
>
> +               /* When the NAPI instance uses a timeout, we still need to
> +                * somehow bound the time packets are keept in the GRO layer

nit: keept -> kept

> +                * under heavy traffic

this only requires one packet per gro_flush_timeout?

> +                */
> +               napi_gro_flush(n, !!timeout);
>                 if (timeout)
>                         hrtimer_start(&n->timer, ns_to_ktime(timeout),
>                                       HRTIMER_MODE_REL_PINNED);
> -               else
> -                       napi_gro_flush(n, false);
>         }
>         if (unlikely(!list_empty(&n->poll_list))) {
>                 /* If n->poll_list is not empty, we need to mask irqs */
> --
> 2.17.2
>