[PATCH 0/4] libnetfilter_queue: gso handling support

[PATCH 0/4] libnetfilter_queue: gso handling support

[Florian Westphal]

Hello,

these are the userspace changes for gso-avoidance in nf_queue.
These are really just updates to header files to get the new flag
and attribute, plus a small change to the validation function.

Two major changes:
- the example program
- partial revert of the libmnl API change

The latter needs double-checking that everything is consistent
now.

I'll push these changes to -next branch after a couple of days
[ unless there are issues that need fixing, of course ].

Cheers,
Florian

[PATCH 1/4] Revert: add new libnetfilter_queue API for libmnl

[Florian Westphal]

This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4

Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
added in that commit.

1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
    which is part of the linux kernel API

2), we already have
    include/libnetfilter_queue/linux_nfnetlink_queue.h

which contains the same definitions/structures/macros, so it makes
little sense to have two headers in libnetfilter_queue that share
almost their entire content.

[ worse, the nfnetlink_queue.h header reverted here actually is
incompatible with mainline kernels, since a few defines have the
wrong value ... ]

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Makefile.am                               |    2 +-
 configure.ac                              |    3 +-
 include/Makefile.am                       |    2 +-
 include/linux/Makefile.am                 |    1 -
 include/linux/netfilter/Makefile.am       |    1 -
 include/linux/netfilter/nfnetlink_queue.h |   98 -----------------------------
 src/nlmsg.c                               |    2 -
 7 files changed, 3 insertions(+), 106 deletions(-)
 delete mode 100644 include/linux/Makefile.am
 delete mode 100644 include/linux/netfilter/Makefile.am
 delete mode 100644 include/linux/netfilter/nfnetlink_queue.h

diff --git a/Makefile.am b/Makefile.am
index 6b4ef77..1230dc1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,6 @@
 ACLOCAL_AMFLAGS = -I m4
 
-EXTRA_DIST = $(man_MANS) include/linux
+EXTRA_DIST = $(man_MANS)
 
 SUBDIRS = src utils include examples
 
diff --git a/configure.ac b/configure.ac
index 07747a6..649060d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -32,6 +32,5 @@ PKG_CHECK_MODULES([LIBMNL], [libmnl >= 1.0.3])
 dnl Output the makefiles
 AC_CONFIG_FILES([Makefile src/Makefile utils/Makefile examples/Makefile
         libnetfilter_queue.pc doxygen.cfg
-	include/Makefile include/libnetfilter_queue/Makefile
-	include/linux/Makefile include/linux/netfilter/Makefile])
+	include/Makefile include/libnetfilter_queue/Makefile])
 AC_OUTPUT
