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

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

On Wed, Apr 29, 2020 at 11:05:12PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Apr 30, 2020 at 06:30:29AM +1000, Duncan Roe wrote:
> > Hi Pablo,
> >
> > I sent this email (explanation of how the system works) before I saw your email
> > asking for that explanation.
> >
> > Then I replied to that email of yours before I saw this one.
> >
> > On Wed, Apr 29, 2020 at 09:16:43PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Apr 30, 2020 at 05:10:47AM +1000, Duncan Roe wrote:
> > > [...]
> > > > Sorry, I should have explained a bit more how the system would work:
> > > >
> > > > struct pkt_buff has 3 new members:
> > > >
> > > >         bool    copy_done;
> > > >         uint32_t extra;
> > > >         uint8_t *copy_buf;
> > > >
> > > > When extra > 0, pktb_alloc2 verifies that buflen is >= len + extra. It then
> > > > stores extra and copy_buf in pktb, ready for use by pktb_mangle() (all the other
> > > > manglers call this eventually).
> > > >
> > > > So that's how pktb_mangle() doesn't need to allocate a buffer.
> > >
> > > Thanks for the explaining. Given this is in userspace, it is easier if
> >
> > Tiny nit - this could be userspace on an embedded device where memory is really
> > tight. So perhaps document with "If memory is at a premium, you really only need
> > len + extra" otherwise a big buf is fine.
>
> This buffer is still relatively small, the reallocation also forces
> the user to refetch pointers. Skipping all that complexity for a bit
> of memory in userspace is fine.

Oh well in that case, how about:

>	struct pkt_buff *pktb_alloc2(int family, void *buf, size_t buf_size, void *data, size_t len, size_t extra);

I.e. exactly as you suggested in
https://www.spinics.net/lists/netfilter-devel/msg65830.html except s/head/buf/

And we tell users to dimension buf to NFQ_BUFFER_SIZE. We don't even need to
expose pktb_head_size().
>
> > > the user allocates the maximum packet length that is possible:
> > >
> > >         0xffff + (MNL_SOCKET_BUFFER_SIZE/2);
> > >
> > > We can probably expose this to the header so they can pre-allocate a
> > > buffer that is large enough and, hence, _mangle() is guaranteed to
> > > have always enough room to add extra bytes.
> >
> > Yes I saw that expression in examples/nf-queue.c. How about
> >
> > #define COPY_BUF_SIZE (0xffff + (MNL_SOCKET_BUFFER_SIZE/2))
> >
> > or what other name would you like?
>
> I'd suggest to add the NFQ_ prefix, probably NFQ_BUFFER_SIZE is fine.
>
> > --- Off-topic
> >
> > I'm intrigued that you ccan use MNL_SOCKET_BUFFER_SIZE when dimensioning static
> > variables. The expansion is
> >
> > #define MNL_SOCKET_BUFFER_SIZE (sysconf(_SC_PAGESIZE) < 8192L ?
> > sysconf(_SC_PAGESIZE) : 8192L)
> >
> > and sysconf is an actual function:
> >
> > unistd.h:622:extern long int sysconf (int __name) __THROW;
> >
> > If I try to dimension a static variable using pktb_head_size(), the compiler
> > throws an error.  Why is sysconf() ok but not pktb_head_size()?
>
> I'm fine to expose if you would like to add pktb_head_size() if you
> prefer to store the pkt_buff in the stack. You will have to use
> pktb_build_data() [1] to initialize the pkt_buff. Would that work for you?
>
> [1] https://patchwork.ozlabs.org/project/netfilter-devel/patch/20200426132356.8346-2-pablo@netfilter.org/