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: <m2bk1uq3a7.fsf@ja-home.int.chopps.org>
Date: Wed, 14 Aug 2024 11:34:31 -0400
From: Christian Hopps <chopps@...pps.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>, 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

Paolo Abeni <pabeni@...hat.com> writes:

> 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.

IP-TFS sometimes needs to allocate new skb[s] to fragment a too-large tunnel ingress user packet. IP-TFS may also need to extract multiple aggregated user packets for tunnel egress from inside a single IPTFS tunnel packet. For these 1 to N cases (which are different from regular IPsec which is always 1-1 and thus re-using the existing skb) we need to create multiple skbs from a single source skb and we need to replicate the work done to the existing skb so far in the netdev/xfrm infrastructure (e.g. the _refdst and _nfct are expected to be there and refcounted as they are dropped later in the stack). This work is captured in those first few values that we are copying. The `headers` and other field values; however, are not appropriate to be copied (or clobbered e.g., alloc_cpu) into the new allocated skb.

I originally had this code local to the IP-TFS implementation, but I was persuaded to move it in skbuff.c to track any possible changes to these fields in the future.

Thanks,
Chris.

> Cheers,
>
> Paolo