Re: [PATCH libnetfilter_queue 0/3] pktbuff API updates

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Apr 29, 2020 at 07:14:52AM +1000, Duncan Roe wrote:
> On Tue, Apr 28, 2020 at 12:34:07PM +0200, Pablo Neira Ayuso wrote:
[...]
> > pktb_alloc2() still has a memcpy which is not needed by people that do
> > not need to mangle the packet.
> 
> No it does not. Please look again. There is only a memcpy if the caller
> specifies extra > 0, in which case she clearly intends to mangle it (perhaps
> depending on its contents).

Right, it only happens if extra is specified.

+       if (extra) {
+               pkt_data = buf;
+               memcpy(pkt_data, data, len);
+               memset((uint8_t *)pkt_data + len, 0, extra);
+       } else {
+               pkt_data = data;
+       }

So buf is only used if extra is specified?

> "depending on its contents" is where the memcpy deferral comes in. pktb_alloc2()
> verifies that the supplied buffer is big enough (size >= len + extra). The user
> declared it as a stack variable that size so it will be. With the deferral
> enhancement, pktb_alloc2() records the buffer address and extra in the enlarged
> struct pktbuff (extra is needed to tell pktb_mangle how much memory to memset to
> 0).

I agree that deferring the memcpy() and avoiding the malloc() is the
way to go, we only have to agree in the way to achieve this.

> If pktb_mangle() finds it has to make the packet larger then its original length
> and the packet is still in its original location then copy it and zero extra.
> (i.e. pktb_mangle() doesn't just check whether it was asked to make the packet
> bigger: it might have previously been asked to make it smaller).
>
> Also (and this *is* tricky, update relevant pointers in the struct pktbuff).
> That invalidates any poiners the caller may have obtained from e.g. pktb_data()
> - see end of email.

Regarding pktb_mangle() reallocation case, refetching the pointers
sounds fine, documenting this is sufficient.

[...]
> > Revisiting, I would prefer to keep things simple. The caller should
> > make sure that pktb_mangle() has a buffer containing enough room. I
> > think it's more simple for the caller to allocate a buffer that is
> > large enough for any mangling.
> 
> Yes it's more complex. No problem with the buffer - the user gave that to
> pktb_alloc2().

I'm just hesitating about the new pktb_alloc2() approach because it
has many parameters, it's just looks a bit complicated to me (this
function takes 8 parameters).

If you can just pre-allocate the pkt_buff head from the configuration
phase (before receiving packets from the kernel), then attach the
buffer via pktb_setup_metadata() for each packet that is received (so
the pkt_buff head is recycled). With this approach, pktb_head_size()
won't be needed either.

My understanding is that requirements are:

* Users that do not want to mangle the packet, they use the buffer
  that was passed to recvmsg().
* Users that want to mangle the packet, they use the _mangle()
  function that might reallocate the data buffer (the one you would
  like to have). However, if this data buffer reallocation happens,
  then pkt_buff should annotate that this pkt_buff object needs to
  release this data buffer from pktb_free() otherwise.

> Problem is that if mangler moves the packet, then any packet pointer the caller
> had is invalid (points to the un-mangled copy). This applies at all levels, e.g.
> nfq_udp_get_payload(). There is no way for the mangler functions to address
> this: it just has to be highlighted in the documentation.

That's fine, this is exactly how the kernel works: if the function
might reallocate the data area, then you have to refetch pointers
after this. If you teach _mangle() to do reallocations, then
documenting this is fine.

However, those reallocation need pktb_free() to release that new data
buffer, right?

> Still, I really like the deferred copy enhancement. Your thoughts?

The deferred copy idea when mangling sounds fine, we only have to
agree on how to get this done.

Thanks.