netdev - Re: [PATCH ipsec-next v2 1/2] net: refactor common skb header copy code for re-use

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: <4d226a9e-8231-4794-a5ac-d426fac03361@redhat.com>
Date: Wed, 14 Aug 2024 16:16:16 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: Christian Hopps <chopps@...pps.org>, devel@...ux-ipsec.org,
 netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Florian Westphal <fw@...len.de>, Sabrina Dubroca <sd@...asysnail.net>,
 Simon Horman <horms@...nel.org>, Antony Antony <antony@...nome.org>,
 Christian Hopps <chopps@...n.net>
Subject: Re: [PATCH ipsec-next v2 1/2] net: refactor common skb header copy
 code for re-use

On 8/14/24 12:38, Steffen Klassert wrote:
> On Wed, Aug 14, 2024 at 11:46:56AM +0200, Paolo Abeni wrote:
>> On 8/9/24 10:34, Christian Hopps wrote:
>>> From: Christian Hopps <chopps@...n.net>
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -1515,7 +1515,7 @@ EXPORT_SYMBOL(napi_consume_skb);
>>>    	BUILD_BUG_ON(offsetof(struct sk_buff, field) !=		\
>>>    		     offsetof(struct sk_buff, headers.field));	\
>>> -static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
>>> +void ___copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
>>>    {
>>>    	new->tstamp		= old->tstamp;
>>>    	/* We do not copy old->sk */
>>> @@ -1524,6 +1524,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
>>>    	skb_dst_copy(new, old);
>>>    	__skb_ext_copy(new, old);
>>>    	__nf_copy(new, old, false);
>>> +}
>>> +EXPORT_SYMBOL_GPL(___copy_skb_header);
>>> +
>>> +static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
>>> +{
>>> +	___copy_skb_header(new, old); >
>>>    	/* Note : this field could be in the headers group.
>>>    	 * It is not yet because we do not want to have a 16 bit hole
>>
>> Could you please point where/how are you going to use this helper? factoring
>> out this very core bits of skbuff copy looks quite bug prone - and exporting
>> the helper could introduce additional unneeded function calls in the core
>> code.
> 
> It is supposed to be used in the IPTFS pachset:
> 
> https://lore.kernel.org/netdev/20240807211331.1081038-12-chopps@chopps.org/
> 
> It was open coded before, but there were some concerns that
> IPTFS won't get updated if __copy_skb_header changes.

The code is copying a subset of the skb header from a 'template' skb to 
a newly allocated skbuff.
It's unclear to me why would be useful to copy only a subset of the skb 
header, excluding queue_mapping, priority, etc..
I think we need a good justification for that, otherwise we could end-up 
with a large amount of "almost copy" skb header slicing the skb in many 
different ways.

Cheers,

Paolo