[PATCH libnetfilter_queue 0/3] pktbuff API updates

[PATCH libnetfilter_queue 0/3] pktbuff API updates

[Pablo Neira Ayuso]

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?

Thanks.

P.S: I'm sorry for the time being, it's been hectic here.

Pablo Neira Ayuso (3):
  pktbuff: add pktb_alloc_head() and pktb_build_data()
  example: nf-queue: use pkt_buff
  pktbuff: add pktb_reset_network_header() and pktb_set_network_header()

 examples/nf-queue.c                  | 25 +++++++++++++++++++--
 include/libnetfilter_queue/pktbuff.h |  6 +++++
 src/extra/pktbuff.c                  | 33 ++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

--
2.20.1

[PATCH libnetfilter_queue 1/3] pktbuff: add pktb_alloc_head() and pktb_build_data()

[Pablo Neira Ayuso]

Add two new helper functions to skip memcpy()'ing the payload from the
netlink message.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/libnetfilter_queue/pktbuff.h |  3 +++
 src/extra/pktbuff.c                  | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index 42bc153ec337..f9bddaf072fb 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -4,8 +4,11 @@
 struct pkt_buff;
 
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
+struct pkt_buff *pktb_alloc_head(void);
 void pktb_free(struct pkt_buff *pktb);
 
+void pktb_build_data(struct pkt_buff *pktb, uint8_t *payload, uint32_t len);
+
 uint8_t *pktb_data(struct pkt_buff *pktb);
 uint32_t pktb_len(struct pkt_buff *pktb);
 
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 6dd0ca98aff2..a93e72ac7795 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -93,6 +93,26 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	return pktb;
 }
 
+EXPORT_SYMBOL
+struct pkt_buff *pktb_alloc_head(void)
+{
+	struct pkt_buff *pktb;
+
+	pktb = calloc(1, sizeof(struct pkt_buff));
+	if (pktb == NULL)
+		return NULL;
+
+	return pktb;
+}
+
+EXPORT_SYMBOL
+void pktb_build_data(struct pkt_buff *pktb, uint8_t *payload, uint32_t len)
+{
+	pktb->len = len;
+	pktb->data_len = len;
+	pktb->data = payload;
+}
+
 /**
  * pktb_data - get pointer to network packet
  * \param pktb Pointer to userspace packet buffer
-- 
2.20.1

Re: [PATCH libnetfilter_queue 1/3] pktbuff: add pktb_alloc_head() and pktb_build_data()

[Duncan Roe]

On Sun, Apr 26, 2020 at 03:23:54PM +0200, Pablo Neira Ayuso wrote:
> Add two new helper functions to skip memcpy()'ing the payload from the
> netlink message.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  include/libnetfilter_queue/pktbuff.h |  3 +++
>  src/extra/pktbuff.c                  | 20 ++++++++++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
> index 42bc153ec337..f9bddaf072fb 100644
> --- a/include/libnetfilter_queue/pktbuff.h
> +++ b/include/libnetfilter_queue/pktbuff.h
> @@ -4,8 +4,11 @@
>  struct pkt_buff;
>
>  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
> +struct pkt_buff *pktb_alloc_head(void);
>  void pktb_free(struct pkt_buff *pktb);
>
> +void pktb_build_data(struct pkt_buff *pktb, uint8_t *payload, uint32_t len);
> +
>  uint8_t *pktb_data(struct pkt_buff *pktb);
>  uint32_t pktb_len(struct pkt_buff *pktb);
>
> diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> index 6dd0ca98aff2..a93e72ac7795 100644
> --- a/src/extra/pktbuff.c
> +++ b/src/extra/pktbuff.c
> @@ -93,6 +93,26 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>  	return pktb;
>  }
>
> +EXPORT_SYMBOL
> +struct pkt_buff *pktb_alloc_head(void)

I think we agreed to dispense with this function?
> +{
> +	struct pkt_buff *pktb;
> +
> +	pktb = calloc(1, sizeof(struct pkt_buff));
The callback (CB) needs to zeroise head: calling calloc() here is ineffective.
At least the *mangled* flag must be cleared, also the new *copy_done* flag.
> +	if (pktb == NULL)
> +		return NULL;
The above 2 lines are unnecessary
> +
> +	return pktb;
> +}
> +
> +EXPORT_SYMBOL
> +void pktb_build_data(struct pkt_buff *pktb, uint8_t *payload, uint32_t len)
> +{
> +	pktb->len = len;
> +	pktb->data_len = len;
> +	pktb->data = payload;
Also
+	mangled = false;
Maybe nullify the other pointers?
> +}
> +
>  /**
>   * pktb_data - get pointer to network packet
>   * \param pktb Pointer to userspace packet buffer
> --
> 2.20.1
>
-Duncan

[PATCH libnetfilter_queue 2/3] example: nf-queue: use pkt_buff

[Pablo Neira Ayuso]

Update example file to use the pkt_buff structure to store the payload.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 examples/nf-queue.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index 3da2c249da23..f0d4c2ee7276 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -14,6 +14,7 @@
 #include <linux/netfilter/nfnetlink_queue.h>
 
 #include <libnetfilter_queue/libnetfilter_queue.h>
+#include <libnetfilter_queue/pktbuff.h>
 
 /* only for NFQA_CT, not needed otherwise: */
 #include <linux/netfilter/nfnetlink_conntrack.h>
