[PATCH libnetfilter_queue v3 0/5] Speed-up

[PATCH libnetfilter_queue v3 0/5] Speed-up

[Duncan Roe]

This is a re-issue of the patch I submitted  18th May 2021, with some extras.

Patch #1 is the original patch, rebased
Patch #2 is a new patch to get rid of a compiler warning
Patch #3 is a new patch to rename 'struct qwerty' to something meaningful
Patch #4 is a new patch to get rid of doxygen warnings
Patch #5 is a new patch to expose 'struct pktbuff' (to simplify code,
         since there is never a buffer tacked on the end of one any more)

Duncan Roe (5):
  Eliminate packet copy when constructing struct pkt_buff
  src: Avoid compiler warning
  src: Use more meaningful name in callback.c
  build: doc: Eliminate doxygen warnings
  src: struct pktbuff is no longer opaque

-- 
2.17.5

[PATCH libnetfilter_queue v3 1/5] src: eliminate packet copy when constructing struct pkt_buff

[Duncan Roe]

To avoid a copy, the new code takes advantage of the fact that the netfilter
netlink queue never returns multipart messages.
This means that the buffer space following that callback data is available for
packet expansion when mangling.

nfq_cb_run() is a new nfq-specific callback runqueue for netlink messages.
The principal function of nfq_cb_run() is to pass to the called function what is
the length of free space after the packet.
As a side benefit, nfq_cb_run() also gives the called function a pointer to a
zeroised struct pkt_buff, avoiding the malloc / free that was previously needed.

nfq_cb_t is a new typedef for the function called by nfq_cb_run()
[c.f. mnl_cb_t / mnl_cb_run].

No doxygen documentation update yet -
will do once function prototypes are agreed.
In the meantime, examples/nf-queue.c is updated to demonstrate the new API.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
v2: Pass saved vars in a struct rather than use (slower) thread-local
v3: Rebase to master commit c3bada27
 examples/nf-queue.c                    | 22 ++++++-
 include/libnetfilter_queue/Makefile.am |  1 +
 include/libnetfilter_queue/callback.h  | 11 ++++
 include/libnetfilter_queue/pktbuff.h   |  2 +
 src/Makefile.am                        |  1 +
 src/extra/callback.c                   | 60 +++++++++++++++++++
 src/extra/pktbuff.c                    | 80 ++++++++++++++++++--------
 7 files changed, 149 insertions(+), 28 deletions(-)
 create mode 100644 include/libnetfilter_queue/callback.h
 create mode 100644 src/extra/callback.c

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index 3da2c24..15c0d77 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -13,6 +13,8 @@
 #include <linux/types.h>
 #include <linux/netfilter/nfnetlink_queue.h>
 
+#include <libnetfilter_queue/pktbuff.h>
+#include <libnetfilter_queue/callback.h>
 #include <libnetfilter_queue/libnetfilter_queue.h>
 
 /* NFQA_CT requires CTA_* attributes defined in nfnetlink_conntrack.h */
@@ -46,13 +48,17 @@ nfq_send_verdict(int queue_num, uint32_t id)
 	}
 }
 
-static int queue_cb(const struct nlmsghdr *nlh, void *data)
+static int queue_cb(const struct nlmsghdr *nlh, void *data,
+		    struct pkt_buff *supplied_pktb, size_t supplied_extra)
 {
 	struct nfqnl_msg_packet_hdr *ph = NULL;
 	struct nlattr *attr[NFQA_MAX+1] = {};
+	struct pkt_buff *pktb;
 	uint32_t id = 0, skbinfo;
 	struct nfgenmsg *nfg;
+	uint8_t *payload;
 	uint16_t plen;
+	int i;
 
 	if (nfq_nlmsg_parse(nlh, attr) < 0) {
 		perror("problems parsing");
@@ -69,7 +75,10 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 	ph = mnl_attr_get_payload(attr[NFQA_PACKET_HDR]);
 
 	plen = mnl_attr_get_payload_len(attr[NFQA_PAYLOAD]);
-	/* void *payload = mnl_attr_get_payload(attr[NFQA_PAYLOAD]); */
+	payload = mnl_attr_get_payload(attr[NFQA_PAYLOAD]);
+
+	pktb = pktb_populate(supplied_pktb, AF_INET, payload, plen,
+			     supplied_extra);
 
 	skbinfo = attr[NFQA_SKB_INFO] ? ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO])) : 0;
 
