[PATCH v2] NFQUEUE v2 target with 'queue bypass' support

[PATCH v2] NFQUEUE v2 target with 'queue bypass' support

[Florian Westphal]

This is V2 of the NFQUEUEv2 target revision, adding support for accepting
packets in case the userspace listener is not available.
This fixes issues pointed out by Pablo in his review.

See individual patches for changes vs. V1.
Patch to iptables userspace follows in a couple of minutes.

These changes are also available via git pull:

The following changes since commit d862a6622e9db508d4b28cc7c5bc28bd548cc24e:

  netfilter: nf_conntrack: use is_vmalloc_addr() (2011-01-14 15:45:56 +0100)

are available in the git repository at:
  git://git.breakpoint.cc/fw/nf-next-2.6.git nfq_bypass_v2

Florian Westphal (6):
      netfilter: kconfig: NFQUEUE is useless without NETFILTER_NETLINK_QUEUE
      netfilter: nfnetlink_queue: return error number to caller
      netfilter: nfnetlink_queue: do not free skb on error
      netfilter: reduce NF_VERDICT_MASK to 0xff
      netfilter: allow NFQUEUE bypass if no listener is available
      netfilter: do not omit re-route check on NF_QUEUE verdict

 include/linux/netfilter.h            |   21 ++++++++---
 include/linux/netfilter/xt_NFQUEUE.h |    6 +++
 net/ipv4/netfilter/iptable_mangle.c  |    2 +-
 net/netfilter/Kconfig                |    1 +
 net/netfilter/core.c                 |   16 ++++++--
 net/netfilter/nf_queue.c             |   64 ++++++++++++++++++++++++----------
 net/netfilter/nfnetlink_queue.c      |   22 +++++++----
 net/netfilter/xt_NFQUEUE.c           |   28 +++++++++++++--
 8 files changed, 120 insertions(+), 40 deletions(-)

[PATCH 1/6] netfilter: kconfig: NFQUEUE is useless without NETFILTER_NETLINK_QUEUE

[Florian Westphal]

NFLOG already does the same thing for NETFILTER_NETLINK_LOG.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 1b79353..03fc794 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -477,6 +477,7 @@ config NETFILTER_XT_TARGET_NFLOG
 config NETFILTER_XT_TARGET_NFQUEUE
 	tristate '"NFQUEUE" target Support'
 	depends on NETFILTER_ADVANCED
+	select NETFILTER_NETLINK_QUEUE
 	help
 	  This target replaced the old obsolete QUEUE target.
 
-- 
1.7.2.2

[PATCH 2/6] netfilter: nfnetlink_queue: return error number to caller

[Florian Westphal]

instead of returning -1 on error, return an error number to allow the
caller to handle some errors differently.

ECANCELED is used to indicate that the hook is going away and should be
ignored.

A followup patch will introduce more 'ignore this hook' conditions,
(depending on queue settings) and will move kfree_skb responsibility
to the caller.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
Changes since v1:
- keep 'ignore hook on try_module_get failure' via -ECANCELED retval.
- avoid renaming 'int status' in __nf_queue to reduce diff size
- if queue handler does not exist, return ENOENT instead of EINVAL.
- don't unconditionally free skbs in nf_queue() GSO path
[ all pointed out by Pablo ]

 net/netfilter/core.c            |    6 +++-
 net/netfilter/nf_queue.c        |   44 +++++++++++++++++++++++++++-----------
 net/netfilter/nfnetlink_queue.c |   22 ++++++++++++-------
 3 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e69d537..91d66d2f 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -179,9 +179,11 @@ next_hook:
 		if (ret == 0)
 			ret = -EPERM;
 	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
-		if (!nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
-			      verdict >> NF_VERDICT_BITS))
+		ret = nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
+			       verdict >> NF_VERDICT_BITS);
+		if (ret == -ECANCELED)
 			goto next_hook;
