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

[Duncan Roe <duncan_roe@optusnet.com.au>]

On Wed, Apr 29, 2020 at 12:55:20AM +0200, Pablo Neira Ayuso wrote:
> 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?

Yes, that's right.
>
> > "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.

I reckon they'll just copy the code from the nfq_nlmsg_verdict_put_pkt() man /
web page. After declaring "char pktbuf[plen + EXTRA];" one can use "sizeof
pktbuf" as the length argument.
> >
> > 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).

It has the original 4 from pktb_alloc() plus 2 {buffer, size} pairs. It could
have been just one pair, with packet data appended to metadata as in
pktb_alloc() but I thought it would be really awkward to document how to
dimension it.
>
> 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.

I think we should not be usurping the data pointer of mnl_cb_run(). I can see
people wanting to use it to pass a pointer to e.g. some kind of database that
callbacks need to access. There's no performance gain to recycling the buffer:
the CB doesn't need to call pktb_head_size() on every invocation, that can be
done once by main() e.g.

 static size_t sizeof_head;
 ...
 int main(int argc, char *argv[])
 {
 ...
         sizeof_head = pktb_head_size(); /* Avoid multiple calls in CB */
 ...
 static int queue_cb(const struct nlmsghdr *nlh, void *data)
 {
         char head[sizeof_head];

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

No, there is nothing to release. We told pktb_alloc2() where the buffer was,
it's on the stack (usually).
>
> > 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?

No way. There is no malloc() nor free() anywhere. The data buffer is
(recommended to be) on the stack; for running under gdb it may be preferred to
us a static buffer which has to be dimensioned hugely.
>
> > 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.

Cheers ... Duncan.