@@ -50,9 +51,12 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nfqnl_msg_packet_hdr *ph = NULL;
 	struct nlattr *attr[NFQA_MAX+1] = {};
+	struct pkt_buff *pktb = data;
 	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 +73,8 @@ 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]); */
+
+	pktb_build_data(pktb, mnl_attr_get_payload(attr[NFQA_PAYLOAD]), plen);
 
 	skbinfo = attr[NFQA_SKB_INFO] ? ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO])) : 0;
 
@@ -97,6 +102,14 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 		printf(", checksum not ready");
 	puts(")");
 
+	printf("payload (len=%d) [", plen);
+	payload = pktb_data(pktb);
+
+	for (i = 0; i < pktb_len(pktb); i++)
+		printf("%x", payload[i] & 0xff);
+
+	printf("]\n");
+
 	nfq_send_verdict(ntohs(nfg->res_id), id);
 
 	return MNL_CB_OK;
@@ -107,6 +120,7 @@ int main(int argc, char *argv[])
 	char *buf;
 	/* largest possible packet payload, plus netlink data overhead: */
 	size_t sizeof_buf = 0xffff + (MNL_SOCKET_BUFFER_SIZE/2);
+	struct pkt_buff *pktb;
 	struct nlmsghdr *nlh;
 	int ret;
 	unsigned int portid, queue_num;
@@ -161,6 +175,12 @@ int main(int argc, char *argv[])
 	ret = 1;
 	mnl_socket_setsockopt(nl, NETLINK_NO_ENOBUFS, &ret, sizeof(int));
 
+	pktb = pktb_alloc_head();
+	if (!pktb) {
+		perror("pktb_alloc");
+		exit(EXIT_FAILURE);
+	}
+
 	for (;;) {
 		ret = mnl_socket_recvfrom(nl, buf, sizeof_buf);
 		if (ret == -1) {
@@ -168,13 +188,14 @@ int main(int argc, char *argv[])
 			exit(EXIT_FAILURE);
 		}
 
-		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
+		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, pktb);
 		if (ret < 0){
 			perror("mnl_cb_run");
 			exit(EXIT_FAILURE);
 		}
 	}
 
+	pktb_free(pktb);
 	mnl_socket_close(nl);
 
 	return 0;
-- 
2.20.1

Re: [PATCH libnetfilter_queue 2/3] example: nf-queue: use pkt_buff

[Duncan Roe]

Hi Pablo,

To hopefully save you time, the email after this is a patch implementing the
suggestions below, and also using the 5-args pktb_setup() interface which we
haven't agreed on yet.

Cheers ... Duncan.