diff --git a/include/Makefile.am b/include/Makefile.am
index 54ea0b4..1e766d5 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -1 +1 @@
-SUBDIRS= libnetfilter_queue linux
+SUBDIRS= libnetfilter_queue
diff --git a/include/linux/Makefile.am b/include/linux/Makefile.am
deleted file mode 100644
index 38eb109..0000000
--- a/include/linux/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-SUBDIRS = netfilter
diff --git a/include/linux/netfilter/Makefile.am b/include/linux/netfilter/Makefile.am
deleted file mode 100644
index d0937cb..0000000
--- a/include/linux/netfilter/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@
-noinst_HEADERS = nfnetlink_queue.h
diff --git a/include/linux/netfilter/nfnetlink_queue.h b/include/linux/netfilter/nfnetlink_queue.h
deleted file mode 100644
index da44b33..0000000
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ /dev/null
@@ -1,98 +0,0 @@
-#ifndef _NFNETLINK_QUEUE_H
-#define _NFNETLINK_QUEUE_H
-
-#include <linux/types.h>
-#include <linux/netfilter/nfnetlink.h>
-
-enum nfqnl_msg_types {
-	NFQNL_MSG_PACKET,		/* packet from kernel to userspace */
-	NFQNL_MSG_VERDICT,		/* verdict from userspace to kernel */
-	NFQNL_MSG_CONFIG,		/* connect to a particular queue */
-	NFQNL_MSG_VERDICT_BATCH,	/* batchv from userspace to kernel */
-
-	NFQNL_MSG_MAX
-};
-
-struct nfqnl_msg_packet_hdr {
-	__be32		packet_id;	/* unique ID of packet in queue */
-	__be16		hw_protocol;	/* hw protocol (network order) */
-	__u8	hook;		/* netfilter hook */
-} __attribute__ ((packed));
-
-struct nfqnl_msg_packet_hw {
-	__be16		hw_addrlen;
-	__u16	_pad;
-	__u8	hw_addr[8];
-};
-
-struct nfqnl_msg_packet_timestamp {
-	__aligned_be64	sec;
-	__aligned_be64	usec;
-};
-
-enum nfqnl_attr_type {
-	NFQA_UNSPEC,
-	NFQA_PACKET_HDR,
-	NFQA_VERDICT_HDR,		/* nfqnl_msg_verdict_hrd */
-	NFQA_MARK,			/* __u32 nfmark */
-	NFQA_TIMESTAMP,			/* nfqnl_msg_packet_timestamp */
-	NFQA_IFINDEX_INDEV,		/* __u32 ifindex */
-	NFQA_IFINDEX_OUTDEV,		/* __u32 ifindex */
-	NFQA_IFINDEX_PHYSINDEV,		/* __u32 ifindex */
-	NFQA_IFINDEX_PHYSOUTDEV,	/* __u32 ifindex */
-	NFQA_HWADDR,			/* nfqnl_msg_packet_hw */
-	NFQA_PAYLOAD,			/* opaque data payload */
-	NFQA_CT,			/* nf_conntrack_netlink.h */
-	NFQA_CT_INFO,			/* enum ip_conntrack_info */
-
-	__NFQA_MAX
-};
-#define NFQA_MAX (__NFQA_MAX - 1)
-
-struct nfqnl_msg_verdict_hdr {
-	__be32 verdict;
-	__be32 id;
-};
-
-
-enum nfqnl_msg_config_cmds {
-	NFQNL_CFG_CMD_NONE,
-	NFQNL_CFG_CMD_BIND,
-	NFQNL_CFG_CMD_UNBIND,
-	NFQNL_CFG_CMD_PF_BIND,
-	NFQNL_CFG_CMD_PF_UNBIND,
-};
-
-struct nfqnl_msg_config_cmd {
-	__u8	command;	/* nfqnl_msg_config_cmds */
-	__u8	_pad;
-	__be16		pf;		/* AF_xxx for PF_[UN]BIND */
-};
-
-enum nfqnl_config_mode {
-	NFQNL_COPY_NONE,
-	NFQNL_COPY_META,
-	NFQNL_COPY_PACKET,
-};
-
-struct nfqnl_msg_config_params {
-	__be32		copy_range;
-	__u8	copy_mode;	/* enum nfqnl_config_mode */
-} __attribute__ ((packed));
-
-enum nfqnl_flags {
-	NFQNL_F_NONE		= 0,
-	NFQNL_F_CONNTRACK	= (1 << 0),
-};
-
-enum nfqnl_attr_config {
-	NFQA_CFG_UNSPEC,
-	NFQA_CFG_CMD,			/* nfqnl_msg_config_cmd */
-	NFQA_CFG_PARAMS,		/* nfqnl_msg_config_params */
-	NFQA_CFG_QUEUE_MAXLEN,		/* __u32 */
-	NFQA_CFG_FLAGS,			/* __u32 */
-	__NFQA_CFG_MAX
-};
-#define NFQA_CFG_MAX (__NFQA_CFG_MAX-1)
-
-#endif /* _NFNETLINK_QUEUE_H */
diff --git a/src/nlmsg.c b/src/nlmsg.c
index 6c4a139..e592ebd 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -19,8 +19,6 @@
 #define __aligned_le64 __le64 __attribute__((aligned(8)))
 #endif
 