@@ -115,6 +124,13 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 		printf(", checksum not ready");
 	puts(")");
 
+	printf("payload (len=%d) [", plen);
+
+	for (i = 0; i < pktb_len(pktb); i++)
+		printf("%02x", payload[i]);
+
+	printf("]\n");
+
 	nfq_send_verdict(ntohs(nfg->res_id), id);
 
 	return MNL_CB_OK;
@@ -186,7 +202,7 @@ int main(int argc, char *argv[])
 			exit(EXIT_FAILURE);
 		}
 
-		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
+		ret = nfq_cb_run(buf, ret, sizeof_buf, portid, queue_cb, NULL);
 		if (ret < 0){
 			perror("mnl_cb_run");
 			exit(EXIT_FAILURE);
diff --git a/include/libnetfilter_queue/Makefile.am b/include/libnetfilter_queue/Makefile.am
index e436bab..ed6524b 100644
--- a/include/libnetfilter_queue/Makefile.am
+++ b/include/libnetfilter_queue/Makefile.am
@@ -5,4 +5,5 @@ pkginclude_HEADERS = libnetfilter_queue.h	\
 		     libnetfilter_queue_ipv6.h	\
 		     libnetfilter_queue_tcp.h	\
 		     libnetfilter_queue_udp.h	\
+		     callback.h			\
 		     pktbuff.h
diff --git a/include/libnetfilter_queue/callback.h b/include/libnetfilter_queue/callback.h
new file mode 100644
index 0000000..27bfe31
--- /dev/null
+++ b/include/libnetfilter_queue/callback.h
@@ -0,0 +1,11 @@
+#ifndef _LIBNETFILTER_QUEUE_CALLBACK_H_
+#define _LIBNETFILTER_QUEUE_CALLBACK_H_
+
+struct nlattr;
+struct pkt_buff;
+
+typedef int (*nfq_cb_t)(const struct nlmsghdr *nlh, void *data, struct pkt_buff *pktb, size_t extra);
+
+int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap, unsigned int portid, nfq_cb_t cb_data, void *data);
+
+#endif
diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index 42bc153..33829cc 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -6,6 +6,8 @@ struct pkt_buff;
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
 void pktb_free(struct pkt_buff *pktb);
 
+struct pkt_buff *pktb_populate(struct pkt_buff *pktb, int family, void *data, size_t len, size_t extra);
+
 uint8_t *pktb_data(struct pkt_buff *pktb);
 uint32_t pktb_len(struct pkt_buff *pktb);
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 079853e..376bc45 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -31,6 +31,7 @@ libnetfilter_queue_la_LDFLAGS = -Wc,-nostartfiles \
 libnetfilter_queue_la_SOURCES = libnetfilter_queue.c	\
 				nlmsg.c			\
 				extra/checksum.c	\
+				extra/callback.c	\
 				extra/icmp.c		\
 				extra/ipv6.c		\
 				extra/tcp.c		\
