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: <30db82e2a1bdcabd61a31e2e8dac58a035d7023d.camel@mellanox.com>
Date:   Tue, 21 Jan 2020 20:39:06 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "ecree@...arflare.com" <ecree@...arflare.com>,
        Maxim Mikityanskiy <maximmi@...lanox.com>,
        "alobakin@...nk.ru" <alobakin@...nk.ru>
CC:     "luciano.coelho@...el.com" <luciano.coelho@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "johannes.berg@...el.com" <johannes.berg@...el.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Jiri Pirko <jiri@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "emmanuel.grumbach@...el.com" <emmanuel.grumbach@...el.com>
Subject: Re: [PATCH net v2] net: Fix packet reordering caused by GRO and
 listified RX cooperation

On Tue, 2020-01-21 at 15:09 +0000, 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.
> 
> 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(-)
> 

LGTM,

Acked-by: Saeed Mahameed <saeedm@...lanox.com>