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] [day] [month] [year] [list]

Message-ID: <m2sev592an.fsf@ja-home.int.chopps.org>
Date: Thu, 15 Aug 2024 12:24:12 -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


Christian Hopps <chopps@...pps.org> writes:

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

I've gone back through the skb use again for both tunnel ingress and egress and I need even less than the subset of fields being copied here.

I'm going to resend this patchset removing this somewhat controversial refactoring for now, and just leave the second commit with the useful copy from read_seq function.

Thanks,
Chris.


>
> Thanks,
> Chris.
>
>> Cheers,
>>
>> Paolo

a