+		ret = 0;
 	}
 	rcu_read_unlock();
 	return ret;
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 1876f74..c361084 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -125,7 +125,7 @@ static int __nf_queue(struct sk_buff *skb,
 		      int (*okfn)(struct sk_buff *),
 		      unsigned int queuenum)
 {
-	int status;
+	int status = -ENOENT;
 	struct nf_queue_entry *entry = NULL;
 #ifdef CONFIG_BRIDGE_NETFILTER
 	struct net_device *physindev;
@@ -146,8 +146,10 @@ static int __nf_queue(struct sk_buff *skb,
 		goto err_unlock;
 
 	entry = kmalloc(sizeof(*entry) + afinfo->route_key_size, GFP_ATOMIC);
-	if (!entry)
+	if (!entry) {
+		status = -ENOMEM;
 		goto err_unlock;
+	}
 
 	*entry = (struct nf_queue_entry) {
 		.skb	= skb,
@@ -163,9 +165,8 @@ static int __nf_queue(struct sk_buff *skb,
 	if (!try_module_get(entry->elem->owner)) {
 		rcu_read_unlock();
 		kfree(entry);
-		return 0;
+		return -ECANCELED;
 	}
-
 	/* Bump dev refs so they don't vanish while packet is out */
 	if (indev)
 		dev_hold(indev);
@@ -192,14 +193,14 @@ static int __nf_queue(struct sk_buff *skb,
 		goto err;
 	}
 
-	return 1;
+	return 0;
 
 err_unlock:
 	rcu_read_unlock();
 err:
 	kfree_skb(skb);
 	kfree(entry);
-	return 1;
+	return status;
 }
 
 int nf_queue(struct sk_buff *skb,
@@ -211,6 +212,8 @@ int nf_queue(struct sk_buff *skb,
 	     unsigned int queuenum)
 {
 	struct sk_buff *segs;
+	int err;
+	unsigned int queued;
 
 	if (!skb_is_gso(skb))
 		return __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
@@ -227,19 +230,32 @@ int nf_queue(struct sk_buff *skb,
 
 	segs = skb_gso_segment(skb, 0);
 	kfree_skb(skb);
+	/* Does not use PTR_ERR to limit the number of error codes that can be
+	 * returned by nf_queue.  For instance, callers rely on -ECANCELED to mean
+	 * 'ignore this hook'.
+	 */
 	if (IS_ERR(segs))
-		return 1;
+		return -EINVAL;
 
+	queued = 0;
+	err = 0;
 	do {
 		struct sk_buff *nskb = segs->next;
 
 		segs->next = NULL;
-		if (!__nf_queue(segs, elem, pf, hook, indev, outdev, okfn,
-				queuenum))
+		if (err == 0)
+			err = __nf_queue(segs, elem, pf, hook, indev,
+					   outdev, okfn, queuenum);
+		if (err == 0)
+			queued++;
+		else
 			kfree_skb(segs);
 		segs = nskb;
 	} while (segs);
-	return 1;
+
+	if (unlikely(err && queued))
+		err = 0;
+	return err;
 }
 
 void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
@@ -247,6 +263,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 	struct sk_buff *skb = entry->skb;
 	struct list_head *elem = &entry->elem->list;
 	const struct nf_afinfo *afinfo;
+	int err;
 
 	rcu_read_lock();
 
@@ -280,9 +297,10 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 		local_bh_enable();
 		break;
 	case NF_QUEUE:
-		if (!__nf_queue(skb, elem, entry->pf, entry->hook,
-				entry->indev, entry->outdev, entry->okfn,
-				verdict >> NF_VERDICT_BITS))
+		err = __nf_queue(skb, elem, entry->pf, entry->hook,
+				 entry->indev, entry->outdev, entry->okfn,
+				 verdict >> NF_VERDICT_BITS);
+		if (err == -ECANCELED)
 			goto next_hook;
 		break;
 	case NF_STOLEN:
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 68e67d1..b83123f 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -387,25 +387,31 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 {
 	struct sk_buff *nskb;
 	struct nfqnl_instance *queue;
-	int err;
+	int err = -ENOBUFS;
 
 	/* rcu_read_lock()ed by nf_hook_slow() */
 	queue = instance_lookup(queuenum);
-	if (!queue)
+	if (!queue) {
+		err = -ESRCH;
 		goto err_out;
+	}
 
-	if (queue->copy_mode == NFQNL_COPY_NONE)
+	if (queue->copy_mode == NFQNL_COPY_NONE) {
+		err = -EINVAL;
 		goto err_out;
+	}
 
 	nskb = nfqnl_build_packet_message(queue, entry);
-	if (nskb == NULL)
+	if (nskb == NULL) {
+		err = -ENOMEM;
 		goto err_out;
-
+	}
 	spin_lock_bh(&queue->lock);
 
-	if (!queue->peer_pid)
+	if (!queue->peer_pid) {
+		err = -EINVAL;
 		goto err_out_free_nskb;
-
+	}
 	if (queue->queue_total >= queue->queue_maxlen) {
 		queue->queue_dropped++;
 		if (net_ratelimit())
@@ -432,7 +438,7 @@ err_out_free_nskb:
 err_out_unlock:
 	spin_unlock_bh(&queue->lock);
 err_out:
-	return -1;
+	return err;
 }
 
 static int
-- 
1.7.2.2

[PATCH 3/6] netfilter: nfnetlink_queue: do not free skb on error

[Florian Westphal]

Move free responsibility from nf_queue to caller.
This enables more flexible error handling; we can now accept the skb
instead of freeing it.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 no changes since v1, because the next_hook goto is not removed
 anymore in the previous patch.

 net/netfilter/core.c     |    7 +++++--
 net/netfilter/nf_queue.c |   17 ++++++++++-------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 91d66d2f..0c5b796 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -181,8 +181,11 @@ next_hook:
 	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
 		ret = nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
 			       verdict >> NF_VERDICT_BITS);
-		if (ret == -ECANCELED)
-			goto next_hook;
+		if (ret < 0) {
+			if (ret == -ECANCELED)
+				goto next_hook;
+			kfree_skb(skb);
+		}
 		ret = 0;
 	}
 	rcu_read_unlock();
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index c361084..2a9a6fc 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -163,9 +163,8 @@ static int __nf_queue(struct sk_buff *skb,
 
 	/* If it's going away, ignore hook. */
 	if (!try_module_get(entry->elem->owner)) {
-		rcu_read_unlock();
-		kfree(entry);
-		return -ECANCELED;
+		status = -ECANCELED;
+		goto err_unlock;
 	}
 	/* Bump dev refs so they don't vanish while packet is out */
 	if (indev)
@@ -198,7 +197,6 @@ static int __nf_queue(struct sk_buff *skb,
 err_unlock:
 	rcu_read_unlock();
 err:
-	kfree_skb(skb);
 	kfree(entry);
 	return status;
 }
@@ -229,7 +227,6 @@ int nf_queue(struct sk_buff *skb,
 	}
 
 	segs = skb_gso_segment(skb, 0);
-	kfree_skb(skb);
 	/* Does not use PTR_ERR to limit the number of error codes that can be
 	 * returned by nf_queue.  For instance, callers rely on -ECANCELED to mean
 	 * 'ignore this hook'.
@@ -253,8 +250,11 @@ int nf_queue(struct sk_buff *skb,
 		segs = nskb;
 	} while (segs);
 
+	/* also free orig skb if only some segments were queued */
 	if (unlikely(err && queued))
 		err = 0;
+	if (err == 0)
+		kfree_skb(skb);
 	return err;
 }
 
@@ -300,8 +300,11 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 		err = __nf_queue(skb, elem, entry->pf, entry->hook,
 				 entry->indev, entry->outdev, entry->okfn,
 				 verdict >> NF_VERDICT_BITS);
-		if (err == -ECANCELED)
-			goto next_hook;
+		if (err < 0) {
+			if (err == -ECANCELED)
+				goto next_hook;
+			kfree_skb(skb);
+		}
 		break;
 	case NF_STOLEN:
 	default:
-- 
1.7.2.2

[PATCH 4/6] netfilter: reduce NF_VERDICT_MASK to 0xff

[Florian Westphal]

NF_VERDICT_MASK is currently 0xffff. This is because the upper
16 bits are used to store errno (for NF_DROP) or the queue number
(NF_QUEUE verdict).

As there are up to 0xffff different queues available, there is no more
room to store additional flags.

At the moment there are only 6 different verdicts, i.e. we can reduce
NF_VERDICT_MASK to 0xff to allow storing additional flags in the 0xff00 space.

NF_VERDICT_BITS would then be reduced to 8, but because the value is
exported to userspace, this might cause breakage; e.g.:

e.g. 'queuenr = (1 << NF_VERDICT_BITS) | NF_QUEUE'  would now break.

Thus, remove NF_VERDICT_BITS usage in the kernel and move the old value
to the 'userspace compat' section.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 no changes since v1.  In particular, I've kept NF_VERDICT_BITS define
 in the userspace compat section, as Patrick did prefer to keept it.

 include/linux/netfilter.h |   20 +++++++++++++++-----
 net/netfilter/core.c      |    4 ++--
 net/netfilter/nf_queue.c  |    2 +-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 0ab7ca7..78b73cc 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -24,16 +24,19 @@
 #define NF_MAX_VERDICT NF_STOP
 
 /* we overload the higher bits for encoding auxiliary data such as the queue
- * number. Not nice, but better than additional function arguments. */
-#define NF_VERDICT_MASK 0x0000ffff
-#define NF_VERDICT_BITS 16
+ * number or errno values. Not nice, but better than additional function
+ * arguments. */
+#define NF_VERDICT_MASK 0x000000ff
+
+/* extra verdict flags have mask 0x0000ff00 */
 
+/* queue number (NF_QUEUE) or errno (NF_DROP) */
 #define NF_VERDICT_QMASK 0xffff0000
 #define NF_VERDICT_QBITS 16
 
-#define NF_QUEUE_NR(x) ((((x) << NF_VERDICT_BITS) & NF_VERDICT_QMASK) | NF_QUEUE)
+#define NF_QUEUE_NR(x) ((((x) << 16) & NF_VERDICT_QMASK) | NF_QUEUE)
 
-#define NF_DROP_ERR(x) (((-x) << NF_VERDICT_BITS) | NF_DROP)
+#define NF_DROP_ERR(x) (((-x) << 16) | NF_DROP)
 
 /* only for userspace compatibility */
 #ifndef __KERNEL__
@@ -41,6 +44,9 @@
    <= 0x2000 is used for protocol-flags. */
 #define NFC_UNKNOWN 0x4000
 #define NFC_ALTERED 0x8000
+
+/* NF_VERDICT_BITS should be 8 now, but userspace might break if this changes */
+#define NF_VERDICT_BITS 16
 #endif
 
 enum nf_inet_hooks {
@@ -72,6 +78,10 @@ union nf_inet_addr {
 
 #ifdef __KERNEL__
 #ifdef CONFIG_NETFILTER
+static inline int NF_DROP_GETERR(int verdict)
+{
+	return -(verdict >> NF_VERDICT_QBITS);
+}
 
 static inline int nf_inet_addr_cmp(const union nf_inet_addr *a1,
 				   const union nf_inet_addr *a2)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 0c5b796..4d88e45 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -175,12 +175,12 @@ next_hook:
 		ret = 1;
 	} else if ((verdict & NF_VERDICT_MASK) == NF_DROP) {
 		kfree_skb(skb);
-		ret = -(verdict >> NF_VERDICT_BITS);
+		ret = NF_DROP_GETERR(verdict);
 		if (ret == 0)
 			ret = -EPERM;
 	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
 		ret = nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
-			       verdict >> NF_VERDICT_BITS);
+			       verdict >> NF_VERDICT_QBITS);
 		if (ret < 0) {
 			if (ret == -ECANCELED)
 				goto next_hook;
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 2a9a6fc..4800649 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -299,7 +299,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 	case NF_QUEUE:
 		err = __nf_queue(skb, elem, entry->pf, entry->hook,
 				 entry->indev, entry->outdev, entry->okfn,
-				 verdict >> NF_VERDICT_BITS);
+				 verdict >> NF_VERDICT_QBITS);
 		if (err < 0) {
 			if (err == -ECANCELED)
 				goto next_hook;
-- 
1.7.2.2

[PATCH 5/6] netfilter: allow NFQUEUE bypass if no listener is available

[Florian Westphal]

If an skb is to be NF_QUEUE'd, but no program has opened the queue, the
packet is dropped.

This adds a v2 target revision of xt_NFQUEUE that allows packets to
continue through the ruleset instead.

Because the actual queueing happens outside of the target context, the
'bypass' flag has to be communicated back to the netfilter core.

Unfortunately the only choice to do this without adding a new function
argument is to use the target function return value (i.e. the verdict).

In the NF_QUEUE case, the upper 16bit already contain the queue number
to use.  The previous patch reduced NF_VERDICT_MASK to 0xff, i.e.
we now have extra room for a new flag.

If a hook issued a NF_QUEUE verdict, then the netfilter core will
continue packet processing if the queueing hook
returns -ESRCH (== "this queue does not exist") and the new
NF_VERDICT_FLAG_QUEUE_BYPASS flag is set in the verdict value.

Note: If the queue exists, but userspace does not consume packets fast
enough, the skb will still be dropped.

Signed-off-by: Florian Westphal <fwestphal@astaro.com>
---
 no changes since v1.

 include/linux/netfilter.h            |    1 +
 include/linux/netfilter/xt_NFQUEUE.h |    6 ++++++
 net/netfilter/core.c                 |    3 +++
 net/netfilter/nf_queue.c             |    7 ++++++-
 net/netfilter/xt_NFQUEUE.c           |   28 +++++++++++++++++++++++++---
 5 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 78b73cc..eeec00a 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -29,6 +29,7 @@
 #define NF_VERDICT_MASK 0x000000ff
 
 /* extra verdict flags have mask 0x0000ff00 */
+#define NF_VERDICT_FLAG_QUEUE_BYPASS	0x00008000
 
 /* queue number (NF_QUEUE) or errno (NF_DROP) */
 #define NF_VERDICT_QMASK 0xffff0000
diff --git a/include/linux/netfilter/xt_NFQUEUE.h b/include/linux/netfilter/xt_NFQUEUE.h
index 2584f4a..9eafdbb 100644
--- a/include/linux/netfilter/xt_NFQUEUE.h
+++ b/include/linux/netfilter/xt_NFQUEUE.h
@@ -20,4 +20,10 @@ struct xt_NFQ_info_v1 {
 	__u16 queues_total;
 };
 
+struct xt_NFQ_info_v2 {
+	__u16 queuenum;
+	__u16 queues_total;
+	__u16 bypass;
+};
+
 #endif /* _XT_NFQ_TARGET_H */
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 4d88e45..1e00bf7 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -184,6 +184,9 @@ next_hook:
 		if (ret < 0) {
 			if (ret == -ECANCELED)
 				goto next_hook;
+			if (ret == -ESRCH &&
+			   (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
+				goto next_hook;
 			kfree_skb(skb);
 		}
 		ret = 0;
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 4800649..45dd8bc 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -138,8 +138,10 @@ static int __nf_queue(struct sk_buff *skb,
 	rcu_read_lock();
 
 	qh = rcu_dereference(queue_handler[pf]);
-	if (!qh)
+	if (!qh) {
+		status = -ESRCH;
 		goto err_unlock;
+	}
 
 	afinfo = nf_get_afinfo(pf);
 	if (!afinfo)
@@ -303,6 +305,9 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 		if (err < 0) {
 			if (err == -ECANCELED)
 				goto next_hook;
+			if (err == -ESRCH &&
+			   (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
+				goto next_hook;
 			kfree_skb(skb);
 		}
 		break;
diff --git a/net/netfilter/xt_NFQUEUE.c b/net/netfilter/xt_NFQUEUE.c
index 3962770..d4f4b5d 100644
--- a/net/netfilter/xt_NFQUEUE.c
+++ b/net/netfilter/xt_NFQUEUE.c
@@ -83,9 +83,20 @@ nfqueue_tg_v1(struct sk_buff *skb, const struct xt_action_param *par)
 	return NF_QUEUE_NR(queue);
 }
 
-static int nfqueue_tg_v1_check(const struct xt_tgchk_param *par)
+static unsigned int
+nfqueue_tg_v2(struct sk_buff *skb, const struct xt_action_param *par)
 {
-	const struct xt_NFQ_info_v1 *info = par->targinfo;
+	const struct xt_NFQ_info_v2 *info = par->targinfo;
+	unsigned int ret = nfqueue_tg_v1(skb, par);
+
+	if (info->bypass)
+		ret |= NF_VERDICT_FLAG_QUEUE_BYPASS;
+	return ret;
+}
+
+static int nfqueue_tg_check(const struct xt_tgchk_param *par)
+{
+	const struct xt_NFQ_info_v2 *info = par->targinfo;
 	u32 maxid;
 
 	if (unlikely(!rnd_inited)) {
@@ -102,6 +113,8 @@ static int nfqueue_tg_v1_check(const struct xt_tgchk_param *par)
 		       info->queues_total, maxid);
 		return -ERANGE;
 	}
+	if (par->target->revision == 2 && info->bypass > 1)
+		return -EINVAL;
 	return 0;
 }
 
@@ -117,11 +130,20 @@ static struct xt_target nfqueue_tg_reg[] __read_mostly = {
 		.name		= "NFQUEUE",
 		.revision	= 1,
 		.family		= NFPROTO_UNSPEC,
-		.checkentry	= nfqueue_tg_v1_check,
+		.checkentry	= nfqueue_tg_check,
 		.target		= nfqueue_tg_v1,
 		.targetsize	= sizeof(struct xt_NFQ_info_v1),
 		.me		= THIS_MODULE,
 	},
+	{
+		.name		= "NFQUEUE",
+		.revision	= 2,
+		.family		= NFPROTO_UNSPEC,
+		.checkentry	= nfqueue_tg_check,
+		.target		= nfqueue_tg_v2,
+		.targetsize	= sizeof(struct xt_NFQ_info_v2),
+		.me		= THIS_MODULE,
+	},
 };
 
 static int __init nfqueue_tg_init(void)
-- 
1.7.2.2

[PATCH 6/6] netfilter: do not omit re-route check on NF_QUEUE verdict

[Florian Westphal]

ret != NF_QUEUE only works in the "--queue-num 0" case; for
queues > 0 the test should be '(ret & NF_VERDICT_MASK) != NF_QUEUE'.

However, NF_QUEUE no longer DROPs the skb unconditionally if queueing
fails (due to NF_VERDICT_FLAG_QUEUE_BYPASS verdict flag), so the
re-route test should also be performed if this flag is set in the
verdict.

The full test would then look something like

&& ((ret & NF_VERDICT_MASK) == NF_QUEUE && (ret & NF_VERDICT_FLAG_QUEUE_BYPASS))

This is rather ugly, so just remove the NF_QUEUE test altogether.

The only effect is that we might perform an unnecessary route lookup
in the NF_QUEUE case.

ip6table_mangle did not have such a check.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 no changes since v1.

diff --git a/net/ipv4/netfilter/iptable_mangle.c b/net/ipv4/netfilter/iptable_mangle.c
index 294a2a3..aef5d1f 100644
--- a/net/ipv4/netfilter/iptable_mangle.c
+++ b/net/ipv4/netfilter/iptable_mangle.c
@@ -60,7 +60,7 @@ ipt_mangle_out(struct sk_buff *skb, const struct net_device *out)
 	ret = ipt_do_table(skb, NF_INET_LOCAL_OUT, NULL, out,
 			   dev_net(out)->ipv4.iptable_mangle);
 	/* Reroute for ANY change. */
-	if (ret != NF_DROP && ret != NF_STOLEN && ret != NF_QUEUE) {
+	if (ret != NF_DROP && ret != NF_STOLEN) {
 		iph = ip_hdr(skb);
 
 		if (iph->saddr != saddr ||
-- 
1.7.2.2

Re: [PATCH 1/6] netfilter: kconfig: NFQUEUE is useless without NETFILTER_NETLINK_QUEUE

[Patrick McHardy]

On 16.01.2011 14:19, Florian Westphal wrote:
> NFLOG already does the same thing for NETFILTER_NETLINK_LOG.

Applied, thanks.

Re: [PATCH 2/6] netfilter: nfnetlink_queue: return error number to caller

[Patrick McHardy]

On 16.01.2011 14:19, Florian Westphal wrote:
> instead of returning -1 on error, return an error number to allow the
> caller to handle some errors differently.
> 
> ECANCELED is used to indicate that the hook is going away and should be
> ignored.
> 
> A followup patch will introduce more 'ignore this hook' conditions,
> (depending on queue settings) and will move kfree_skb responsibility
> to the caller.

Applied, thanks.

Re: [PATCH 3/6] netfilter: nfnetlink_queue: do not free skb on error

[Patrick McHardy]

On 16.01.2011 14:19, Florian Westphal wrote:
> Move free responsibility from nf_queue to caller.
> This enables more flexible error handling; we can now accept the skb
> instead of freeing it.
> 

Applied, thanks.

Re: [PATCH 4/6] netfilter: reduce NF_VERDICT_MASK to 0xff

[Patrick McHardy]

On 16.01.2011 14:19, Florian Westphal wrote:
> NF_VERDICT_MASK is currently 0xffff. This is because the upper
> 16 bits are used to store errno (for NF_DROP) or the queue number
> (NF_QUEUE verdict).
> 
> As there are up to 0xffff different queues available, there is no more
> room to store additional flags.
> 
> At the moment there are only 6 different verdicts, i.e. we can reduce
> NF_VERDICT_MASK to 0xff to allow storing additional flags in the 0xff00 space.
> 
> NF_VERDICT_BITS would then be reduced to 8, but because the value is
> exported to userspace, this might cause breakage; e.g.:
> 
> e.g. 'queuenr = (1 << NF_VERDICT_BITS) | NF_QUEUE'  would now break.
> 
> Thus, remove NF_VERDICT_BITS usage in the kernel and move the old value
> to the 'userspace compat' section.

Applied, thanks.

Re: [PATCH 5/6] netfilter: allow NFQUEUE bypass if no listener is available

[Patrick McHardy]

On 16.01.2011 14:19, Florian Westphal wrote:
> If an skb is to be NF_QUEUE'd, but no program has opened the queue, the
> packet is dropped.
> 
> This adds a v2 target revision of xt_NFQUEUE that allows packets to
> continue through the ruleset instead.
> 
> Because the actual queueing happens outside of the target context, the
> 'bypass' flag has to be communicated back to the netfilter core.
> 
> Unfortunately the only choice to do this without adding a new function
> argument is to use the target function return value (i.e. the verdict).
> 
> In the NF_QUEUE case, the upper 16bit already contain the queue number
> to use.  The previous patch reduced NF_VERDICT_MASK to 0xff, i.e.
> we now have extra room for a new flag.
> 
> If a hook issued a NF_QUEUE verdict, then the netfilter core will
> continue packet processing if the queueing hook
> returns -ESRCH (== "this queue does not exist") and the new
> NF_VERDICT_FLAG_QUEUE_BYPASS flag is set in the verdict value.
> 
> Note: If the queue exists, but userspace does not consume packets fast
> enough, the skb will still be dropped.

Applied, thanks.

Re: [PATCH 6/6] netfilter: do not omit re-route check on NF_QUEUE verdict

[Patrick McHardy]

On 16.01.2011 14:19, Florian Westphal wrote:
> ret != NF_QUEUE only works in the "--queue-num 0" case; for
> queues > 0 the test should be '(ret & NF_VERDICT_MASK) != NF_QUEUE'.
> 
> However, NF_QUEUE no longer DROPs the skb unconditionally if queueing
> fails (due to NF_VERDICT_FLAG_QUEUE_BYPASS verdict flag), so the
> re-route test should also be performed if this flag is set in the
> verdict.
> 
> The full test would then look something like
> 
> && ((ret & NF_VERDICT_MASK) == NF_QUEUE && (ret & NF_VERDICT_FLAG_QUEUE_BYPASS))
> 
> This is rather ugly, so just remove the NF_QUEUE test altogether.
> 
> The only effect is that we might perform an unnecessary route lookup
> in the NF_QUEUE case.

Alternatively we could have nf_queue.c perform the rerouting when
a packet is marked for queue bypass, just as it already does when
reinjecting a packet. mangle just needs to check for NF_ACCEPT,
since that is the only verdict we can return from the table that
doesn't cause the packet to be dropped or queued.

Re: [PATCH 6/6] netfilter: do not omit re-route check on NF_QUEUE verdict

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> > However, NF_QUEUE no longer DROPs the skb unconditionally if queueing
> > fails (due to NF_VERDICT_FLAG_QUEUE_BYPASS verdict flag), so the
> > re-route test should also be performed if this flag is set in the
> > verdict.
> > 
> > The full test would then look something like
> > 
> > && ((ret & NF_VERDICT_MASK) == NF_QUEUE && (ret & NF_VERDICT_FLAG_QUEUE_BYPASS))
> > 
> > This is rather ugly, so just remove the NF_QUEUE test altogether.
> > 
> > The only effect is that we might perform an unnecessary route lookup
> > in the NF_QUEUE case.
> 
> Alternatively we could have nf_queue.c perform the rerouting when
> a packet is marked for queue bypass, just as it already does when
> reinjecting a packet. mangle just needs to check for NF_ACCEPT,
> since that is the only verdict we can return from the table that
> doesn't cause the packet to be dropped or queued.

Good point, thanks!

I'll look into it.

Re: [PATCH 6/6] netfilter: do not omit re-route check on NF_QUEUE verdict

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> On 16.01.2011 14:19, Florian Westphal wrote:
> > ret != NF_QUEUE only works in the "--queue-num 0" case; for
> > queues > 0 the test should be '(ret & NF_VERDICT_MASK) != NF_QUEUE'.
> > 
> > However, NF_QUEUE no longer DROPs the skb unconditionally if queueing
> > fails (due to NF_VERDICT_FLAG_QUEUE_BYPASS verdict flag), so the
> > re-route test should also be performed if this flag is set in the
> > verdict.
> > 
> > The full test would then look something like
> > 
> > && ((ret & NF_VERDICT_MASK) == NF_QUEUE && (ret & NF_VERDICT_FLAG_QUEUE_BYPASS))
> > 
> > This is rather ugly, so just remove the NF_QUEUE test altogether.
> > 
> > The only effect is that we might perform an unnecessary route lookup
> > in the NF_QUEUE case.
> 
> Alternatively we could have nf_queue.c perform the rerouting when
> a packet is marked for queue bypass, just as it already does when
> reinjecting a packet. mangle just needs to check for NF_ACCEPT,
> since that is the only verdict we can return from the table that
> doesn't cause the packet to be dropped or queued.

Hrm.  I was looking into this, but the current logic appears to be subtly
broken, or at least it is inconsistent.

Here is what I did:

ip addr add 192.168.7.20/24 dev eth0
ip addr add 192.168.8.20/24 dev eth1
ip route add default via 192.168.7.1
ip route add default via 192.168.8.1 table 2
ip rule fwmark 2 lookup 2

iptables -t mangle -A OUTPUT -d 10.1.1.1 -j MARK --set-mark 2

When I run 'ping -c 1 10.1.1.1' i see the icmp packet on eth1.

But, when I add
iptables -t mangle -A OUTPUT -d 10.1.1.1 -j NFQUEUE

I see the packet leaving via eth0 (i am running the nfqueue test program
from libnetfilter_queue on id 0).

When I change the rule to -j NFQUEUE --queue-id 1, it leaves via eth1
%-)

Here is what I think is happening:

In the --queue-id 0 case, ipt_mangle_out() skips rerouting, because
it tests for ret != NF_QUEUE.

But nf_queue does not re-route either:
__nf_queue calls afinfo->saveroute(), and, because the test program
does not modify the nfmark any further, afinfo->reroute does not do
anything.

In the --queue-id 1 case, ipt_mangle_out() does re-routing because
the return value is (1 << 16|NF_QUEUE) and the nfmark is different
than it was before OUTPUT was called.

My think it would be best to just remove the 'ret != NF_QUEUE' test
in ipt_mangle_out so we always re-route when the nfmark was modified.

Thoughts?

Thanks, Florian

Re: [PATCH 6/6] netfilter: do not omit re-route check on NF_QUEUE verdict

[Patrick McHardy]

On 20.01.2011 00:14, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
>> On 16.01.2011 14:19, Florian Westphal wrote:
>>> ret != NF_QUEUE only works in the "--queue-num 0" case; for
>>> queues > 0 the test should be '(ret & NF_VERDICT_MASK) != NF_QUEUE'.
>>>
>>> However, NF_QUEUE no longer DROPs the skb unconditionally if queueing
>>> fails (due to NF_VERDICT_FLAG_QUEUE_BYPASS verdict flag), so the
>>> re-route test should also be performed if this flag is set in the
>>> verdict.
>>>
>>> The full test would then look something like
>>>
>>> && ((ret & NF_VERDICT_MASK) == NF_QUEUE && (ret & NF_VERDICT_FLAG_QUEUE_BYPASS))
>>>
>>> This is rather ugly, so just remove the NF_QUEUE test altogether.
>>>
>>> The only effect is that we might perform an unnecessary route lookup
>>> in the NF_QUEUE case.
>>
>> Alternatively we could have nf_queue.c perform the rerouting when
>> a packet is marked for queue bypass, just as it already does when
>> reinjecting a packet. mangle just needs to check for NF_ACCEPT,
>> since that is the only verdict we can return from the table that
>> doesn't cause the packet to be dropped or queued.
> 
> Hrm.  I was looking into this, but the current logic appears to be subtly
> broken, or at least it is inconsistent.
> 
> Here is what I did:
> 
> ip addr add 192.168.7.20/24 dev eth0
> ip addr add 192.168.8.20/24 dev eth1
> ip route add default via 192.168.7.1
> ip route add default via 192.168.8.1 table 2
> ip rule fwmark 2 lookup 2
> 
> iptables -t mangle -A OUTPUT -d 10.1.1.1 -j MARK --set-mark 2
> 
> When I run 'ping -c 1 10.1.1.1' i see the icmp packet on eth1.
> 
> But, when I add
> iptables -t mangle -A OUTPUT -d 10.1.1.1 -j NFQUEUE
> 
> I see the packet leaving via eth0 (i am running the nfqueue test program
> from libnetfilter_queue on id 0).
> 
> When I change the rule to -j NFQUEUE --queue-id 1, it leaves via eth1
> %-)
> 
> Here is what I think is happening:
> 
> In the --queue-id 0 case, ipt_mangle_out() skips rerouting, because
> it tests for ret != NF_QUEUE.
> 
> But nf_queue does not re-route either:
> __nf_queue calls afinfo->saveroute(), and, because the test program
> does not modify the nfmark any further, afinfo->reroute does not do
> anything.
> 
> In the --queue-id 1 case, ipt_mangle_out() does re-routing because
> the return value is (1 << 16|NF_QUEUE) and the nfmark is different
> than it was before OUTPUT was called.
> 
> My think it would be best to just remove the 'ret != NF_QUEUE' test
> in ipt_mangle_out so we always re-route when the nfmark was modified.
> 
> Thoughts?

Indeed, that seems inconsistent and I agree with your explanation
and suggested fix. I'll apply your patch once the current batch
has been merged, thanks.

Re: [PATCH 6/6] netfilter: do not omit re-route check on NF_QUEUE verdict

[Patrick McHardy]

Am 16.01.2011 14:19, schrieb Florian Westphal:
> ret != NF_QUEUE only works in the "--queue-num 0" case; for
> queues > 0 the test should be '(ret & NF_VERDICT_MASK) != NF_QUEUE'.
> 
> However, NF_QUEUE no longer DROPs the skb unconditionally if queueing
> fails (due to NF_VERDICT_FLAG_QUEUE_BYPASS verdict flag), so the
> re-route test should also be performed if this flag is set in the
> verdict.
> 
> The full test would then look something like
> 
> && ((ret & NF_VERDICT_MASK) == NF_QUEUE && (ret & NF_VERDICT_FLAG_QUEUE_BYPASS))
> 
> This is rather ugly, so just remove the NF_QUEUE test altogether.
> 
> The only effect is that we might perform an unnecessary route lookup
> in the NF_QUEUE case.
> 
> ip6table_mangle did not have such a check.

Applied, thanks Florian.

Re: [PATCH 2/6] netfilter: nfnetlink_queue: return error number to caller

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On 12/01/11 21:49, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >>> -	/* If it's going away, ignore hook. */
> >>> -	if (!try_module_get(entry->elem->owner)) {
> >>> -		rcu_read_unlock();
> >>> -		kfree(entry);
> >>> -		return 0;
> >>> -	}
> >>> +	if (!try_module_get(entry->elem->owner))
> >>> +		goto err_unlock;
> >>
> >> With this change, we're now releasing the skb, is this deliberate?
> > 
> > I think its not necessary to make this a special case (i.e. just
> > kfree_skb(skb) instead).
> 
> This case is rare but I'd prefer not to drop packets if the hook is
> going away.

Okay. I used -ECANCELED here, its rarely used in the kernel and
i think it comes closest.

> >> Better propagate the error of skb_gso_segment(...) with PTR_ERR()
> > 
> > The reason I did not do that is because I wanted to limit the number
> > of code one has to check for possible errno values that can be returned
> > by nf_queue.
> > 
> > Its probably reasonable to assume that skb_gso_segment() won't return
> > ESRCH (which I use to determine that qeueuing failed because the queue number did
> > not exist), so it might be safe to use PTR_ERR here.
> 
> Currently, it only return -EINVAL. Anyway, it makes sense what you
> mention. Better use -EINVAL and add a short comment upon it that tells
> why we're doing this.

I added a comment about this and plan to send v2 of the patch set on
sunday.

Thanks again for reviewing,
Florian

Re: [PATCH 2/6] netfilter: nfnetlink_queue: return error number to caller

[Pablo Neira Ayuso]

On 12/01/11 21:49, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> I've got some questions and comments on this patchset, please see below.
> 
> Thank you for spending time on reviewing this patchset!

welcome Florian ;-)

>> On 27/12/10 00:58, Florian Westphal wrote:
>>> instead of returning -1 on error, return an error number to allow the
>>> caller to handle some errors differently.
>>>
>>> To make things simpler try_module_get() failure is no longer special-cased.
>>>
>>> The new return values will be used in followup patch.
>>>
>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>> --- a/net/netfilter/core.c
>>> +++ b/net/netfilter/core.c
>>> @@ -179,9 +179,8 @@ next_hook:
>>>  		if (ret == 0)
>>>  			ret = -EPERM;
>>>  	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
>>> -		if (!nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
>>> -			      verdict >> NF_VERDICT_BITS))
>>> -			goto next_hook;
>>> +		nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
>>> +			       verdict >> NF_VERDICT_BITS);
>>
>> You have to remove the next_hook label if you want to do this.
>>
>> It's not clear to me why we need to remove the goto jump, could you
>> clarify this?
> 
> Sure. The only place where nf_queue returns 0 is this snippet in __nf_queue():

I see, only the try_module_get part.

>>> -	/* If it's going away, ignore hook. */
>>> -	if (!try_module_get(entry->elem->owner)) {
>>> -		rcu_read_unlock();
>>> -		kfree(entry);
>>> -		return 0;
>>> -	}
>>> +	if (!try_module_get(entry->elem->owner))
>>> +		goto err_unlock;
>>
>> With this change, we're now releasing the skb, is this deliberate?
> 
> I think its not necessary to make this a special case (i.e. just
> kfree_skb(skb) instead).

This case is rare but I'd prefer not to drop packets if the hook is
going away.

[...]
>> Better propagate the error of skb_gso_segment(...) with PTR_ERR()
> 
> The reason I did not do that is because I wanted to limit the number
> of code one has to check for possible errno values that can be returned
> by nf_queue.
> 
> Its probably reasonable to assume that skb_gso_segment() won't return
> ESRCH (which I use to determine that qeueuing failed because the queue number did
> not exist), so it might be safe to use PTR_ERR here.

Currently, it only return -EINVAL. Anyway, it makes sense what you
mention. Better use -EINVAL and add a short comment upon it that tells
why we're doing this.

Re: [PATCH 2/6] netfilter: nfnetlink_queue: return error number to caller

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I've got some questions and comments on this patchset, please see below.

Thank you for spending time on reviewing this patchset!

> On 27/12/10 00:58, Florian Westphal wrote:
> > instead of returning -1 on error, return an error number to allow the
> > caller to handle some errors differently.
> > 
> > To make things simpler try_module_get() failure is no longer special-cased.
> > 
> > The new return values will be used in followup patch.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > --- a/net/netfilter/core.c
> > +++ b/net/netfilter/core.c
> > @@ -179,9 +179,8 @@ next_hook:
> >  		if (ret == 0)
> >  			ret = -EPERM;
> >  	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
> > -		if (!nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> > -			      verdict >> NF_VERDICT_BITS))
> > -			goto next_hook;
> > +		nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> > +			       verdict >> NF_VERDICT_BITS);
> 
> You have to remove the next_hook label if you want to do this.
> 
> It's not clear to me why we need to remove the goto jump, could you
> clarify this?

Sure. The only place where nf_queue returns 0 is this snippet in __nf_queue():

> > -	/* If it's going away, ignore hook. */
> > -	if (!try_module_get(entry->elem->owner)) {
> > -		rcu_read_unlock();
> > -		kfree(entry);
> > -		return 0;
> > -	}
> > +	if (!try_module_get(entry->elem->owner))
> > +		goto err_unlock;
> 
> With this change, we're now releasing the skb, is this deliberate?

I think its not necessary to make this a special case (i.e. just
kfree_skb(skb) instead).

An alternative is to decide on a meaningful errno value that
could be used as return value to signal this condition.

> >  #ifdef CONFIG_BRIDGE_NETFILTER
> >  	struct net_device *physindev;
> > @@ -136,8 +136,10 @@ static int __nf_queue(struct sk_buff *skb,
> >  		goto err_unlock;
> 
> here above there's the following:
> 
>         qh = rcu_dereference(queue_handler[pf]);
>         if (!qh)
>                 goto err_unlock;
> 
>         afinfo = nf_get_afinfo(pf);
>         if (!afinfo)
>                 goto err_unlock;
> 
> With your patch, we're returning -EINVAL. I think it's better to return
> -ENOENT.

Agreed.

> > @@ -218,18 +218,26 @@ int nf_queue(struct sk_buff *skb,
> >  	segs = skb_gso_segment(skb, 0);
> >  	kfree_skb(skb);
> >  	if (IS_ERR(segs))
> > -		return 1;
> > +		return -ENOMEM;
> 
> Better propagate the error of skb_gso_segment(...) with PTR_ERR()

The reason I did not do that is because I wanted to limit the number
of code one has to check for possible errno values that can be returned
by nf_queue.

Its probably reasonable to assume that skb_gso_segment() won't return
ESRCH (which I use to determine that qeueuing failed because the queue number did
not exist), so it might be safe to use PTR_ERR here.

What do you think?

> > +	queued = 0;
> >  	do {
> >  		struct sk_buff *nskb = segs->next;
> >  
> >  		segs->next = NULL;
> > -		if (!__nf_queue(segs, elem, pf, hook, indev, outdev, okfn,
> > -				queuenum))
> > -			kfree_skb(segs);
> > +		if (error == 0) {
> > +			error = __nf_queue(segs, elem, pf, hook, indev,
> > +					   outdev, okfn, queuenum);
> > +			if (error == 0)
> > +				queued++;
> > +		}
> > +		kfree_skb(segs);
> 
> Before this patch, segs were only released if try_module_get() failed.
> Now it always release it?

Yes, my bad.
I'll add it to the list of things to fix up. Thanks for catching this.

Re: [PATCH 2/6] netfilter: nfnetlink_queue: return error number to caller

[Pablo Neira Ayuso]

Hi Florian,

I've got some questions and comments on this patchset, please see below.

On 27/12/10 00:58, Florian Westphal wrote:
> instead of returning -1 on error, return an error number to allow the
> caller to handle some errors differently.
> 
> To make things simpler try_module_get() failure is no longer special-cased.
> 
> The new return values will be used in followup patch.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/core.c            |    5 +--
>  net/netfilter/nf_queue.c        |   50 ++++++++++++++++++++++----------------
>  net/netfilter/nfnetlink_queue.c |   22 +++++++++++------
>  3 files changed, 45 insertions(+), 32 deletions(-)
> 
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index 32fcbe2..518a2e3 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -179,9 +179,8 @@ next_hook:
>  		if (ret == 0)
>  			ret = -EPERM;
>  	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
> -		if (!nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> -			      verdict >> NF_VERDICT_BITS))
> -			goto next_hook;
> +		nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> +			       verdict >> NF_VERDICT_BITS);

You have to remove the next_hook label if you want to do this.

It's not clear to me why we need to remove the goto jump, could you
clarify this?

>  	}
>  	rcu_read_unlock();
>  	return ret;
> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
> index 74aebed..a417458 100644
> --- a/net/netfilter/nf_queue.c
> +++ b/net/netfilter/nf_queue.c
> @@ -115,7 +115,7 @@ static int __nf_queue(struct sk_buff *skb,
>  		      int (*okfn)(struct sk_buff *),
>  		      unsigned int queuenum)
>  {
> -	int status;
> +	int error = -EINVAL;

Please, don't rename this variable unless that you have a good reason to
do so. Better keep the same name to reduce the number of lines that has
changed in this patch.

>  	struct nf_queue_entry *entry = NULL;
>  #ifdef CONFIG_BRIDGE_NETFILTER
>  	struct net_device *physindev;
> @@ -136,8 +136,10 @@ static int __nf_queue(struct sk_buff *skb,
>  		goto err_unlock;

here above there's the following:

        qh = rcu_dereference(queue_handler[pf]);
        if (!qh)
                goto err_unlock;

        afinfo = nf_get_afinfo(pf);
        if (!afinfo)
                goto err_unlock;

With your patch, we're returning -EINVAL. I think it's better to return
-ENOENT.

>  	entry = kmalloc(sizeof(*entry) + afinfo->route_key_size, GFP_ATOMIC);
> -	if (!entry)
> +	if (!entry) {
> +		error = -ENOMEM;
>  		goto err_unlock;
> +	}
>  
>  	*entry = (struct nf_queue_entry) {
>  		.skb	= skb,
> @@ -149,12 +151,8 @@ static int __nf_queue(struct sk_buff *skb,
>  		.okfn	= okfn,
>  	};
>  
> -	/* If it's going away, ignore hook. */
> -	if (!try_module_get(entry->elem->owner)) {
> -		rcu_read_unlock();
> -		kfree(entry);
> -		return 0;
> -	}
> +	if (!try_module_get(entry->elem->owner))
> +		goto err_unlock;

With this change, we're now releasing the skb, is this deliberate?

>  	/* Bump dev refs so they don't vanish while packet is out */
>  	if (indev)
> @@ -173,23 +171,23 @@ static int __nf_queue(struct sk_buff *skb,
>  #endif
>  	skb_dst_force(skb);
>  	afinfo->saveroute(skb, entry);
> -	status = qh->outfn(entry, queuenum);
> +	error = qh->outfn(entry, queuenum);
>  
>  	rcu_read_unlock();
>  
> -	if (status < 0) {
> +	if (error < 0) {
>  		nf_queue_entry_release_refs(entry);
>  		goto err;
>  	}
>  
> -	return 1;
> +	return 0;
>  
>  err_unlock:
>  	rcu_read_unlock();
>  err:
>  	kfree_skb(skb);
>  	kfree(entry);
> -	return 1;
> +	return error;
>  }
>  
>  int nf_queue(struct sk_buff *skb,
> @@ -201,6 +199,8 @@ int nf_queue(struct sk_buff *skb,
>  	     unsigned int queuenum)
>  {
>  	struct sk_buff *segs;
> +	int error = 0;
> +	unsigned int queued;
>  
>  	if (!skb_is_gso(skb))
>  		return __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> @@ -218,18 +218,26 @@ int nf_queue(struct sk_buff *skb,
>  	segs = skb_gso_segment(skb, 0);
>  	kfree_skb(skb);
>  	if (IS_ERR(segs))
> -		return 1;
> +		return -ENOMEM;

Better propagate the error of skb_gso_segment(...) with PTR_ERR()

> +	queued = 0;
>  	do {
>  		struct sk_buff *nskb = segs->next;
>  
>  		segs->next = NULL;
> -		if (!__nf_queue(segs, elem, pf, hook, indev, outdev, okfn,
> -				queuenum))
> -			kfree_skb(segs);
> +		if (error == 0) {
> +			error = __nf_queue(segs, elem, pf, hook, indev,
> +					   outdev, okfn, queuenum);
> +			if (error == 0)
> +				queued++;
> +		}
> +		kfree_skb(segs);

Before this patch, segs were only released if try_module_get() failed.
Now it always release it?

>  		segs = nskb;
>  	} while (segs);
> -	return 1;
> +
> +	if (unlikely(error && queued))
> +		error = 0;
> +	return error;
>  }
>  
>  void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
> @@ -237,6 +245,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
>  	struct sk_buff *skb = entry->skb;
>  	struct list_head *elem = &entry->elem->list;
>  	const struct nf_afinfo *afinfo;
> +	int err;
>  
>  	rcu_read_lock();
>  
> @@ -270,10 +279,9 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
>  		local_bh_enable();
>  		break;
>  	case NF_QUEUE:
> -		if (!__nf_queue(skb, elem, entry->pf, entry->hook,
> -				entry->indev, entry->outdev, entry->okfn,
> -				verdict >> NF_VERDICT_BITS))
> -			goto next_hook;
> +		__nf_queue(skb, elem, entry->pf, entry->hook,
> +				 entry->indev, entry->outdev, entry->okfn,
> +				 verdict >> NF_VERDICT_BITS);

Again, the next_label was not removed.

>  		break;
>  	case NF_STOLEN:
>  	default:
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 68e67d1..b83123f 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -387,25 +387,31 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
>  {
>  	struct sk_buff *nskb;
>  	struct nfqnl_instance *queue;
> -	int err;
> +	int err = -ENOBUFS;
>  
>  	/* rcu_read_lock()ed by nf_hook_slow() */
>  	queue = instance_lookup(queuenum);
> -	if (!queue)
> +	if (!queue) {
> +		err = -ESRCH;
>  		goto err_out;
> +	}
>  
> -	if (queue->copy_mode == NFQNL_COPY_NONE)
> +	if (queue->copy_mode == NFQNL_COPY_NONE) {
> +		err = -EINVAL;
>  		goto err_out;
> +	}
>  
>  	nskb = nfqnl_build_packet_message(queue, entry);
> -	if (nskb == NULL)
> +	if (nskb == NULL) {
> +		err = -ENOMEM;
>  		goto err_out;
> -
> +	}
>  	spin_lock_bh(&queue->lock);
>  
> -	if (!queue->peer_pid)
> +	if (!queue->peer_pid) {
> +		err = -EINVAL;
>  		goto err_out_free_nskb;
> -
> +	}
>  	if (queue->queue_total >= queue->queue_maxlen) {
>  		queue->queue_dropped++;
>  		if (net_ratelimit())
> @@ -432,7 +438,7 @@ err_out_free_nskb:
>  err_out_unlock:
>  	spin_unlock_bh(&queue->lock);
>  err_out:
> -	return -1;
> +	return err;
>  }
>  
>  static int

[PATCH 2/6] netfilter: nfnetlink_queue: return error number to caller

[Florian Westphal]

instead of returning -1 on error, return an error number to allow the
caller to handle some errors differently.

To make things simpler try_module_get() failure is no longer special-cased.

The new return values will be used in followup patch.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/core.c            |    5 +--
 net/netfilter/nf_queue.c        |   50 ++++++++++++++++++++++----------------
 net/netfilter/nfnetlink_queue.c |   22 +++++++++++------
 3 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 32fcbe2..518a2e3 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -179,9 +179,8 @@ next_hook:
 		if (ret == 0)
 			ret = -EPERM;
 	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
-		if (!nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
-			      verdict >> NF_VERDICT_BITS))
-			goto next_hook;
+		nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
+			       verdict >> NF_VERDICT_BITS);
 	}
 	rcu_read_unlock();
 	return ret;
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 74aebed..a417458 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -115,7 +115,7 @@ static int __nf_queue(struct sk_buff *skb,
 		      int (*okfn)(struct sk_buff *),
 		      unsigned int queuenum)
 {
-	int status;
+	int error = -EINVAL;
 	struct nf_queue_entry *entry = NULL;
 #ifdef CONFIG_BRIDGE_NETFILTER
 	struct net_device *physindev;
@@ -136,8 +136,10 @@ static int __nf_queue(struct sk_buff *skb,
 		goto err_unlock;
 
 	entry = kmalloc(sizeof(*entry) + afinfo->route_key_size, GFP_ATOMIC);
-	if (!entry)
+	if (!entry) {
+		error = -ENOMEM;
 		goto err_unlock;
+	}
 
 	*entry = (struct nf_queue_entry) {
 		.skb	= skb,
@@ -149,12 +151,8 @@ static int __nf_queue(struct sk_buff *skb,
 		.okfn	= okfn,
 	};
 
-	/* If it's going away, ignore hook. */
-	if (!try_module_get(entry->elem->owner)) {
-		rcu_read_unlock();
-		kfree(entry);
-		return 0;
-	}
+	if (!try_module_get(entry->elem->owner))
+		goto err_unlock;
 
 	/* Bump dev refs so they don't vanish while packet is out */
 	if (indev)
@@ -173,23 +171,23 @@ static int __nf_queue(struct sk_buff *skb,
 #endif
 	skb_dst_force(skb);
 	afinfo->saveroute(skb, entry);
-	status = qh->outfn(entry, queuenum);
+	error = qh->outfn(entry, queuenum);
 
 	rcu_read_unlock();
 
-	if (status < 0) {
+	if (error < 0) {
 		nf_queue_entry_release_refs(entry);
 		goto err;
 	}
 
-	return 1;
+	return 0;
 
 err_unlock:
 	rcu_read_unlock();
 err:
 	kfree_skb(skb);
 	kfree(entry);
-	return 1;
+	return error;
 }
 
 int nf_queue(struct sk_buff *skb,
@@ -201,6 +199,8 @@ int nf_queue(struct sk_buff *skb,
 	     unsigned int queuenum)
 {
 	struct sk_buff *segs;
+	int error = 0;
+	unsigned int queued;
 
 	if (!skb_is_gso(skb))
 		return __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
@@ -218,18 +218,26 @@ int nf_queue(struct sk_buff *skb,
 	segs = skb_gso_segment(skb, 0);
 	kfree_skb(skb);
 	if (IS_ERR(segs))
-		return 1;
+		return -ENOMEM;
 
+	queued = 0;
 	do {
 		struct sk_buff *nskb = segs->next;
 
 		segs->next = NULL;
-		if (!__nf_queue(segs, elem, pf, hook, indev, outdev, okfn,
-				queuenum))
-			kfree_skb(segs);
+		if (error == 0) {
+			error = __nf_queue(segs, elem, pf, hook, indev,
+					   outdev, okfn, queuenum);
+			if (error == 0)
+				queued++;
+		}
+		kfree_skb(segs);
 		segs = nskb;
 	} while (segs);
-	return 1;
+
+	if (unlikely(error && queued))
+		error = 0;
+	return error;
 }
 
 void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
@@ -237,6 +245,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 	struct sk_buff *skb = entry->skb;
 	struct list_head *elem = &entry->elem->list;
 	const struct nf_afinfo *afinfo;
+	int err;
 
 	rcu_read_lock();
 
@@ -270,10 +279,9 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 		local_bh_enable();
 		break;
 	case NF_QUEUE:
-		if (!__nf_queue(skb, elem, entry->pf, entry->hook,
-				entry->indev, entry->outdev, entry->okfn,
-				verdict >> NF_VERDICT_BITS))
-			goto next_hook;
+		__nf_queue(skb, elem, entry->pf, entry->hook,
+				 entry->indev, entry->outdev, entry->okfn,
+				 verdict >> NF_VERDICT_BITS);
 		break;
 	case NF_STOLEN:
 	default:
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 68e67d1..b83123f 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -387,25 +387,31 @@ nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
 {
 	struct sk_buff *nskb;
 	struct nfqnl_instance *queue;
-	int err;
+	int err = -ENOBUFS;
 
 	/* rcu_read_lock()ed by nf_hook_slow() */
 	queue = instance_lookup(queuenum);
-	if (!queue)
+	if (!queue) {
+		err = -ESRCH;
 		goto err_out;
+	}
 
-	if (queue->copy_mode == NFQNL_COPY_NONE)
+	if (queue->copy_mode == NFQNL_COPY_NONE) {
+		err = -EINVAL;
 		goto err_out;
+	}
 
 	nskb = nfqnl_build_packet_message(queue, entry);
-	if (nskb == NULL)
+	if (nskb == NULL) {
+		err = -ENOMEM;
 		goto err_out;
-
+	}
 	spin_lock_bh(&queue->lock);
 
-	if (!queue->peer_pid)
+	if (!queue->peer_pid) {
+		err = -EINVAL;
 		goto err_out_free_nskb;
-
+	}
 	if (queue->queue_total >= queue->queue_maxlen) {
 		queue->queue_dropped++;
 		if (net_ratelimit())
@@ -432,7 +438,7 @@ err_out_free_nskb:
 err_out_unlock:
 	spin_unlock_bh(&queue->lock);
 err_out:
-	return -1;
+	return err;
 }
 
 static int
-- 
1.7.2.2