From: Duncan Roe <duncan_roe@optusnet.com.au>
To: Netfilter Development <netfilter-devel@vger.kernel.org>
Subject: Re: [PATCH libnetfilter_queue 0/3] pktbuff API updates
Date: Thu, 30 Apr 2020 05:54:14 +1000 [thread overview]
Message-ID: <20200429195414.GC3833@dimstar.local.net> (raw)
In-Reply-To: <20200429190020.GA16096@salvia>
On Wed, Apr 29, 2020 at 09:00:20PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Apr 29, 2020 at 11:28:40PM +1000, Duncan Roe wrote:
> > 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.
>
> OK. Then, the user must pass the buf only if extra is set on.
>
> > > > 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.
>
> I'm starting to think this function is hard to document, too many
> parameters.
The documentation looks fine to me - I'm looking at it in a web browser right
now. Have you tried that?
>
> > 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];
>
> You might also declare the pre-allocated pkt_buff as a global if you
> don't want to use the data pointer in mnl_cb_run().
I'm uneasy about this. We're writing a library here. We shouldn't be dictating
to the user that he must declare globals. "static" won't do in a multi-threaded
program, but you could use "thread local" (malloc'd under the covers, (tiny)
performance hit c/w stack (which is always thread local)).
"The road to bloat is paved with tiny performance hits" [1]
>
> static struct pkt_buff *pkt;
>
> int main(int argc, char *argv[])
> {
> ...
> pkt = pktb_head_alloc();
> ...
> }
>
> Then, use it from queue_cb().
>
> Alternatively, you can also define a wrapper structure that you can
> pass to mnl_cb_run(), e.g.
>
> struct my_data {
> struct pkt_buff *pktb;
> void *something_ese;
> };
>
> > > 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).
>
> Then, I'm not sure I understand yet what extension you would like to
> make to _mangle(), please, clarify.
>
> > > > 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.
>
> If the user pre-allocates the heap or place it in the stack is
> irrelevant, the save for the user is the memcpy() if it's only
> inspecting the packet (no mangling) and the out-of-line pkt_buff
> allocation / or place in the stack.
>
> If pktb_build_data() takes the extra parameter, I think the
> showstopper you mentioned is gone. Otherwise, please tell me what you
> cannot achieve with my patchset.
My patchset comes with documentation and yours does not. I am not volunteering
to document yours.
Would you like me to post a detailed review of your patchset? It will not be
pretty.
>
> Thanks.
[1] I just amde that up. Good eh?
Cheers ... Duncan.
next prev parent reply other threads:[~2020-04-29 19:54 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
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 [this message]
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=20200429195414.GC3833@dimstar.local.net \
--to=duncan_roe@optusnet.com.au \
--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).