On Sun, Apr 26, 2020 at 03:23:55PM +0200, Pablo Neira Ayuso wrote:
> Update example file to use the pkt_buff structure to store the payload.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  examples/nf-queue.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/examples/nf-queue.c b/examples/nf-queue.c
> index 3da2c249da23..f0d4c2ee7276 100644
> --- a/examples/nf-queue.c
> +++ b/examples/nf-queue.c
> @@ -14,6 +14,7 @@
>  #include <linux/netfilter/nfnetlink_queue.h>
>
>  #include <libnetfilter_queue/libnetfilter_queue.h>
> +#include <libnetfilter_queue/pktbuff.h>
>
>  /* only for NFQA_CT, not needed otherwise: */
>  #include <linux/netfilter/nfnetlink_conntrack.h>
> @@ -50,9 +51,12 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
>  {
>  	struct nfqnl_msg_packet_hdr *ph = NULL;
>  	struct nlattr *attr[NFQA_MAX+1] = {};
> +	struct pkt_buff *pktb = data;
>  	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 +73,8 @@ 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]); */
> +
> +	pktb_build_data(pktb, mnl_attr_get_payload(attr[NFQA_PAYLOAD]), plen);
>
>  	skbinfo = attr[NFQA_SKB_INFO] ? ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO])) : 0;
>
> @@ -97,6 +102,14 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
>  		printf(", checksum not ready");
>  	puts(")");
>
> +	printf("payload (len=%d) [", plen);
> +	payload = pktb_data(pktb);
> +
> +	for (i = 0; i < pktb_len(pktb); i++)

'& 0xff' not necessary for uint8_t
"%02x" gives output that is more useful (longer though)

> +		printf("%x", payload[i] & 0xff);
> +
> +	printf("]\n");
> +
>  	nfq_send_verdict(ntohs(nfg->res_id), id);
>
>  	return MNL_CB_OK;
> @@ -107,6 +120,7 @@ int main(int argc, char *argv[])
>  	char *buf;
>  	/* largest possible packet payload, plus netlink data overhead: */
>  	size_t sizeof_buf = 0xffff + (MNL_SOCKET_BUFFER_SIZE/2);
> +	struct pkt_buff *pktb;
>  	struct nlmsghdr *nlh;
>  	int ret;
>  	unsigned int portid, queue_num;
> @@ -161,6 +175,12 @@ int main(int argc, char *argv[])
>  	ret = 1;
>  	mnl_socket_setsockopt(nl, NETLINK_NO_ENOBUFS, &ret, sizeof(int));
>
> +	pktb = pktb_alloc_head();

s.b. pktb_head_alloc (did this compile?)

See following patch for simplified code

> +	if (!pktb) {
> +		perror("pktb_alloc");
> +		exit(EXIT_FAILURE);
> +	}
> +
>  	for (;;) {
>  		ret = mnl_socket_recvfrom(nl, buf, sizeof_buf);
>  		if (ret == -1) {
> @@ -168,13 +188,14 @@ int main(int argc, char *argv[])
>  			exit(EXIT_FAILURE);
>  		}
>
> -		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
> +		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, pktb);
>  		if (ret < 0){
>  			perror("mnl_cb_run");
>  			exit(EXIT_FAILURE);
>  		}
>  	}
>
> +	pktb_free(pktb);
>  	mnl_socket_close(nl);
>
>  	return 0;
> --
> 2.20.1
>

Duncan Roe (1):
  example: nf-queue: use pkt_buff (updated)

 examples/nf-queue.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

-- 
2.14.5

[PATCH libnetfilter_queue 1/1] example: nf-queue: use pkt_buff (updated)

[Duncan Roe]

- Use the 5-args pktb_setup() variant.
- Demonstrate using pktb_head_size() once in main() to set size of array
  in callback stack.
- Output 2 hex digits per character.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 examples/nf-queue.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index f0d4c2e..1d0cba4 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -20,6 +20,7 @@
 #include <linux/netfilter/nfnetlink_conntrack.h>
 
 static struct mnl_socket *nl;