-#include <linux/netfilter/nfnetlink_queue.h>
-
 #include <libnetfilter_queue/libnetfilter_queue.h>
 
 #include "internal.h"
-- 
1.7.8.6

Re: [PATCH 1/4] Revert: add new libnetfilter_queue API for libmnl

[Pablo Neira Ayuso]

Hi Florian,

On Thu, Apr 25, 2013 at 12:43:28PM +0200, Florian Westphal wrote:
> This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4
> 
> Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
> added in that commit.

The idea for caching that file header is to make sure
libnetfilter_queue compiles out of the box, without external linux
kernel headers installed in the system. The idea is to make sure the
libraries compilation does not break in case there's an old library
header installed in the system.

> 1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
>     which is part of the linux kernel API

As said, that file may not be available or may be stale.

> 2), we already have
>     include/libnetfilter_queue/linux_nfnetlink_queue.h

That was added initially for the old libnetfilter_queue API, I'd like
to get rid of it. It's currently being installed and it results in
(very likely) two duplicated headers in the system (the one from the
linux kernel headers and this one from libnetfilter_queue).

Once the old API is deprecated (we should do that anytime soon), we
can get finally rid of linux_nfnetlink_queue.h

> which contains the same definitions/structures/macros, so it makes
> little sense to have two headers in libnetfilter_queue that share
> almost their entire content.
> 
> [ worse, the nfnetlink_queue.h header reverted here actually is
> incompatible with mainline kernels, since a few defines have the
> wrong value ... ]

Then, please refresh it.

Re: [PATCH 1/4] Revert: add new libnetfilter_queue API for libmnl

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Apr 25, 2013 at 12:43:28PM +0200, Florian Westphal wrote:
> > This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4
> > 
> > Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
> > added in that commit.
> 
> The idea for caching that file header is to make sure
> libnetfilter_queue compiles out of the box, without external linux
> kernel headers installed in the system. The idea is to make sure the
> libraries compilation does not break in case there's an old library
> header installed in the system.
> 
> > 1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
> >     which is part of the linux kernel API
> 
> As said, that file may not be available or may be stale.

Yes, but at this time the system-wide header is used, and not this copy.
Compile of libnetfilter_queue breaks with the other patches applied
because the new attribute is missing from the system header.

> > 2), we already have
> >     include/libnetfilter_queue/linux_nfnetlink_queue.h
> 
> That was added initially for the old libnetfilter_queue API, I'd like
> to get rid of it. It's currently being installed and it results in
> (very likely) two duplicated headers in the system (the one from the
> linux kernel headers and this one from libnetfilter_queue).

Hrm, yes.  I thought that was intentional.
Or are you saying that you basically just want to rename
linux_nfnetlink_queue.h and carry a copy of nfnetlink_queue.h instead?

> Once the old API is deprecated (we should do that anytime soon), we
> can get finally rid of linux_nfnetlink_queue.h

I guess what confused me is that there is nothing related to the old
api in that header, so we might as well keep it?

> > which contains the same definitions/structures/macros, so it makes
> > little sense to have two headers in libnetfilter_queue that share
> > almost their entire content.
> > 
> > [ worse, the nfnetlink_queue.h header reverted here actually is
> > incompatible with mainline kernels, since a few defines have the
> > wrong value ... ]
> 
> Then, please refresh it.

Alright, but its not enough; the build system would require
additional treatment to give precedence to that header when compiling
the library.

Re: [PATCH 1/4] Revert: add new libnetfilter_queue API for libmnl

[Pablo Neira Ayuso]