diff --git a/src/extra/callback.c b/src/extra/callback.c
new file mode 100644
index 0000000..2d67848
--- /dev/null
+++ b/src/extra/callback.c
@@ -0,0 +1,60 @@
+/*
+ * (C) 2021 by Duncan Roe <duncan_roe@optusnet.com.au>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; either version 2.1 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <errno.h>
+#include <stdlib.h>
+#include <libmnl/libmnl.h>
+#include <linux/netlink.h>
+#include <libnetfilter_queue/callback.h>
+#include <libnetfilter_queue/libnetfilter_queue.h>
+#include "internal.h"
+
+/* ---------------------------------------------------------------------- */
+/* It would be less code to have local_cb() declared within nfq_cb_run(). */
+/* gcc is fine with that; unfortunately clang 8.0.1 is not.               */
+/* Lexical scoping would have given access to the 3 outer stack variables */
+/* needed by local_cb(); absent that, pass them down through the data arg */
+/* ---------------------------------------------------------------------- */
+
+typedef struct qwerty
+{
+	nfq_cb_t cb_func;
+	size_t buflen;
+	size_t bufcap;
+	void *data;
+} werty;
+
+static int local_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct pkt_buff pktb_instance = { };
+	werty *w = (werty *)data;
+
+	return w->cb_func(nlh, w->data, &pktb_instance, w->bufcap - w->buflen);
+}
+
+EXPORT_SYMBOL int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap,
+			     unsigned int portid, nfq_cb_t cb_func, void *data)
+{
+	const struct nlmsghdr *nlh = buf;
+	werty wert;
+
+	wert.cb_func = cb_func;
+	wert.bufcap = bufcap;
+	wert.buflen = buflen;
+	wert.data = data;
+
+	/* Verify not multi-part */
+	if (nlh->nlmsg_flags & NLM_F_MULTI) {
+		errno = E2BIG;
+		return MNL_CB_ERROR;
+	}
+
+
+	return mnl_cb_run(buf, buflen, 0, portid, local_cb, &wert);
+}
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 005172c..df23335 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -19,6 +19,8 @@
 #include <netinet/tcp.h>
 
 #include "internal.h"
+#include <libnetfilter_queue/pktbuff.h> /* I.e. local copy */
+					/* (to verify prototypes) */
 
 /**
  * \defgroup pktbuff User-space network packet buffer
@@ -67,6 +60,15 @@ static int __pktb_setup(int family, struct pkt_buff *pktb)
 	return 0;
 }
 
+static void pktb_setup_metadata(struct pkt_buff *pktb, void *pkt_data,
+				size_t len, size_t extra)
+{
+	pktb->len = len;
+	pktb->data_len = len + extra;
+
+	pktb->data = pkt_data;
+}
+
 /**
  * pktb_alloc - allocate a new packet buffer
  * \param family Indicate what family. Currently supported families are
@@ -100,10 +102,7 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	pkt_data = (uint8_t *)pktb + sizeof(struct pkt_buff);
 	memcpy(pkt_data, data, len);
 
-	pktb->len = len;
-	pktb->data_len = len + extra;
-
-	pktb->data = pkt_data;
+	pktb_setup_metadata(pktb, pkt_data, len, extra);
 
 	if (__pktb_setup(family, pktb) < 0) {
 		free(pktb);
@@ -113,6 +112,17 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	return pktb;
 }
 
+EXPORT_SYMBOL
+struct pkt_buff *pktb_populate(struct pkt_buff *pktb, int family, void *data,
+			       size_t len, size_t extra)
+{
+	pktb_setup_metadata(pktb, data, len, extra);
+	if (__pktb_setup(family, pktb) < 0)
+		pktb = NULL;
+	return pktb;
+}
+
+
 /**
  * pktb_data - get pointer to network packet
  * \param pktb Pointer to userspace packet buffer

[PATCH libnetfilter_queue v3 2/5] src: Avoid compiler warning

[Duncan Roe]

The warning said to add '#include <linux/netfilter/nfnetlink_queue.h>' before
'#include <libnetfilter_queue/libnetfilter_queue.h>' but it turns out we don't
need libnetfilter_queue.h

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
v3: New patch
 src/extra/callback.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/extra/callback.c b/src/extra/callback.c
index 2d67848..057d55a 100644
--- a/src/extra/callback.c
+++ b/src/extra/callback.c
@@ -12,7 +12,6 @@
 #include <libmnl/libmnl.h>
 #include <linux/netlink.h>
 #include <libnetfilter_queue/callback.h>
-#include <libnetfilter_queue/libnetfilter_queue.h>
 #include "internal.h"
 
 /* ---------------------------------------------------------------------- */
-- 
2.17.5

[PATCH libnetfilter_queue v3 3/5] src: Use more meaningful name in callback.c

[Duncan Roe]

