netdev - Re: [PATCH net v2] net: Fix packet reordering caused by GRO and listified RX cooperation

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: <df97177e-0b73-8173-584a-54643c6cac6f@solarflare.com>
Date:   Wed, 22 Jan 2020 10:16:41 +0000
From:   Edward Cree <ecree@...arflare.com>
To:     Maxim Mikityanskiy <maximmi@...lanox.com>,
        Alexander Lobakin <alobakin@...nk.ru>,
        Saeed Mahameed <saeedm@...lanox.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Johannes Berg <johannes.berg@...el.com>,
        "Emmanuel Grumbach" <emmanuel.grumbach@...el.com>,
        Luca Coelho <luciano.coelho@...el.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net v2] net: Fix packet reordering caused by GRO and
 listified RX cooperation

On 21/01/2020 15:09, Maxim Mikityanskiy wrote:
> Commit 323ebb61e32b ("net: use listified RX for handling GRO_NORMAL
> skbs") introduces batching of GRO_NORMAL packets in napi_frags_finish,
> and commit 6570bc79c0df ("net: core: use listified Rx for GRO_NORMAL in
> napi_gro_receive()") adds the same to napi_skb_finish. However,
> dev_gro_receive (that is called just before napi_{frags,skb}_finish) can
> also pass skbs to the networking stack: e.g., when the GRO session is
> flushed, napi_gro_complete is called, which passes pp directly to
> netif_receive_skb_internal, skipping napi->rx_list. It means that the
> packet stored in pp will be handled by the stack earlier than the
> packets that arrived before, but are still waiting in napi->rx_list. It
> leads to TCP reorderings that can be observed in the TCPOFOQueue counter
> in netstat.
>
> This commit fixes the reordering issue by making napi_gro_complete also
> use napi->rx_list, so that all packets going through GRO will keep their
> order. In order to keep napi_gro_flush working properly, gro_normal_list
> calls are moved after the flush to clear napi->rx_list.
>
> iwlwifi calls napi_gro_flush directly and does the same thing that is
> done by gro_normal_list, so the same change is applied there:
> napi_gro_flush is moved to be before the flush of napi->rx_list.
>
> A few other drivers also use napi_gro_flush (brocade/bna/bnad.c,
> cortina/gemini.c, hisilicon/hns3/hns3_enet.c). The first two also use
> napi_complete_done afterwards, which performs the gro_normal_list flush,
> so they are fine. The latter calls napi_gro_receive right after
> napi_gro_flush, so it can end up with non-empty napi->rx_list anyway.
>
> Fixes: 323ebb61e32b ("net: use listified RX for handling GRO_NORMAL skbs")
> Signed-off-by: Maxim Mikityanskiy <maximmi@...lanox.com>
> Cc: Alexander Lobakin <alobakin@...nk.ru>
> Cc: Edward Cree <ecree@...arflare.com>
> ---
> v2 changes:
>
> Flush napi->rx_list after napi_gro_flush, not before. Do it in iwlwifi
> as well.
I believe David prefers for patch change logs to go in the commit message,
 not below the line.
> Please also pay attention that there is gro_flush_oldest that also calls
> napi_gro_complete and DOESN'T do gro_normal_list to flush napi->rx_list.
> I guess, it's not required in this flow, but if I'm wrong, please tell
> me.
>
>  drivers/net/wireless/intel/iwlwifi/pcie/rx.c |  4 +-
>  net/core/dev.c                               | 64 ++++++++++----------
>  2 files changed, 35 insertions(+), 33 deletions(-)
Acked-by: Edward Cree <ecree@...arflare.com>