On Fri, Apr 26, 2013 at 09:32:39AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Apr 25, 2013 at 12:43:28PM +0200, Florian Westphal wrote:
> > > This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4
> > > 
> > > Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
> > > added in that commit.
> > 
> > The idea for caching that file header is to make sure
> > libnetfilter_queue compiles out of the box, without external linux
> > kernel headers installed in the system. The idea is to make sure the
> > libraries compilation does not break in case there's an old library
> > header installed in the system.
> > 
> > > 1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
> > >     which is part of the linux kernel API
> > 
> > As said, that file may not be available or may be stale.
> 
> Yes, but at this time the system-wide header is used, and not this copy.
> Compile of libnetfilter_queue breaks with the other patches applied
> because the new attribute is missing from the system header.

Hm, it's using the local cache copy here.

I tested it by adding some new (random) attribute definition
(NFQA_XYZ) to the locally cached header file, and using it from some
of the source files (e.g. nlmsg.c).

> > > 2), we already have
> > >     include/libnetfilter_queue/linux_nfnetlink_queue.h
> > 
> > That was added initially for the old libnetfilter_queue API, I'd like
> > to get rid of it. It's currently being installed and it results in
> > (very likely) two duplicated headers in the system (the one from the
> > linux kernel headers and this one from libnetfilter_queue).
> 
> Hrm, yes.  I thought that was intentional.
> Or are you saying that you basically just want to rename
> linux_nfnetlink_queue.h and carry a copy of nfnetlink_queue.h instead?

We cannot rename that header. Some people are including it into it
into their source code and that will break their compilation.

We can add some #warning in that file to say that people should use
cache some fresh kernel header in their source code tree, or simply
tell them to use the system kernel headers.

Regards.

Re: [PATCH 1/4] Revert: add new libnetfilter_queue API for libmnl

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Yes, but at this time the system-wide header is used, and not this copy.
> > Compile of libnetfilter_queue breaks with the other patches applied
> > because the new attribute is missing from the system header.
> 
> Hm, it's using the local cache copy here.

You're right.

> > Hrm, yes.  I thought that was intentional.
> > Or are you saying that you basically just want to rename
> > linux_nfnetlink_queue.h and carry a copy of nfnetlink_queue.h instead?
> 
> We cannot rename that header. Some people are including it into it
> into their source code and that will break their compilation.

Yes, I meant "in the long run, when old API is removed".
I got rid of the revert and it works for me after refreshing
with a copy from net-next.

I hope the V2 i posted is in the direction you had in mind.

Re: [PATCH 1/4] Revert: add new libnetfilter_queue API for libmnl

[Pablo Neira Ayuso]

On Fri, Apr 26, 2013 at 12:02:47PM +0200, Florian Westphal wrote:
[...]
> > We cannot rename that header. Some people are including it into it
> > into their source code and that will break their compilation.
> 
> Yes, I meant "in the long run, when old API is removed".
> I got rid of the revert and it works for me after refreshing
> with a copy from net-next.

Feel free to post a patch for that refresh.

> I hope the V2 i posted is in the direction you had in mind.

Looks good, only missing some explanation in the doxygen documentation
on the "csum not ready" thing.

Thanks.

Re: [PATCH 1/4] Revert: add new libnetfilter_queue API for libmnl

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Looks good, only missing some explanation in the doxygen documentation
> on the "csum not ready" thing.

Thanks. I amended it:

--- a/src/libnetfilter_queue.c
+++ b/src/libnetfilter_queue.c
@@ -639,6 +639,23 @@ int nfq_set_mode(struct nfq_q_handle *qh,
  * - NFQA_CFG_F_CONNTRACK (requires Linux kernel >= 3.6): the kernel will
  *   include the Connection Tracking system information.
  *
+ * - NFQA_CFG_F_GSO (requires Linux kernel >= 3.10): the kernel will
+ *   not normalize offload packets, i.e. your application will need to
+ *   be able to handle packets larger than the mtu (up to 64k).
+ *
+ *   If your application validates checksums (e.g., tcp checksum),
+ *   then you must also check if the NFQA_SKB_INFO attribute is present.
+ *   If it is, you need to test the NFQA_SKB_CSUMNOTREADY bit:
+ * \verbatim
+       if (attr[NFQA_SKB_INFO]) {
+               uint32_t info = ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO]));
+               if (info & NFQA_SKB_CSUMNOTREADY)
+                       validate_checksums = false;
+       }
+\endverbatim
+ *  if this bit is set, the layer 3/4 checksums of the packet appear incorrect,
+ *  but are not (because they will be corrected later by the kernel).
+ *
  * Here's a little code snippet to show how to use this API:
  * \verbatim

