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

Date: Tue, 28 Nov 2023 15:39:22 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard Brouer
 <hawk@...nel.org>
Cc: Yan Zhai <yan@...udflare.com>, 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?

Daniel Borkmann <daniel@...earbox.net> writes:

> On 11/28/23 2:06 PM, Jesper Dangaard Brouer wrote:
>> On 11/28/23 13:37, Jesper Dangaard Brouer wrote:
>>> Hi Daniel,
>>>
>>> I'm trying to understand why skb_metadata_differs() needed to block GRO ?
>>>
>>> I was looking at XDP storing information in metadata area that also
>>> survives into SKBs layer.  E.g. the RX timestamp.
>>>
>>> Then I noticed that GRO code (gro_list_prepare) will not allow
>>> aggregating if metadata isn't the same in all packets via
>>> skb_metadata_differs().  Is this really needed?
>>> Can we lift/remove this limitation?
>> 
>> (Answering myself)
>> I understand/see now, that when an SKB gets GRO aggregated, I will
>> "lose" access to the metadata information and only have access to the
>> metadata in the "first" SKB.
>> Thus, GRO layer still needs this check and it cannot know if the info
>> was important or not.
>
> ^ This exactly in order to avoid loosing information for the upper stack. I'm
> not sure if there is an alternative scheme we could do where BPF prog can tell
> 'it's okay to loose meta data if skb can get aggregated', and then we just skip
> the below skb_metadata_differs() check. We could probably encode a flag in the
> meta_len given the latter requires 4 byte alignment. Then BPF prog can
> decide.

A flag seems sane. I guess we could encode some flag values in the upper
bits of the 'offset' argument of the bpf_xdp_adjust_meta() helper, since
valid values are guaranteed to be pretty small anyway? :)

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
set? Or should we instead have a "metadata_xdp_only" flag that just
prevents the skb metadata field from being set entirely? Or would both
be useful?

-Toke