+static size_t sizeof_pktbuff;
 
 static void
 nfq_send_verdict(int queue_num, uint32_t id)
@@ -51,7 +52,8 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nfqnl_msg_packet_hdr *ph = NULL;
 	struct nlattr *attr[NFQA_MAX+1] = {};
-	struct pkt_buff *pktb = data;
+	struct pkt_buff *pktb;
+	uint8_t pktbuff[sizeof_pktbuff];
 	uint32_t id = 0, skbinfo;
 	struct nfgenmsg *nfg;
 	uint8_t *payload;
@@ -74,7 +76,8 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 
 	plen = mnl_attr_get_payload_len(attr[NFQA_PAYLOAD]);
 
-	pktb_build_data(pktb, mnl_attr_get_payload(attr[NFQA_PAYLOAD]), plen);
+	pktb = pktb_setup(AF_INET, pktbuff, sizeof pktbuff,
+			  mnl_attr_get_payload(attr[NFQA_PAYLOAD]), plen);
 
 	skbinfo = attr[NFQA_SKB_INFO] ? ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO])) : 0;
 
@@ -106,7 +109,7 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 	payload = pktb_data(pktb);
 
 	for (i = 0; i < pktb_len(pktb); i++)
-		printf("%x", payload[i] & 0xff);
+		printf("%02x", payload[i]);
 
 	printf("]\n");
 
@@ -120,7 +123,6 @@ int main(int argc, char *argv[])
 	char *buf;
 	/* largest possible packet payload, plus netlink data overhead: */
 	size_t sizeof_buf = 0xffff + (MNL_SOCKET_BUFFER_SIZE/2);
-	struct pkt_buff *pktb;
 	struct nlmsghdr *nlh;
 	int ret;
 	unsigned int portid, queue_num;
@@ -174,12 +176,7 @@ int main(int argc, char *argv[])
 	 */
 	ret = 1;
 	mnl_socket_setsockopt(nl, NETLINK_NO_ENOBUFS, &ret, sizeof(int));
-
-	pktb = pktb_alloc_head();
-	if (!pktb) {
-		perror("pktb_alloc");
-		exit(EXIT_FAILURE);
-	}
+	sizeof_pktbuff = pktb_head_size(); /* Avoid multiple calls in CB */
 
 	for (;;) {
 		ret = mnl_socket_recvfrom(nl, buf, sizeof_buf);
@@ -188,14 +185,13 @@ int main(int argc, char *argv[])
 			exit(EXIT_FAILURE);
 		}
 
-		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, pktb);
+		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
 		if (ret < 0){
 			perror("mnl_cb_run");
 			exit(EXIT_FAILURE);
 		}
 	}
 
-	pktb_free(pktb);
 	mnl_socket_close(nl);
 
 	return 0;
-- 
2.14.5

[PATCH libnetfilter_queue 3/3] pktbuff: add pktb_reset_network_header() and pktb_set_network_header()

[Pablo Neira Ayuso]

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/libnetfilter_queue/pktbuff.h |  3 +++
 src/extra/pktbuff.c                  | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index f9bddaf072fb..875157922c81 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -9,6 +9,9 @@ void pktb_free(struct pkt_buff *pktb);
 
 void pktb_build_data(struct pkt_buff *pktb, uint8_t *payload, uint32_t len);
 
+void pktb_reset_network_header(struct pkt_buff *pktb);
+void pktb_set_network_header(struct pkt_buff *pktb, const int offset);
+
 uint8_t *pktb_data(struct pkt_buff *pktb);
 uint32_t pktb_len(struct pkt_buff *pktb);
 
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index a93e72ac7795..3ff287e57315 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -267,6 +267,19 @@ uint8_t *pktb_network_header(struct pkt_buff *pktb)
 	return pktb->network_header;
 }
 
