Re: [PATCH ipsec-next v8 10/16] xfrm: iptfs: add fragmenting of larger than MTU user packets

[Sabrina Dubroca <sd@queasysnail.net>]

2024-08-07, 15:40:14 -0400, Christian Hopps wrote:
> 
> Steffen Klassert <steffen.klassert@secunet.com> writes:
> 
> > On Mon, Aug 05, 2024 at 12:25:57AM +0200, Sabrina Dubroca wrote:
> > > 
> > > > +/**
> > > > + * skb_copy_bits_seq - copy bits from a skb_seq_state to kernel buffer
> > > > + * @st: source skb_seq_state
> > > > + * @offset: offset in source
> > > > + * @to: destination buffer
> > > > + * @len: number of bytes to copy
> > > > + *
> > > > + * Copy @len bytes from @offset bytes into the source @st to the destination
> > > > + * buffer @to. `offset` should increase (or be unchanged) with each subsequent
> > > > + * call to this function. If offset needs to decrease from the previous use `st`
> > > > + * should be reset first.
> > > > + *
> > > > + * Return: 0 on success or a negative error code on failure
> > > > + */
> > > > +static int skb_copy_bits_seq(struct skb_seq_state *st, int offset, void *to,
> > > > +			     int len)
> > > 
> > > Probably belongs in net/core/skbuff.c, although I'm really not
> > > convinced copying data around is the right way to implement the type
> > > of packet splitting IPTFS does (which sounds a bit like a kind of
> > > GSO).
> > 
> > I tried to come up with a 'GSO like' variant of this when I did the
> > initial review last year at the IPsec workshop. But it turned out
> > that things will get even more complicated as they are now.
> > We did some performance tests and it was quite compareable to
> > tunnel mode, so for a first implementation I'd be ok with the
> > copy variant.

Ok.

> > > And there are helpers in net/core/skbuff.c (such as
> > > pskb_carve/pskb_extract) that seem to do similar things to what you
> > > need here, without as much data copying.
> > 
> > In case we have helpers that will fit here, we should use them of
> > course.
> 
> FWIW, The reason I didn't use pskb_extract() rather than the simple
> iptfs_copy_create_frag() is because pskb_extract uses skb_clone on
> the original skb then pskb_carve() to narrow the (copied) data
> pointers to a subset of the original. The new skb data is read-only
> which does not work for us.
> 
> Each of these new skbs are IP-TFS tunnel packets and as such we need
> to push and write IPTFS+ESP+IP+Ethernet headers on them. In order to
> make pskb_extract()s skbs writable we would have to allocate new
> buffer space and copy the data turning them into a writeable skb
> buffer, and now we're doing something more complex and more cpu
> intensive to arrive back to what iptfs_copy_create_frag() did simply
> and straight-forwardly to begin with.

That only requires the header to be writeable, not the full packet,
right? I doubt it would actually be more cpu/memory intensive.

-- 
Sabrina