From: Pablo Neira Ayuso <pablo@netfilter.org>
To: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH libnetfilter_queue 0/3] pktbuff API updates
Date: Tue, 28 Apr 2020 12:34:07 +0200 [thread overview]
Message-ID: <20200428103407.GA1160@salvia> (raw)
In-Reply-To: <20200428043302.GB15436@dimstar.local.net>
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.
next prev parent reply other threads:[~2020-04-28 10:34 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-26 13:23 [PATCH libnetfilter_queue 0/3] pktbuff API updates Pablo Neira Ayuso
2020-04-26 13:23 ` [PATCH libnetfilter_queue 1/3] pktbuff: add pktb_alloc_head() and pktb_build_data() Pablo Neira Ayuso
2020-04-30 5:41 ` Duncan Roe
2020-04-26 13:23 ` [PATCH libnetfilter_queue 2/3] example: nf-queue: use pkt_buff Pablo Neira Ayuso
2020-05-14 4:35 ` Duncan Roe
2020-05-14 4:35 ` [PATCH libnetfilter_queue 1/1] example: nf-queue: use pkt_buff (updated) Duncan Roe
2020-04-26 13:23 ` [PATCH libnetfilter_queue 3/3] pktbuff: add pktb_reset_network_header() and pktb_set_network_header() Pablo Neira Ayuso
2020-04-27 11:06 ` [PATCH libnetfilter_queue 0/3] pktbuff API updates Duncan Roe
2020-04-27 17:06 ` Pablo Neira Ayuso
2020-04-28 4:33 ` Duncan Roe
2020-04-28 10:34 ` Pablo Neira Ayuso [this message]
2020-04-28 21:14 ` Duncan Roe
2020-04-28 22:55 ` Pablo Neira Ayuso
2020-04-29 13:28 ` Duncan Roe
2020-04-29 19:00 ` Pablo Neira Ayuso
2020-04-29 19:54 ` Duncan Roe
2020-04-29 21:12 ` Pablo Neira Ayuso
2020-04-29 19:10 ` Duncan Roe
2020-04-29 19:16 ` Pablo Neira Ayuso
2020-04-29 20:30 ` Duncan Roe
2020-04-29 21:05 ` Pablo Neira Ayuso
2020-04-30 6:34 ` Duncan Roe
2020-05-02 12:50 ` Duncan Roe
2020-05-05 12:30 ` Pablo Neira Ayuso
2020-05-06 0:57 ` Duncan Roe
2020-05-06 2:39 ` Duncan Roe
2020-05-08 1:13 ` Duncan Roe
2020-05-09 8:26 ` Duncan Roe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200428103407.GA1160@salvia \
--to=pablo@netfilter.org \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).