I'll wait for a couple of more days before pushing the patches to
give others a chance to review them.

Cheers,
Florian

[PATCH 2/4] examples/nf-queue: handle recv error, use larger buffer

[Florian Westphal]

We ask for 0xffff copy size, so we need a buffer that can
hold 0xffff, plus a few more bytes to allow for netlink attributes.

Also, turn off/handle ENOBUFS.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 examples/nf-queue.c |   38 +++++++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index 7adac21..57ba483 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -1,3 +1,4 @@
+#include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -82,7 +83,8 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 
 int main(int argc, char *argv[])
 {
-	char buf[MNL_SOCKET_BUFFER_SIZE];
+	char *buf;
+	size_t sizeof_buf = 0xffff + 2084;
 	struct nlmsghdr *nlh;
 	int ret;
 	unsigned int portid, queue_num;
@@ -105,6 +107,12 @@ int main(int argc, char *argv[])
 	}
 	portid = mnl_socket_get_portid(nl);
 
+	buf = malloc(sizeof_buf);
+	if (!buf) {
+		perror("allocate receive buffer");
+		exit(EXIT_FAILURE);
+	}
+
 	nlh = nfq_hdr_put(buf, NFQNL_MSG_CONFIG, 0);
 	nfq_nlmsg_cfg_put_cmd(nlh, AF_INET, NFQNL_CFG_CMD_PF_UNBIND);
 
