netdev - Re: Does skb_metadata_differs really need to stop GRO aggregation?

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: <CAO3-PbptY8PvyVnAfM=n=T8ihivso-jD1iwyWO=8WVWyLFe81A@mail.gmail.com>
Date: Fri, 1 Dec 2023 11:09:50 -0600
From: Yan Zhai <yan@...udflare.com>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: Toke Høiland-Jørgensen <toke@...hat.com>, 
	Edward Cree <ecree.xilinx@...il.com>, Jesper Dangaard Brouer <hawk@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	Alexei Starovoitov <ast@...nel.org>, kernel-team <kernel-team@...udflare.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>, 
	"David S. Miller" <davem@...emloft.net>, Jakub Sitnicki <jakub@...udflare.com>
Subject: Re: Does skb_metadata_differs really need to stop GRO aggregation?

On Thu, Nov 30, 2023 at 10:32 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 11/30/23 2:55 PM, Toke Høiland-Jørgensen wrote:
> > Daniel Borkmann <daniel@...earbox.net> writes
> >> On 11/29/23 10:52 PM, Toke Høiland-Jørgensen wrote:
> >>> Edward Cree <ecree.xilinx@...il.com> writes:
> >>>> On 28/11/2023 14:39, Toke Høiland-Jørgensen wrote:
> >>>>> I'm not quite sure what should be the semantics of that, though. I.e.,
> >>>>> if you are trying to aggregate two packets that have the flag set, which
> >>>>> packet do you take the value from? What if only one packet has the flag
> >>
> >> It would probably make sense if both packets have it set.
> >
> > Right, so "aggregate only if both packets have the flag set, keeping the
> > metadata area from the first packet", then?
>
> Yes, sgtm.
>

There is one flaw for TCP in current implementation (before adding the
flag), which we experienced earlier in production: when metadata
differs on TCP packets, it not only disables GRO, but also reorder all
PSH packets. This happens because when metadata differs, the new
packet will be linked as a different node on the GRO merge list, since
NAPI_GRO_CB->same_flow is set to 0 for all previous packets. However,
packets with flags like PSH will be immediately flushed to the upper
stack, while its predecessor packets might still be waiting on the
merge list. I think it might make sense to first delay metadata
comparison before skb_gro_receive, then add the flag to determine if
the difference really matters.

Yan