+EXPORT_SYMBOL
+void pktb_reset_network_header(struct pkt_buff *pktb)
+{
+	pktb->network_header = pktb->data;
+}
+
+EXPORT_SYMBOL
+void pktb_set_network_header(struct pkt_buff *pktb, const int offset)
+{
+	pktb_reset_network_header(pktb);
+	pktb->network_header += offset;
+}
+
 /**
  * pktb_transport_header - get address of layer 4 header (if known)
  * \param pktb Pointer to userspace packet buffer
-- 
2.20.1

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

[Duncan Roe]

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.

For "extra" support, you need something with the sophistication of pktb_malloc2.
If extra == 0, pktb_malloc2 optimises by leaving the packet data where it was.
Actually pktb_malloc2 doesn't need to make this decision. That can be deferred
to pktb_mangle, which could do the copy if it has been told to expand a packet
and the copy has not already been done (new "copy done" flag in the opaque
struct pkt_buff).

My nfq-based accidentally-written ad blocker would benefit from that deferment -
I allow extra bytes in case I have to lengthen a domain name, but most of the
time I'm shortening them.
>
> Thanks.
>
> P.S: I'm sorry for the time being, it's been hectic here.
>
> Pablo Neira Ayuso (3):
>   pktbuff: add pktb_alloc_head() and pktb_build_data()
>   example: nf-queue: use pkt_buff
>   pktbuff: add pktb_reset_network_header() and pktb_set_network_header()
>
>  examples/nf-queue.c                  | 25 +++++++++++++++++++--
>  include/libnetfilter_queue/pktbuff.h |  6 +++++
>  src/extra/pktbuff.c                  | 33 ++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+), 2 deletions(-)
>
> --
> 2.20.1
>
In https://www.spinics.net/lists/netfilter-devel/msg65830.html, you suggested a
pair of functions: pktb_alloc2 & pktb_head_size.

I really prefer that to your new suggestions.

More later,

Cheers ... Duncan.

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

[Pablo Neira Ayuso]

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.

> For "extra" support, you need something with the sophistication of pktb_malloc2.
> If extra == 0, pktb_malloc2 optimises by leaving the packet data where it was.

With this patchset, the user is in control of the data buffer memory
area that is attached to the pkt_buff head, so you can just allocate
the as many extra byte as you require.

> Actually pktb_malloc2 doesn't need to make this decision. That can be deferred
> to pktb_mangle, which could do the copy if it has been told to expand a packet
> and the copy has not already been done (new "copy done" flag in the opaque
> struct pkt_buff).

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.

> My nfq-based accidentally-written ad blocker would benefit from that deferment -
> I allow extra bytes in case I have to lengthen a domain name, but most of the
> time I'm shortening them.

Thanks for explaining.

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

[Duncan Roe]

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);

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. 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)

maybe with a better name for buf - data_copy_buf?

>
> > For "extra" support,
> > you need something with the sophistication of pktb_malloc2.
> > If extra == 0,
> > pktb_malloc2 optimises by leaving the packet data where it was.
>
> With this patchset, the user is in control of the data buffer memory
> area that is attached to the pkt_buff head, so you can just allocate
> the as many extra byte as you require.
>
> > Actually pktb_malloc2 doesn't need to make this decision.
> > That can be deferred to pktb_mangle,
> > which could do the copy if it has been told to expand a packet
> > and the copy has not already been done (new "copy done" flag in the opaque
> > struct pkt_buff).
>
> 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
>
> > My nfq-based accidentally-written ad blocker would benefit from that
> > deferment - I allow extra bytes in case I have to lengthen a domain name,
> > but most of the time I'm shortening them.
>
> Thanks for explaining.

Cheers ... Duncan.

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

[Pablo Neira Ayuso]

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.

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

[Duncan Roe]

On Tue, Apr 28, 2020 at 12:34:07PM +0200, Pablo Neira Ayuso wrote:
> 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.

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).

"depending on its contents" is where the memcpy deferral comes in. pktb_alloc2()
verifies that the supplied buffer is big enough (size >= len + extra). The user
declared it as a stack variable that size so it will be. With the deferral
enhancement, pktb_alloc2() records the buffer address and extra in the enlarged
struct pktbuff (extra is needed to tell pktb_mangle how much memory to memset to
0).

If pktb_mangle() finds it has to make the packet larger then its original length
and the packet is still in its original location then copy it and zero extra.
(i.e. pktb_mangle() doesn't just check whether it was asked to make the packet
bigger: it might have previously been asked to make it smaller).

Also (and this *is* tricky, update relevant pointers in the struct pktbuff).
That invalidates any poiners the caller may have obtained from e.g. pktb_data()
- see end of email.
>
> > 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.

Yes it's more complex. No problem with the buffer - the user gave that to
pktb_alloc2().

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.

Still, I really like the deferred copy enhancement. Your thoughts?

Cheers ... Duncan.

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

[Pablo Neira Ayuso]

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?

> "depending on its contents" is where the memcpy deferral comes in. pktb_alloc2()
> verifies that the supplied buffer is big enough (size >= len + extra). The user
> declared it as a stack variable that size so it will be. With the deferral
> enhancement, pktb_alloc2() records the buffer address and extra in the enlarged
> struct pktbuff (extra is needed to tell pktb_mangle how much memory to memset to
> 0).

I agree that deferring the memcpy() and avoiding the malloc() is the
way to go, we only have to agree in the way to achieve this.

> If pktb_mangle() finds it has to make the packet larger then its original length
> and the packet is still in its original location then copy it and zero extra.
> (i.e. pktb_mangle() doesn't just check whether it was asked to make the packet
> bigger: it might have previously been asked to make it smaller).
>
> Also (and this *is* tricky, update relevant pointers in the struct pktbuff).
> That invalidates any poiners the caller may have obtained from e.g. pktb_data()
> - see end of email.

Regarding pktb_mangle() reallocation case, refetching the pointers
sounds fine, documenting this is sufficient.

[...]
> > 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.
> 
> 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).

If you can just pre-allocate the pkt_buff head from the configuration
phase (before receiving packets from the kernel), then attach the
buffer via pktb_setup_metadata() for each packet that is received (so
the pkt_buff head is recycled). With this approach, pktb_head_size()
won't be needed either.

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.

> 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?

> Still, I really like the deferred copy enhancement. Your thoughts?

The deferred copy idea when mangling sounds fine, we only have to
agree on how to get this done.

Thanks.

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

[Duncan Roe]

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.
>
> > "depending on its contents" is where the memcpy deferral comes in. pktb_alloc2()
> > verifies that the supplied buffer is big enough (size >= len + extra). The user
> > declared it as a stack variable that size so it will be. With the deferral
> > enhancement, pktb_alloc2() records the buffer address and extra in the enlarged
> > struct pktbuff (extra is needed to tell pktb_mangle how much memory to memset to
> > 0).
>
> I agree that deferring the memcpy() and avoiding the malloc() is the
> way to go, we only have to agree in the way to achieve this.
>
> > If pktb_mangle() finds it has to make the packet larger then its original length
> > and the packet is still in its original location then copy it and zero extra.
> > (i.e. pktb_mangle() doesn't just check whether it was asked to make the packet
> > bigger: it might have previously been asked to make it smaller).
> >
> > Also (and this *is* tricky, update relevant pointers in the struct pktbuff).
> > That invalidates any poiners the caller may have obtained from e.g. pktb_data()
> > - see end of email.
>
> Regarding pktb_mangle() reallocation case, refetching the pointers
> sounds fine, documenting this is sufficient.
>
> [...]
> > > 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.

I reckon they'll just copy the code from the nfq_nlmsg_verdict_put_pkt() man /
web page. After declaring "char pktbuf[plen + EXTRA];" one can use "sizeof
pktbuf" as the length argument.
> >
> > 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.
>
> If you can just pre-allocate the pkt_buff head from the configuration
> phase (before receiving packets from the kernel), then attach the
> buffer via pktb_setup_metadata() for each packet that is received (so
> the pkt_buff head is recycled). With this approach, pktb_head_size()
> won't be needed either.

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];

>
> 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).
>
> > 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.
>
> > Still, I really like the deferred copy enhancement. Your thoughts?
>
> The deferred copy idea when mangling sounds fine, we only have to
> agree on how to get this done.
>
> Thanks.

Cheers ... Duncan.

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

[Pablo Neira Ayuso]

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.

> 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().

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.

Thanks.

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

[Duncan Roe]

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.

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

[Pablo Neira Ayuso]

On Thu, Apr 30, 2020 at 05:54:14AM +1000, Duncan Roe wrote:
> 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:
> > > > [...]
> > > 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]

In nf_queue, the way to go is to set the _GSO flag on the client and
then set on NFT_QUEUE_FLAG_CPU_FANOUT from the nft queue rule. If you
need multiple processes, then users pre-allocate one pkt_buff per
process.

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

[Duncan Roe]

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.
> >
> > > "depending on its contents" is where the memcpy deferral comes in. pktb_alloc2()
> > > verifies that the supplied buffer is big enough (size >= len + extra). The user
> > > declared it as a stack variable that size so it will be. With the deferral
> > > enhancement, pktb_alloc2() records the buffer address and extra in the enlarged
> > > struct pktbuff (extra is needed to tell pktb_mangle how much memory to memset to
> > > 0).
> >
> > I agree that deferring the memcpy() and avoiding the malloc() is the
> > way to go, we only have to agree in the way to achieve this.
> >
> > > If pktb_mangle() finds it has to make the packet larger then its original length
> > > and the packet is still in its original location then copy it and zero extra.
> > > (i.e. pktb_mangle() doesn't just check whether it was asked to make the packet
> > > bigger: it might have previously been asked to make it smaller).
> > >
> > > Also (and this *is* tricky, update relevant pointers in the struct pktbuff).
> > > That invalidates any poiners the caller may have obtained from e.g. pktb_data()
> > > - see end of email.
> >
> > Regarding pktb_mangle() reallocation case, refetching the pointers
> > sounds fine, documenting this is sufficient.
> >
> > [...]
> > > > 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.
>
> I reckon they'll just copy the code from the nfq_nlmsg_verdict_put_pkt() man /
> web page. After declaring "char pktbuf[plen + EXTRA];" one can use "sizeof
> pktbuf" as the length argument.
> > >
> > > 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.
> >
> > If you can just pre-allocate the pkt_buff head from the configuration
> > phase (before receiving packets from the kernel), then attach the
> > buffer via pktb_setup_metadata() for each packet that is received (so
> > the pkt_buff head is recycled). With this approach, pktb_head_size()
> > won't be needed either.
>
> 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];
>
> >
> > 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).
> >
> > > 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.
> >
> > > Still, I really like the deferred copy enhancement. Your thoughts?
> >
> > The deferred copy idea when mangling sounds fine, we only have to
> > agree on how to get this done.
> >
> > Thanks.
>
> Cheers ... Duncan.

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.

Cheers ... Duncan.

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

[Pablo Neira Ayuso]

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
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.

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

[Duncan Roe]

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.

> 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?

--- 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()?

Cheers ... Duncan.

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

[Pablo Neira Ayuso]

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@netfilter.org/

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

[Duncan Roe]

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/

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

[Duncan Roe]

On Thu, Apr 30, 2020 at 04:34:04PM +1000, Duncan Roe wrote:
> 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/

To be clear, the email reference was to

https://www.spinics.net/lists/netfilter-devel/msg65830.html

I should have put a comma between that and "except s/head/buf/"

Sorry about that,

Cheers ... Duncan.

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

[Pablo Neira Ayuso]

Hi Duncan,

On Thu, Apr 30, 2020 at 04:34:04PM +1000, Duncan Roe wrote:
[..]
> 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);

Getting better. But why do you still need '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().

NFQ_BUFFER_SIZE tells what is the maximum netlink message size coming
from the kernel. That netlink message contains metadata and the actual
payload data.

The pktbuff structure helps you deal with the payload data, not the
netlink message itself.

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

[Duncan Roe]

Hi Pablo,

On Tue, May 05, 2020 at 02:30:34PM +0200, Pablo Neira Ayuso wrote:
> Hi Duncan,
>
> On Thu, Apr 30, 2020 at 04:34:04PM +1000, Duncan Roe wrote:
> [..]
> > 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);
>
> Getting better. But why do you still need '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().
>
> NFQ_BUFFER_SIZE tells what is the maximum netlink message size coming
> from the kernel. That netlink message contains metadata and the actual
> payload data.
>
> The pktbuff structure helps you deal with the payload data, not the
> netlink message itself.

2 reasons, the first being more important:

1. We zeroise memory from 'data + len' for 'extra' bytes. This mirrors original
behaviour where calloc() was used to zeroise everything. Zeroising is only done
if a data copy is needed to mangle packet length to be larger than it was
originally. Do we need to zeroise at all? You tell me. We do need to zeroise the
'struct pkt_buff' - was that why calloc() was originally used?

2. We use extra to verify that 'buf_size' is big enough. It must be at least
'sizeof(struct pkt_buff) + (extra ? len + extra : 0)'.

If zeroising is unnecessary then yes, we don't need 'extra'. pktb_mangle() can
return 0 if 'buf_size' is inadequate. (pktb_alloc2() checks 'buf_size >=
sizeof(struct pkt_buff)' and copies 'buf_size' into the enlarged 'pktb' so it's
available to pktb_mangle()).

Cheeers ... Duncan.

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

[Duncan Roe]

On Wed, May 06, 2020 at 10:57:15AM +1000, Duncan Roe wrote:
[...]
> If zeroising is unnecessary then yes, we don't need 'extra'. pktb_mangle() can
> return 0 if 'buf_size' is inadequate. (pktb_alloc2() checks 'buf_size >=
> sizeof(struct pkt_buff)' and copies 'buf_size' into the enlarged 'pktb'
> so it's available to pktb_mangle()).

No need for new element - we already have data_len
>
> Cheeers ... Duncan.

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

[Duncan Roe]

Hi Pablo,

On Tue, May 05, 2020 at 02:30:34PM +0200, Pablo Neira Ayuso wrote: [...]
> > And we tell users to dimension buf to NFQ_BUFFER_SIZE. We don't even need to
> > expose pktb_head_size().

On second thoughts, maybe document in a Note the actual formula for how big the
buffer needs to be. And keep pktb_head_size().
>
> NFQ_BUFFER_SIZE tells what is the maximum netlink message size coming
> from the kernel. That netlink message contains metadata and the actual
> payload data.

I meant NFQ_BUFFER_SIZE (or some better name) to be a new macro expanding to
'0xffff + (MNL_SOCKET_BUFFER_SIZE/2)' as you suggested in
https://www.spinics.net/lists/netfilter-devel/msg66938.html. Is that only just
large enough for largest possible packet? Or is there room for struct pkt_buff
as well?

Cheers ... Duncan.

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

[Duncan Roe]

Hi Pablo,

On Tue, May 05, 2020 at 02:30:34PM +0200, Pablo Neira Ayuso wrote:
> Hi Duncan,
>
> On Thu, Apr 30, 2020 at 04:34:04PM +1000, Duncan Roe wrote:
> [..]
> > 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);
>
> Getting better. But why do you still need 'extra'?

As per other emails, no. I think it's fine not to zeroise that memory.
>
> > 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().
>
> NFQ_BUFFER_SIZE tells what is the maximum netlink message size coming
> from the kernel. That netlink message contains metadata and the actual
> payload data.

As per other emails, I'll define and document NFQ_BUFFER_SIZE. If you have a
suggestion for a smaller value, I can put that in a v2.
>
> The pktbuff structure helps you deal with the payload data, not the
> netlink message itself.

Pablo, can we agree to proceed with

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

then I can get on with the rest of the release.

Cheers ... Duncan.