@@ -137,23 +145,27 @@ int main(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
-	if (ret == -1) {
-		perror("mnl_socket_recvfrom");
-		exit(EXIT_FAILURE);
-	}
-	while (ret > 0) {
-		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
-		if (ret < 0){
-			perror("mnl_cb_run");
-			exit(EXIT_FAILURE);
-		}
+	/* ENOBUFS is signalled to userspace when packets were lost
+	 * on kernel side.  In most cases, userspace isn't interested
+	 * in this information, so turn it off.
+	 */
+	ret = 1;
+	mnl_socket_setsockopt(nl, NETLINK_NO_ENOBUFS, &ret, sizeof(int));
 
-		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+	for (;;) {
+		ret = mnl_socket_recvfrom(nl, buf, sizeof_buf);
 		if (ret == -1) {
+			if (errno == ENOBUFS) /* messages were lost */
+				continue;
 			perror("mnl_socket_recvfrom");
 			exit(EXIT_FAILURE);
 		}
+
+		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
+		if (ret < 0){
+			perror("mnl_cb_run");
+			exit(EXIT_FAILURE);
+		}
 	}
 
 	mnl_socket_close(nl);
-- 
1.7.8.6

Re: [PATCH 2/4] examples/nf-queue: handle recv error, use larger buffer

[Pablo Neira Ayuso]

On Thu, Apr 25, 2013 at 12:43:29PM +0200, Florian Westphal wrote:
> We ask for 0xffff copy size, so we need a buffer that can
> hold 0xffff, plus a few more bytes to allow for netlink attributes.
> 
> Also, turn off/handle ENOBUFS.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  examples/nf-queue.c |   38 +++++++++++++++++++++++++-------------
>  1 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/examples/nf-queue.c b/examples/nf-queue.c
> index 7adac21..57ba483 100644
> --- a/examples/nf-queue.c
> +++ b/examples/nf-queue.c
> @@ -1,3 +1,4 @@
> +#include <errno.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -82,7 +83,8 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
>  
>  int main(int argc, char *argv[])
>  {
> -	char buf[MNL_SOCKET_BUFFER_SIZE];
> +	char *buf;
> +	size_t sizeof_buf = 0xffff + 2084;

I think users will appreciate a comment to explain why those black
magic numbers are there ;-). Probably using MNL_SOCKET_BUFFER_SIZE/2
instead of 2084.

>  	struct nlmsghdr *nlh;
>  	int ret;
>  	unsigned int portid, queue_num;
> @@ -105,6 +107,12 @@ int main(int argc, char *argv[])
>  	}
>  	portid = mnl_socket_get_portid(nl);
>  
> +	buf = malloc(sizeof_buf);
> +	if (!buf) {
> +		perror("allocate receive buffer");
> +		exit(EXIT_FAILURE);
> +	}
> +
>  	nlh = nfq_hdr_put(buf, NFQNL_MSG_CONFIG, 0);
>  	nfq_nlmsg_cfg_put_cmd(nlh, AF_INET, NFQNL_CFG_CMD_PF_UNBIND);
>  
> @@ -137,23 +145,27 @@ int main(int argc, char *argv[])
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
> -	if (ret == -1) {
> -		perror("mnl_socket_recvfrom");
> -		exit(EXIT_FAILURE);
> -	}
> -	while (ret > 0) {
> -		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
> -		if (ret < 0){
> -			perror("mnl_cb_run");
> -			exit(EXIT_FAILURE);
> -		}
> +	/* ENOBUFS is signalled to userspace when packets were lost
> +	 * on kernel side.  In most cases, userspace isn't interested
> +	 * in this information, so turn it off.
> +	 */
> +	ret = 1;
> +	mnl_socket_setsockopt(nl, NETLINK_NO_ENOBUFS, &ret, sizeof(int));
>  
> -		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
> +	for (;;) {
> +		ret = mnl_socket_recvfrom(nl, buf, sizeof_buf);
>  		if (ret == -1) {
> +			if (errno == ENOBUFS) /* messages were lost */

Hm, you disabled ENOBUFS errors, right?

> +				continue;
>  			perror("mnl_socket_recvfrom");
>  			exit(EXIT_FAILURE);
>  		}
> +
> +		ret = mnl_cb_run(buf, ret, 0, portid, queue_cb, NULL);
> +		if (ret < 0){
> +			perror("mnl_cb_run");
> +			exit(EXIT_FAILURE);
> +		}
>  	}
>  
>  	mnl_socket_close(nl);
> -- 
> 1.7.8.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] examples/nf-queue: handle recv error, use larger buffer

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Apr 25, 2013 at 12:43:29PM +0200, Florian Westphal wrote:
> > We ask for 0xffff copy size, so we need a buffer that can
> > hold 0xffff, plus a few more bytes to allow for netlink attributes.
> > 
> > Also, turn off/handle ENOBUFS.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  examples/nf-queue.c |   38 +++++++++++++++++++++++++-------------
> >  1 files changed, 25 insertions(+), 13 deletions(-)
> > 
> > diff --git a/examples/nf-queue.c b/examples/nf-queue.c
> > index 7adac21..57ba483 100644
> > --- a/examples/nf-queue.c
> > +++ b/examples/nf-queue.c
> > @@ -1,3 +1,4 @@
> > +#include <errno.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <unistd.h>
> > @@ -82,7 +83,8 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
> >  
> >  int main(int argc, char *argv[])
> >  {
> > -	char buf[MNL_SOCKET_BUFFER_SIZE];
> > +	char *buf;
> > +	size_t sizeof_buf = 0xffff + 2084;
> 
> I think users will appreciate a comment to explain why those black
> magic numbers are there ;-). Probably using MNL_SOCKET_BUFFER_SIZE/2
> instead of 2084.

Good point :-)
I'll do that and add a comment explaining this, e.g.
"largest possible packet payload, plus netlink data overhead"