Replace 'struct qwerty' with 'struct data_carrier'.
Also get rid of the typedef.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
v3: New patch
 src/extra/callback.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/extra/callback.c b/src/extra/callback.c
index 057d55a..dee6fc2 100644
--- a/src/extra/callback.c
+++ b/src/extra/callback.c
@@ -21,32 +21,32 @@
 /* needed by local_cb(); absent that, pass them down through the data arg */
 /* ---------------------------------------------------------------------- */
 
-typedef struct qwerty
+struct data_carrier
 {
 	nfq_cb_t cb_func;
 	size_t buflen;
 	size_t bufcap;
 	void *data;
-} werty;
+};
 
 static int local_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct pkt_buff pktb_instance = { };
-	werty *w = (werty *)data;
+	struct data_carrier *d = (struct data_carrier *)data;
 
-	return w->cb_func(nlh, w->data, &pktb_instance, w->bufcap - w->buflen);
+	return d->cb_func(nlh, d->data, &pktb_instance, d->bufcap - d->buflen);
 }
 
 EXPORT_SYMBOL int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap,
 			     unsigned int portid, nfq_cb_t cb_func, void *data)
 {
 	const struct nlmsghdr *nlh = buf;
-	werty wert;
+	struct data_carrier dc;
 
-	wert.cb_func = cb_func;
-	wert.bufcap = bufcap;
-	wert.buflen = buflen;
-	wert.data = data;
+	dc.cb_func = cb_func;
+	dc.bufcap = bufcap;
+	dc.buflen = buflen;
+	dc.data = data;
 
 	/* Verify not multi-part */
 	if (nlh->nlmsg_flags & NLM_F_MULTI) {
@@ -55,5 +55,5 @@ EXPORT_SYMBOL int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap,
 	}
 
 
-	return mnl_cb_run(buf, buflen, 0, portid, local_cb, &wert);
+	return mnl_cb_run(buf, buflen, 0, portid, local_cb, &dc);
 }
-- 
2.17.5

[PATCH libnetfilter_queue v3 4/5] build: doc: Eliminate doxygen warnings

[Duncan Roe]

Add [struct] data_carrier to EXCLUDE_SYMBOLS.
Temporarily add pktb_populate() as well:
  to be documented once function prototypes are agreed.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
v3: New patch
 doxygen/doxygen.cfg.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doxygen/doxygen.cfg.in b/doxygen/doxygen.cfg.in
index 14bd0cf..b0791d6 100644
--- a/doxygen/doxygen.cfg.in
+++ b/doxygen/doxygen.cfg.in
@@ -13,6 +13,8 @@ EXCLUDE_SYMBOLS        = EXPORT_SYMBOL \
                          nfq_handle \
                          nfq_data \
                          nfq_q_handle \
+                         data_carrier \
+                         pktb_populate \
                          tcp_flag_word
 EXAMPLE_PATTERNS       =
 INPUT_FILTER           = "sed 's/EXPORT_SYMBOL//g'"
-- 
2.17.5

[PATCH libnetfilter_queue v3 5/5] src: struct pktbuff is no longer opaque

[Duncan Roe]

