netdev - Re: [PATCH bpf-next v2 15/20] 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: <8821258e-08e7-8128-8275-74ce550110cd@intel.com>
Date: Mon, 10 Jul 2023 16:01:02 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>, John Fastabend
	<john.fastabend@...il.com>
CC: <bpf@...r.kernel.org>, <ast@...nel.org>, <daniel@...earbox.net>,
	<andrii@...nel.org>, <martin.lau@...ux.dev>, <song@...nel.org>, <yhs@...com>,
	<kpsingh@...nel.org>, <sdf@...gle.com>, <haoluo@...gle.com>,
	<jolsa@...nel.org>, David Ahern <dsahern@...il.com>, Jakub Kicinski
	<kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>, "Jesper Dangaard
 Brouer" <brouer@...hat.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>
Subject: Re: [PATCH bpf-next v2 15/20] net, xdp: allow metadata > 32

From: Larysa Zaremba <larysa.zaremba@...el.com>
Date: Thu, 6 Jul 2023 16:51:22 +0200

> On Mon, Jul 03, 2023 at 02:06:46PM -0700, John Fastabend wrote:
>> 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. 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.
>>>
>>> Signed-off-by: Aleksander Lobakin <aleksander.lobakin@...el.com>
>>> Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
>>> ---
>>>  include/linux/skbuff.h | 13 ++++++++-----
>>>  include/net/xdp.h      |  7 ++++++-
>>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 91ed66952580..cd49cdd71019 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -4209,10 +4209,13 @@ static inline bool __skb_metadata_differs(const struct sk_buff *skb_a,
>>>  {
>>>  	const void *a = skb_metadata_end(skb_a);
>>>  	const void *b = skb_metadata_end(skb_b);
>>> -	/* Using more efficient varaiant than plain call to memcmp(). */
>>> -#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
>>
>> Why are we removing the ifdef here? Its adding a runtime 'if' when its not
>> necessary. I would keep the ifdef and simply add the default case
>> in the switch.
> 
> Seems like Alex has missed your message, but we discussed this with him before, 
> so I know the answer: Compiler will 100% convert it into a compile-time 'if' and 
> this looks nicer than preprocessor condition.

Sorry, I'm not always able to follow all the threads =\

As Larysa said, it's not a runtime `if`. Both conditions are always
known at compilation time.
And this looks a bit less ugly than with ifdefs to me :D

Thanks,
Olek