> > +	/* ENOBUFS is signalled to userspace when packets were lost
> > +	 * on kernel side.  In most cases, userspace isn't interested
> > +	 * in this information, so turn it off.
> > +	 */
> > +	ret = 1;
> > +	mnl_socket_setsockopt(nl, NETLINK_NO_ENOBUFS, &ret, sizeof(int));
> >  
> > -		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
> > +	for (;;) {
> > +		ret = mnl_socket_recvfrom(nl, buf, sizeof_buf);
> >  		if (ret == -1) {
> > +			if (errno == ENOBUFS) /* messages were lost */
> 
> Hm, you disabled ENOBUFS errors, right?

True. I'll remove the check.

[PATCH 3/4] src: add new GSO handling capabilities

[Florian Westphal]

allows userspace to ask for large gso packets via nfqueue.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/libnetfilter_queue/linux_nfnetlink_queue.h |   13 ++++++++++++-
 src/libnetfilter_queue.c                           |    6 ++++++
 src/nlmsg.c                                        |    2 ++
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/include/libnetfilter_queue/linux_nfnetlink_queue.h b/include/libnetfilter_queue/linux_nfnetlink_queue.h
index 58c8ca5..81a485b 100644
--- a/include/libnetfilter_queue/linux_nfnetlink_queue.h
+++ b/include/libnetfilter_queue/linux_nfnetlink_queue.h
@@ -45,6 +45,10 @@ enum nfqnl_attr_type {
 	NFQA_IFINDEX_PHYSOUTDEV,	/* u_int32_t ifindex */
 	NFQA_HWADDR,			/* nfqnl_msg_packet_hw */
 	NFQA_PAYLOAD,			/* opaque data payload */
+	NFQA_CT,			/* nf_conntrack_netlink.h */
+	NFQA_CT_INFO,			/* enum ip_conntrack_info */
+	NFQA_CAP_LEN,                   /* __u32 length of captured packet */
+	NFQA_SKB_INFO,                  /* __u32 skb meta information */
 
 	__NFQA_MAX
 };
@@ -96,6 +100,13 @@ enum nfqnl_attr_config {
 /* Flags/options for NFQA_CFG_FLAGS */
 #define NFQA_CFG_F_FAIL_OPEN		(1 << 0)
 #define NFQA_CFG_F_CONNTRACK		(1 << 1)
-#define NFQA_CFG_F_MAX			(1 << 2)
+#define NFQA_CFG_F_GSO			(1 << 2)
+#define NFQA_CFG_F_MAX			(1 << 3)
+
+/* flags for NFQA_SKB_INFO */
+/* packet appears to have wrong checksums, but they are ok */
+#define NFQA_SKB_CSUMNOTREADY (1 << 0)
+/* packet is GSO (i.e., exceeds device mtu) */
+#define NFQA_SKB_GSO (1 << 1)
 
 #endif /* _NFNETLINK_QUEUE_H */
diff --git a/src/libnetfilter_queue.c b/src/libnetfilter_queue.c
index 2894ccd..b933a2a 100644
--- a/src/libnetfilter_queue.c
+++ b/src/libnetfilter_queue.c
@@ -639,6 +639,12 @@ int nfq_set_mode(struct nfq_q_handle *qh,
  * - NFQA_CFG_F_CONNTRACK (requires Linux kernel >= 3.6): the kernel will
  *   include the Connection Tracking system information.
  *
+ * - NFQA_CFG_F_GSO (requires Linux kernel >= 3.10): the kernel will
+ *   not normalize offload packets, i.e. your application will need to
+ *   be able to handle packets larger than the mtu (up to 64k) and will
+ *   need to check the NFQA_SKB_INFO attribute to determine when ip/tcp
+ *   checksums are valid even if they appear to be invalid.
+ *
  * Here's a little code snippet to show how to use this API:
  * \verbatim
 	uint32_t flags = NFQA_CFG_F_FAIL_OPEN;
diff --git a/src/nlmsg.c b/src/nlmsg.c
index e592ebd..37fa6b8 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -130,6 +130,8 @@ static int nfq_pkt_parse_attr_cb(const struct nlattr *attr, void *data)
 	case NFQA_IFINDEX_OUTDEV:
 	case NFQA_IFINDEX_PHYSINDEV:
 	case NFQA_IFINDEX_PHYSOUTDEV:
+	case NFQA_CAP_LEN:
+	case NFQA_SKB_INFO:
 		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
 			return MNL_CB_ERROR;
 		break;
-- 
1.7.8.6

[PATCH 4/4] examples/nf-queue: receive large gso packets

[Florian Westphal]

also, do not include
<linux/netfilter/nfnetlink_queue.h>.

problem is that the include guard in that file means
that linux_nfnetlink_queue.h (local copy of nfnetlink_queue.h in
libnetfiler_queue) will be empty, ie. the SKB_INFO/GSO attributes
added in the previous commit are not exposed.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 examples/nf-queue.c |   31 +++++++++++++++++++++++++++----
 1 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index 57ba483..67c9be2 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -11,8 +11,6 @@
 #include <linux/netfilter/nfnetlink.h>
 
 #include <linux/types.h>
-#include <linux/netfilter/nfnetlink_queue.h>
-
 #include <libnetfilter_queue/libnetfilter_queue.h>
 
 static struct mnl_socket *nl;
@@ -51,7 +49,7 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nfqnl_msg_packet_hdr *ph = NULL;
 	struct nlattr *attr[NFQA_MAX+1] = {};
-	uint32_t id = 0;
+	uint32_t id = 0, skbinfo;
 	struct nfgenmsg *nfg;
 	uint16_t plen;
 
@@ -72,10 +70,32 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 	plen = mnl_attr_get_payload_len(attr[NFQA_PAYLOAD]);
 	/* void *payload = mnl_attr_get_payload(attr[NFQA_PAYLOAD]); */
 
+	skbinfo = attr[NFQA_SKB_INFO] ? ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO])) : 0;
+
+	if (attr[NFQA_CAP_LEN]) {
+		uint32_t orig_len = ntohl(mnl_attr_get_u32(attr[NFQA_CAP_LEN]));
+		if (orig_len != plen)
+			printf("truncated ");
+	}
+
+	if (skbinfo & NFQA_SKB_GSO)
+		printf("GSO ");
+
 	id = ntohl(ph->packet_id);
-	printf("packet received (id=%u hw=0x%04x hook=%u, payload len %u)\n",
+	printf("packet received (id=%u hw=0x%04x hook=%u, payload len %u",
 		id, ntohs(ph->hw_protocol), ph->hook, plen);
 
+	/*
+	 * ip/tcp checksums are not yet valid, e.g. due to GRO/GSO.
+	 * The application should behave as if the checksums are correct.
+	 *
+	 * If these packets are later forwarded/sent out, the checksums will
+	 * be corrected by kernel/hardware.
+	 */
+	if (skbinfo & NFQA_SKB_CSUMNOTREADY)
+		printf(", checksum not ready");
+	puts(")");
+
 	nfq_send_verdict(ntohs(nfg->res_id), id);
 
 	return MNL_CB_OK;
@@ -140,6 +160,9 @@ int main(int argc, char *argv[])
 	nlh = nfq_hdr_put(buf, NFQNL_MSG_CONFIG, queue_num);
 	nfq_nlmsg_cfg_put_params(nlh, NFQNL_COPY_PACKET, 0xffff);
 
+	mnl_attr_put_u32(nlh, NFQA_CFG_FLAGS, htonl(NFQA_CFG_F_GSO));
+	mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(NFQA_CFG_F_GSO));
+
 	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
 		perror("mnl_socket_send");
 		exit(EXIT_FAILURE);
-- 
1.7.8.6