With the advent of nfq_run_cb() and pktb_populate(),
there is no longer ever a buffer tacked on the end of a struct pktbuff.
Now that struct pktbuff is purely a buffer descriptor, there seems little
point in keeping it opaque so expose it.
Some code simplification ensues: the new callback function prototype only
differs from the old one in having 1 extra arg being available free space.
An application creates a struct pktbuff instance by just declaring it.
pktb_populate() zeroises pktb because it's no longer guaranteed zero.
As before, no new doxygen documentation until function prototypes are agreed,
but examples/nf-queue.c is updated.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
v3: New patch
 examples/nf-queue.c                   |  6 +++---
 include/libnetfilter_queue/callback.h |  2 +-
 include/libnetfilter_queue/pktbuff.h  | 13 ++++++++++++-
 src/extra/callback.c                  |  4 +---
 src/extra/pktbuff.c                   |  1 +
 src/internal.h                        | 14 +-------------
 6 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index 4074e5a..f330b97 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -49,11 +49,12 @@ nfq_send_verdict(int queue_num, uint32_t id)
 }
 
 static int queue_cb(const struct nlmsghdr *nlh, void *data,
-		    struct pkt_buff *supplied_pktb, size_t supplied_extra)
+		    size_t supplied_extra)
 {
 	struct nfqnl_msg_packet_hdr *ph = NULL;
 	struct nlattr *attr[NFQA_MAX+1] = {};
 	struct pkt_buff *pktb;
+	struct pkt_buff pkt_b;
 	uint32_t id = 0, skbinfo;
 	struct nfgenmsg *nfg;
 	uint8_t *payload;
@@ -77,8 +78,7 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data,
 	plen = mnl_attr_get_payload_len(attr[NFQA_PAYLOAD]);
 	payload = mnl_attr_get_payload(attr[NFQA_PAYLOAD]);
 
-	pktb = pktb_populate(supplied_pktb, AF_INET, payload, plen,
-			     supplied_extra);
+	pktb = pktb_populate(&pkt_b, AF_INET, payload, plen, supplied_extra);
 
 	skbinfo = attr[NFQA_SKB_INFO] ? ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO])) : 0;
 
diff --git a/include/libnetfilter_queue/callback.h b/include/libnetfilter_queue/callback.h
index 27bfe31..756cb38 100644
--- a/include/libnetfilter_queue/callback.h
+++ b/include/libnetfilter_queue/callback.h
@@ -4,7 +4,7 @@
 struct nlattr;
 struct pkt_buff;
 
-typedef int (*nfq_cb_t)(const struct nlmsghdr *nlh, void *data, struct pkt_buff *pktb, size_t extra);
+typedef int (*nfq_cb_t)(const struct nlmsghdr *nlh, void *data, size_t extra);
 
 int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap, unsigned int portid, nfq_cb_t cb_data, void *data);
 
diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index 33829cc..7ad7cb1 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -1,7 +1,18 @@
 #ifndef _PKTBUFF_H_
 #define _PKTBUFF_H_
 
-struct pkt_buff;
+struct pkt_buff {
+	uint8_t *mac_header;
+	uint8_t *network_header;
+	uint8_t *transport_header;
+
+	uint8_t *data;
+
+	uint32_t len;
+	uint32_t data_len;
+
+	bool	mangled;
+};
 
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
 void pktb_free(struct pkt_buff *pktb);
diff --git a/src/extra/callback.c b/src/extra/callback.c
index dee6fc2..23e88f7 100644
--- a/src/extra/callback.c
+++ b/src/extra/callback.c
@@ -31,10 +31,9 @@ struct data_carrier
 
 static int local_cb(const struct nlmsghdr *nlh, void *data)
 {
-	struct pkt_buff pktb_instance = { };
 	struct data_carrier *d = (struct data_carrier *)data;
 
-	return d->cb_func(nlh, d->data, &pktb_instance, d->bufcap - d->buflen);
+	return d->cb_func(nlh, d->data, d->bufcap - d->buflen);
 }
 
 EXPORT_SYMBOL int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap,
@@ -54,6 +53,5 @@ EXPORT_SYMBOL int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap,
 		return MNL_CB_ERROR;
 	}
 
