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. > > 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@xxxxxxxxxxxxx/
