netdev - Re: [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata

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: <d53f0150-d74b-7cf6-8fe7-324131b43982@intel.com>
Date: Mon, 22 May 2023 17:28:15 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jesper Dangaard Brouer <jbrouer@...hat.com>, Daniel Borkmann
	<daniel@...earbox.net>
CC: <brouer@...hat.com>, Larysa Zaremba <larysa.zaremba@...el.com>,
	<bpf@...r.kernel.org>, Stanislav Fomichev <sdf@...gle.com>, "Alexei
 Starovoitov" <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>, "Jakub
 Kicinski" <kuba@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, "Song
 Liu" <song@...nel.org>, Yonghong Song <yhs@...com>, John Fastabend
	<john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Jiri Olsa
	<jolsa@...nel.org>, Jesse Brandeburg <jesse.brandeburg@...el.com>, "Tony
 Nguyen" <anthony.l.nguyen@...el.com>, Anatoly Burakov
	<anatoly.burakov@...el.com>, Alexander Lobakin <alexandr.lobakin@...el.com>,
	Magnus Karlsson <magnus.karlsson@...il.com>, Maryam Tahhan
	<mtahhan@...hat.com>, <xdp-hints@...-project.net>, <netdev@...r.kernel.org>,
	<intel-wired-lan@...ts.osuosl.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata > 32

From: Jesper Dangaard Brouer <jbrouer@...hat.com>
Date: Mon, 22 May 2023 13:41:43 +0200

> 
> 
> On 19/05/2023 18.35, Alexander Lobakin wrote:
>> From: Jesper Dangaard Brouer <jbrouer@...hat.com>
>> Date: Tue, 16 May 2023 17:35:27 +0200

[...]

> Not talking about your changes (in this patch).
> 
> I'm realizing that SKBs using metadata area will have a performance hit
> due to accessing another cacheline (the meta_len in skb_shared_info).
> 
> IIRC Daniel complained about this performance hit (in the past), I guess
> this explains it.  IIRC Cilium changed to use percpu variables/datastore
> to workaround this.

Why should we compare metadata of skbs on GRO anyway? I was disabling it
the old hints series (conditionally, if driver asks), moreover...
...if metadata contains full checksum, GRO will be broken completely due
to this comparison (or any other frame-unique fields. VLAN tags and
hashes are okay).

> 
> 
>> The whole xdp_metalen_invalid() gets expanded into:
>>
>>     return (metalen % 4) || metalen > 255;
>>
>> at compile-time. All those typeof shenanigans are only to not open-code
>> meta_len's type/size/max.
>>
>>>
>>> But only use for SKBs that gets created from xdp with metadata, right?
>>>
> 
> Normal netstack processing actually access this skb_shinfo->meta_len in
> gro_list_prepare().  As the caller dev_gro_receive() later access other
> memory in skb_shared_info, then the GRO code path already takes this hit
> to begin with.

You access skb_shinfo() often even before running XDP program, for
example, when a frame is multi-buffer. Plus HW timestamps are also
there, and so on.

> 
> --Jesper
> 

Thanks,
Olek