-
 	return mnl_cb_run(buf, buflen, 0, portid, local_cb, &dc);
 }
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 86d8fe6..ae3e454 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -125,6 +125,7 @@ EXPORT_SYMBOL
 struct pkt_buff *pktb_populate(struct pkt_buff *pktb, int family, void *data,
 			       size_t len, size_t extra)
 {
+	memset(pktb, 0, sizeof *pktb);
 	pktb_setup_metadata(pktb, data, len, extra);
 	if (__pktb_setup(family, pktb) < 0)
 		pktb = NULL;
diff --git a/src/internal.h b/src/internal.h
index ae849d6..6adf3d1 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -4,6 +4,7 @@
 #include "config.h"
 #include <stdint.h>
 #include <stdbool.h>
+#include <libnetfilter_queue/pktbuff.h>
 #ifdef HAVE_VISIBILITY_HIDDEN
 #	define EXPORT_SYMBOL __attribute__((visibility("default")))
 #else
@@ -18,19 +19,6 @@ uint16_t nfq_checksum_tcpudp_ipv4(struct iphdr *iph, uint16_t protonum);
 uint16_t nfq_checksum_tcpudp_ipv6(struct ip6_hdr *ip6h, void *transport_hdr,
 				  uint16_t protonum);
 
-struct pkt_buff {
-	uint8_t *mac_header;
-	uint8_t *network_header;
-	uint8_t *transport_header;
-
-	uint8_t *data;
-
-	uint32_t len;
-	uint32_t data_len;
-
-	bool	mangled;
-};
-
 static inline uint8_t *pktb_tail(struct pkt_buff *pktb)
 {
 	return pktb->data + pktb->len;
-- 
2.17.5

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Duncan Roe]

Guys,

Withot this patchset it remains an embarassing fact that any nfq program that
uses the deprecated libnfnetlink interface and examines packet data will use
more CPU if rewritten to use the libmnl interface.

Please apply the set or give feedback on it as a matter of urgency.

Cheers ... Duncan.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Pablo Neira Ayuso]

Hi Duncan,

On Tue, Jan 18, 2022 at 10:58:14AM +1100, Duncan Roe wrote:
> Guys,
> 
> Withot this patchset it remains an embarassing fact that any nfq program that
> uses the deprecated libnfnetlink interface and examines packet data will use
> more CPU if rewritten to use the libmnl interface.

We should untagged as deprecated libnetfilter_queue old interface in
the documentation, it's fine to keep it there, it should be possible
to make it work over libmnl.

> Please apply the set or give feedback on it as a matter of urgency.

There is absolutely no urgency.

This patch have a number of showstoppers such as exposing structure
layout on the header files.

I think the goal is to avoid the memcpy() I posted a patch sketch
time ago to follow a more simple approach that we can resurrect,
polish and upstream to remove the extra copy.

Thanks.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Duncan Roe]

On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
>
> This patch have a number of showstoppers such as exposing structure
> layout on the header files.
>
That's only in patch 5. You could apply 1-4. There are actually no other
showstoppers, right?

Cheers ... Duncan.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Florian Westphal]

Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
> >
> > This patch have a number of showstoppers such as exposing structure
> > layout on the header files.
> >
> That's only in patch 5. You could apply 1-4. There are actually no other
> showstoppers, right?

Why does patch 3 exist? Shouldn't that just get squashed into patch 1?

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Florian Westphal]

Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
> >
> > This patch have a number of showstoppers such as exposing structure
> > layout on the header files.
> >
> That's only in patch 5. You could apply 1-4. There are actually no other
> showstoppers, right?

Regarding patch 5, I think its ok except the pkt_buff layout freeze.

From a quick glance, there is no assumption that the data area resides
after the pktbuff head, so it should be possible to keep pkt_buff
private, allocate an empty packet and then associate a new buffer with
it.

I agree the memcpy needs to go, nfqueue uses should use F_GSO feature
flag and memcpy'ing 60k big packets isn't ideal.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Pablo Neira Ayuso]

On Thu, Jan 20, 2022 at 01:04:58PM +0100, Florian Westphal wrote:
> Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
> > >
> > > This patch have a number of showstoppers such as exposing structure
> > > layout on the header files.
> > >
> > That's only in patch 5. You could apply 1-4. There are actually no other
> > showstoppers, right?
> 
> Regarding patch 5, I think its ok except the pkt_buff layout freeze.
> 
> From a quick glance, there is no assumption that the data area resides
> after the pktbuff head, so it should be possible to keep pkt_buff
> private, allocate an empty packet and then associate a new buffer with
> it.

Or allocate pktbuff offline and recycle it (re-setup) on new packets
coming from the kernel, it does not need to be allocated in the
stack and exposing the layout is also therefore not requireed.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Duncan Roe]

