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: <b9a879b2-bb62-ba18-0bdd-5c126a1086a9@intel.com>
Date: Tue, 16 May 2023 14:37:13 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>
CC: Jesper Dangaard Brouer <jbrouer@...hat.com>, <bpf@...r.kernel.org>,
	<brouer@...hat.com>, Stanislav Fomichev <sdf@...gle.com>, Alexei Starovoitov
	<ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 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: Larysa Zaremba <larysa.zaremba@...el.com>
Date: Mon, 15 May 2023 19:08:39 +0200

> On Mon, May 15, 2023 at 06:17:02PM +0200, Jesper Dangaard Brouer wrote:
>>
>>
>> On 12/05/2023 17.26, Larysa Zaremba wrote:
>>> From: Aleksander Lobakin <aleksander.lobakin@...el.com>
>>>
>>> When using XDP hints, metadata sometimes has to be much bigger
>>> than 32 bytes. Relax the restriction, allow metadata larger than 32 bytes
>>> and make __skb_metadata_differs() work with bigger lengths.
>>>
>>> Now size of metadata is only limited by the fact it is stored as u8
>>> in skb_shared_info, so maximum possible value is 255.
>>
>> I'm confused, IIRC the metadata area isn't stored "in skb_shared_info".
>> The maximum possible size is limited by the XDP headroom, which is also
>> shared/limited with/by xdp_frame.  I must be reading the sentence wrong,
>> somehow.

skb_shared_info::meta_size is u8. Since metadata gets carried from
xdp_buff to skb, this check is needed (it's compile-time constant anyway).
Check for headroom is done separately already (two sentences below).

> 
> It's not 'metadata is stored as u8', it's 'metadata size is stored as u8' :)
> Maybe I should rephrase it better in v2.
> 
>>
>>> Other important
>>> conditions, such as having enough space for xdp_frame building, are already
>>> checked in bpf_xdp_adjust_meta().
>>>
>>> The requirement of having its length aligned to 4 bytes is still
>>> valid.
BTW I decided to not expand switch-case in __skb_metadata_differs() with
more size values because: 1) it's not a common case; 2) memcmp() is +/-
fast on x86; 3) it's gross already. But I can if needed :D I think it
can be compressed via some macro hell.

(this function is called for each skb when GROing if it carries any
 meta, so sometimes may hurt. Larysa, have you noticed any perf
 regression between meta <= 32 and > 32?)

Thanks,
Olek