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

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

On Tue, Apr 28, 2020 at 02:33:02PM +1000, Duncan Roe wrote:
> On Mon, Apr 27, 2020 at 07:06:56PM +0200, Pablo Neira Ayuso wrote:
> > Hi Duncan,
> >
> > On Mon, Apr 27, 2020 at 09:06:14PM +1000, Duncan Roe wrote:
> > > Hi Pablo,
> > >
> > > On Sun, Apr 26, 2020 at 03:23:53PM +0200, Pablo Neira Ayuso wrote:
> > > > Hi Duncan,
> > > >
> > > > This is another turn / incremental update to the pktbuff API based on
> > > > your feedback:
> > > >
> > > > Patch #1 adds pktb_alloc_head() to allocate the pkt_buff structure.
> > > > 	 This patch also adds pktb_build_data() to set up the pktbuff
> > > > 	 data pointer.
> > > >
> > > > Patch #2 updates the existing example to use pktb_alloc_head() and
> > > >          pktb_build_data().
> > > >
> > > > Patch #3 adds a few helper functions to set up the pointer to the
> > > >          network header.
> > > >
> > > > Your goal is to avoid the memory allocation and the memcpy() in
> > > > pktb_alloc(). With this scheme, users pre-allocate the pktbuff object
> > > > from the configuration step, and then this object is recycled for each
> > > > packet that is received from the kernel.
> > > >
> > > > Would this update fit for your usecase?
> > >
> > > No, sorry. The show-stopper is, no allowance for the "extra" arg,
> > > when you might want to mangle a packet tobe larger than it was.
> >
> > I see, maybe pktb_build_data() can be extended to take the "extra"
> > arg. Or something like this:
> >
> >  void pktb_build_data(struct pkt_buff *pktb, uint8_t *payload, uint32_t size, uint32_t len)
> >
> > where size is the total buffer size, and len is the number of bytes
> > that are in used in the buffer.
> 
> I really do not like the direction this is taking. pktb_build_data() is one of 4
> new functions you are suggesting, the others being pktb_alloc_head(),
> pktb_reset_network_header() and pktb_set_network_header(). In
> https://www.spinics.net/lists/netfilter-devel/msg65830.html, you asked
> 
> > I wonder if all these new functions can be consolidated into one
> > single function, something like:
> >
> >         struct pkt_buff *pktb_alloc2(int family, void *head, size_t head_size, void *data, size_t len, size_t extra);

pktb_alloc2() still has a memcpy which is not needed by people that do
not need to mangle the packet.

> That's what I have delivered, except for 2 extra args on the end for the packet
> copy buffer. And I get rid of pktb_free(), or at least deprecate and move it off
> the main doc page into the "Other functions" page.
> 
> Also pktb_set_network_header() makes no allowance for AF_BRIDGE.

This is not a problem, you only have to call this function with
ETH_HLEN to set the offset in case of bridge.

> Can we please just stick with
> 
> > struct pkt_buff *pktb_alloc2(int family, void *head, size_t headsize,
> >                              void *data, size_t len, size_t extra,
> >                              void *buf, size_t bufsize)

I'm fine if you still like the simplified pktb_alloc2() call, that's OK.

[...]
> > I think it's fine if pktb_mangle() deals with this data buffer
> > reallocation in case it needs to expand the packet, a extra patch on
> > top of this should be fine.
> 
> OK - will start on a patch based on
> https://www.spinics.net/lists/netfilter-devel/msg66710.html

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.