On Thu, Jan 20, 2022 at 07:27:25AM +0100, Florian Westphal wrote:
> Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
> > >
> > > This patch have a number of showstoppers such as exposing structure
> > > layout on the header files.
> > >
> > That's only in patch 5. You could apply 1-4. There are actually no other
> > showstoppers, right?
>
> Why does patch 3 exist? Shouldn't that just get squashed into patch 1?

Didn't think of that. I have a squashed version now.

Cheers ... Duncan.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Duncan Roe]

On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
>
> We should untagged as deprecated libnetfilter_queue old interface in
> the documentation, it's fine to keep it there, it should be possible
> to make it work over libmnl.
>
Agreed. The question is, what to do instead.

We need *some* form of tagging to show whih are old interface and which are new
interface functions. You write a program using either old or new: they don't
mix. It's an early design decision which to use so we need to make it as clear
as we can.

Perhaps tag every module as NEW or OLD, with an explanation near the top of the
Main page as to what is the difference. Ideally would have libnetfilter_queue.7
man page as well.

Suggestions welcome,

Cheers ... Duncan.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Duncan Roe]

On Fri, Jan 21, 2022 at 01:36:34PM +1100, Duncan Roe wrote:
> On Thu, Jan 20, 2022 at 07:27:25AM +0100, Florian Westphal wrote:
> > Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> > > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
> > > >
> > > > This patch have a number of showstoppers such as exposing structure
> > > > layout on the header files.
> > > >
> > > That's only in patch 5. You could apply 1-4. There are actually no other
> > > showstoppers, right?
> >
> > Why does patch 3 exist? Shouldn't that just get squashed into patch 1?
>
> Didn't think of that. I have a squashed version now.
>
Also squashed patch 2 into patch 1
Cheers ... Duncan.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Duncan Roe]

On Thu, Jan 20, 2022 at 01:04:58PM +0100, Florian Westphal wrote:
> Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
> > >
> > > This patch have a number of showstoppers such as exposing structure
> > > layout on the header files.
> > >
> > That's only in patch 5. You could apply 1-4. There are actually no other
> > showstoppers, right?
>
> Regarding patch 5, I think its ok except the pkt_buff layout freeze.
>
> From a quick glance, there is no assumption that the data area resides
> after the pktbuff head, so it should be possible to keep pkt_buff
> private, allocate an empty packet and then associate a new buffer with
> it.
>
> I agree the memcpy needs to go, nfqueue uses should use F_GSO feature
> flag and memcpy'ing 60k big packets isn't ideal.

There is no pkt_buff layout freeze. If we want to change it in future, we bump
the major version of libnetfilter_queue.so, same as we would do if changing the
signature of an existing function.

Or am I missing something?

Cheers ... Duncan.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Florian Westphal]

Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> There is no pkt_buff layout freeze. If we want to change it in future, we bump
> the major version of libnetfilter_queue.so, same as we would do if changing the
> signature of an existing function.

ABI breakage is bad, just say no.

Re: [PATCH libnetfilter_queue v3 1-5/5] src: Speed-up

[Duncan Roe]

On Thu, Jan 20, 2022 at 01:40:09PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 20, 2022 at 01:04:58PM +0100, Florian Westphal wrote:
> > Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> > > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote:
> > > >
> > > > This patch have a number of showstoppers such as exposing structure
> > > > layout on the header files.
> > > >
> > > That's only in patch 5. You could apply 1-4. There are actually no other
> > > showstoppers, right?
> >
> > Regarding patch 5, I think its ok except the pkt_buff layout freeze.
> >
> > From a quick glance, there is no assumption that the data area resides
> > after the pktbuff head, so it should be possible to keep pkt_buff
> > private, allocate an empty packet and then associate a new buffer with
> > it.
>
> Or allocate pktbuff offline and recycle it (re-setup) on new packets
> coming from the kernel, it does not need to be allocated in the
> stack and exposing the layout is also therefore not requireed.

I put it on the stack to get thread locality w/out the (admittedly small)
overhead of using thread_local. We could have a pktbuff_size() to dimension it
and keep the struct opaque.

It's 48 bytes (64 bit) so I thought it was OK on the stack but you could
malloc() it and only have a pointer (again on the stack for thread locality).

Cheers ... Duncan.