[v3 PATCH 0/1] netfilter: "fail-open" feature support for NFQUEUE

[v3 PATCH 0/1] netfilter: "fail-open" feature support for NFQUEUE

[Krishna Kumar]

Many users of an IBM security product, which uses netfilter's NFQUEUE
target to process packets in userspace, face a problem of dropped
connections during heavy load. Incoming packets are queued and
processed by the security module, which does deep packet analysis to
decide whether to accept or reject them. However during heavy load,
NFQUEUE queue (default 1024 entries) fills up and connections fail
after large number of packets drop during enqueue.

This patch implements a "failopen" support for NFQUEUE to help keep
connections open during such failures. This is achieved by allowing
acceptance of packets temporarily when the queue is full, which enables
existing connections to be kept open.

Failopen is enabled/disabled using a new call - nfq_set_flags(qh, mask,
flags), which makes use of two new netlink attributes:
	1. NFQA_CFG_MASK:  Specifies which flags are being modified.
	2. NFQA_CFG_FLAGS: Set/reset the value for each of those flags.


		Tests done:
		------------
- netperf TCP_STREAM
- 64 netperf stress testing to ensure there are no memory leaks
- icmp (ping)
- enabling/disabling failopen in the middle of existing connections
- checksum verification of transferred files (scp)
- different flag/mask values to check that code handling NFQA_CFG_MASK
  works as expected


		Test results:
		-------------
Server:
-------
# iptables -A INPUT -p tcp -m mac --mac-source 00:00:C9:C6:4F:22 \
	-j NFQUEUE --queue-num 0
# Run interceptor program with 50ms delay between packet processing, and also
sets qlen to 16. After every read system call, this program tests and read's a
config file's contents (0 or 1) and calls nfq_set_flags(qh, mask, flags).

Client:
-------
	---> failopen is disabled on server at this time
# netperf -v0 -H 10.0.4.1
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.0.4.1 (10.0.4.1) port 0 AF_INET
0.16   
	---> failopen is enabled on server at this time
# netperf -v0 -H 10.0.4.1
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.0.4.1 (10.0.4.1) port 0 AF_INET
2292.82 

	---> failopen is disabled on server at this time
# scp FILE 10.0.4.1:/tmp
FILE                             0% 2960KB  88.4KB/s 12:19:37 ETA
	---> Enable failopen on server at this time
FILE                             21%  809MB  44.2MB/s   01:08 ETA
	---> Disable failopen on server at this time
FILE                             23%  903MB 157.4KB/s 5:18:01 ETA
	---> Enable failopen on server at this time
FILE                             100% 3835MB  24.1MB/s   02:39    


Changes from rev2:
------------------

1. Changed NFQA_CFG_FAIL_OPEN to generic NFAQ_CFG_FLAGS and NFAQ_CFG_MASK
2. Enqueue handler now returns -ENOSPC upon "fail-open".
3. Do not invoke okfn on ENOSPC, but process all hooks first. nf_hook_slow
   has code to handle failopen.

Please review.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
Signed-off-by: Vivek Kashyap <vivk@us.ibm.com>
Signed-off-by: Sridhar Samudrala <samudrala@us.ibm.com>
---

[v3 PATCH 1/1] netfilter: Add fail-open support.

[Krishna Kumar]

Implement a new "fail-open" mode where packets are not dropped
upon queue-full condition. This mode can be individually enabled
or disabled per queue using netlink NFAQ_CFG_FLAGS & NFAQ_CFG_MASK
attributes.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
Signed-off-by: Vivek Kashyap <vivk@us.ibm.com>
Signed-off-by: Sridhar Samudrala <samudrala@us.ibm.com>
---
 include/linux/netfilter/nfnetlink_queue.h |    5 ++
 net/netfilter/core.c                      |   37 +++++++++++++++++++-
 net/netfilter/nf_queue.c                  |   15 ++++++--
 net/netfilter/nfnetlink_queue.c           |   37 ++++++++++++++++++--
 4 files changed, 87 insertions(+), 7 deletions(-)

diff -ruNp org/include/linux/netfilter/nfnetlink_queue.h new/include/linux/netfilter/nfnetlink_queue.h
--- org/include/linux/netfilter/nfnetlink_queue.h	2012-05-22 08:45:32.648606721 +0530
+++ new/include/linux/netfilter/nfnetlink_queue.h	2012-05-22 16:53:05.035405303 +0530
@@ -84,8 +84,13 @@ enum nfqnl_attr_config {
 	NFQA_CFG_CMD,			/* nfqnl_msg_config_cmd */
 	NFQA_CFG_PARAMS,		/* nfqnl_msg_config_params */
 	NFQA_CFG_QUEUE_MAXLEN,		/* __u32 */
+	NFQA_CFG_MASK,			/* identify which flags to change */
+	NFQA_CFG_FLAGS,			/* value of these flags (__u32) */
 	__NFQA_CFG_MAX
 };
 #define NFQA_CFG_MAX (__NFQA_CFG_MAX-1)
 
+/* Flags for NFQA_CFG_FLAGS */
+#define NFQA_CFG_F_FAIL_OPEN			(1 << 0)
+
 #endif /* _NFNETLINK_QUEUE_H */
diff -ruNp org/net/netfilter/core.c new/net/netfilter/core.c
--- org/net/netfilter/core.c	2012-05-22 08:45:32.651608253 +0530
+++ new/net/netfilter/core.c	2012-05-22 17:35:51.294216873 +0530
@@ -163,6 +163,31 @@ repeat:
 	return NF_ACCEPT;
 }
 
+/*
+ * Handler was not able to enqueue the packet, and returned ENOSPC
+ * since "fail-open" was enabled. We temporarily accept the skb, or
+ * each segment for a segmented skb, and then free up the header.
+ */
+static void handle_fail_open(struct sk_buff *skb,
+			     int (*okfn)(struct sk_buff *))
+{
+	struct sk_buff *segs;
+	bool gso;
+
+	segs = skb->next ? : skb;
+	gso = skb->next != NULL;
+
+	do {
+		struct sk_buff *nskb = segs->next;
+
+		segs->next = NULL;
+		okfn(segs);
+		segs = nskb;
+	} while (segs);
+
+	if (gso)
+		kfree_skb(skb);
+}
 
 /* Returns 1 if okfn() needs to be executed by the caller,
  * -EPERM for NF_DROP, 0 otherwise. */
@@ -174,6 +199,7 @@ int nf_hook_slow(u_int8_t pf, unsigned i
 {
 	struct list_head *elem;
 	unsigned int verdict;
+	int failopen = 0;
 	int ret = 0;
 
 	/* We may already have this, but read-locks nest anyway */
@@ -184,7 +210,8 @@ next_hook:
 	verdict = nf_iterate(&nf_hooks[pf][hook], skb, hook, indev,
 			     outdev, &elem, okfn, hook_thresh);
 	if (verdict == NF_ACCEPT || verdict == NF_STOP) {
-		ret = 1;
+		if (!failopen) /* don't use the default verdict if 'failopen' */
+			ret = 1;
 	} else if ((verdict & NF_VERDICT_MASK) == NF_DROP) {
 		kfree_skb(skb);
 		ret = NF_DROP_GETERR(verdict);
@@ -199,10 +226,18 @@ next_hook:
 			if (err == -ESRCH &&
 			   (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
 				goto next_hook;
+			if (err == -ENOSPC) {
+				failopen = 1;
+				goto next_hook;
+			}
 			kfree_skb(skb);
 		}
 	}
 	rcu_read_unlock();
+
+	if (!ret && failopen)
+		handle_fail_open(skb, okfn);
+
 	return ret;
 }
 EXPORT_SYMBOL(nf_hook_slow);
diff -ruNp org/net/netfilter/nfnetlink_queue.c new/net/netfilter/nfnetlink_queue.c
--- org/net/netfilter/nfnetlink_queue.c	2012-05-22 08:45:32.652606825 +0530
+++ new/net/netfilter/nfnetlink_queue.c	2012-05-22 16:51:21.876842922 +0530
@@ -52,6 +52,7 @@ struct nfqnl_instance {
 
 	u_int16_t queue_num;			/* number of this queue */
 	u_int8_t copy_mode;
+	u_int32_t flags;			/* Set using NFQA_CFG_FLAGS */
 /*
  * Following fields are dirtied for each queued packet,
  * keep them in same cache line if possible.
@@ -431,9 +432,14 @@ nfqnl_enqueue_packet(struct nf_queue_ent
 		goto err_out_free_nskb;
 	}
 	if (queue->queue_total >= queue->queue_maxlen) {
-		queue->queue_dropped++;
-		net_warn_ratelimited("nf_queue: full at %d entries, dropping packets(s)\n",
-				     queue->queue_total);
+		if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
+			/* Accept the packet temporarily skipping rules */
+			err = -ENOSPC;
+		} else {
+			queue->queue_dropped++;
+			net_warn_ratelimited("nf_queue: full at %d entries, dropping packets(s)\n",
+					     queue->queue_total);
+		}
 		goto err_out_free_nskb;
 	}
 	entry->id = ++queue->id_sequence;
@@ -858,6 +864,31 @@ nfqnl_recv_config(struct sock *ctnl, str
 		spin_unlock_bh(&queue->lock);
 	}
 
+	if (nfqa[NFQA_CFG_FLAGS]) {
+		u_int32_t *flags, *mask;
+
+		if (!queue) {
+			ret = -ENODEV;
+			goto err_out_unlock;
+		}
+
+		if (!nfqa[NFQA_CFG_MASK]) {
+			/* A mask is needed to tell which flags are being
+			 * changed.
+			 * */
+			ret = -EINVAL;
+			goto err_out_unlock;
+		}
+
+		flags = nla_data(nfqa[NFQA_CFG_FLAGS]);
+		mask = nla_data(nfqa[NFQA_CFG_MASK]);
+
+		spin_lock_bh(&queue->lock);
+		queue->flags &= ~ntohl(*mask);
+		queue->flags |= ntohl(*flags) & ntohl(*mask);
+		spin_unlock_bh(&queue->lock);
+	}
+
 err_out_unlock:
 	rcu_read_unlock();
 	return ret;
diff -ruNp org/net/netfilter/nf_queue.c new/net/netfilter/nf_queue.c
--- org/net/netfilter/nf_queue.c	2012-05-22 08:45:32.649606572 +0530
+++ new/net/netfilter/nf_queue.c	2012-05-22 14:21:19.578299181 +0530
@@ -268,14 +268,23 @@ int nf_queue(struct sk_buff *skb,
 			err = __nf_queue(segs, elem, pf, hook, indev,
 					   outdev, okfn, queuenum);
 		}
-		if (err == 0)
+
+		if (err == 0) {
 			queued++;
-		else
+		} else if (err == -ENOSPC) {
+			/* Queue failed due to queue-full and handler is
+			 * in "fail-open" mode.
+			 */
+			segs->next = nskb;
+			skb->next = segs;
+			break;
+		} else {
 			kfree_skb(segs);
+		}
 		segs = nskb;
 	} while (segs);
 
-	if (queued) {
+	if (queued && err != -ENOSPC) {
 		kfree_skb(skb);
 		return 0;
 	}

Re: [v3 PATCH 1/1] netfilter: Add fail-open support.

[Florian Westphal]

Krishna Kumar <krkumar2@in.ibm.com> wrote:
> Implement a new "fail-open" mode where packets are not dropped
> upon queue-full condition. This mode can be individually enabled
> or disabled per queue using netlink NFAQ_CFG_FLAGS & NFAQ_CFG_MASK
> attributes.
> 
>  	NFQA_CFG_QUEUE_MAXLEN,		/* __u32 */
> +	NFQA_CFG_MASK,			/* identify which flags to change */
> +	NFQA_CFG_FLAGS,			/* value of these flags (__u32) */

__be32?
I see that QUEUE_MAXLEN gets ntohl treatment, too....

>  	__NFQA_CFG_MAX
>  };
>  #define NFQA_CFG_MAX (__NFQA_CFG_MAX-1)
>  
> +/* Flags for NFQA_CFG_FLAGS */
> +#define NFQA_CFG_F_FAIL_OPEN			(1 << 0)
> +
>  #endif /* _NFNETLINK_QUEUE_H */
> diff -ruNp org/net/netfilter/core.c new/net/netfilter/core.c
> --- org/net/netfilter/core.c	2012-05-22 08:45:32.651608253 +0530
> +++ new/net/netfilter/core.c	2012-05-22 17:35:51.294216873 +0530
> @@ -163,6 +163,31 @@ repeat:
>  	return NF_ACCEPT;
>  }
>  
> +/*
> + * Handler was not able to enqueue the packet, and returned ENOSPC
> + * since "fail-open" was enabled. We temporarily accept the skb, or
> + * each segment for a segmented skb, and then free up the header.
> + */
> +static void handle_fail_open(struct sk_buff *skb,
> +			     int (*okfn)(struct sk_buff *))
> +{
> +	struct sk_buff *segs;
> +	bool gso;
> +
> +	segs = skb->next ? : skb;
> +	gso = skb->next != NULL;
> +
> +	do {
> +		struct sk_buff *nskb = segs->next;
> +
> +		segs->next = NULL;
> +		okfn(segs);
> +		segs = nskb;
> +	} while (segs);
> +
> +	if (gso)
> +		kfree_skb(skb);
> +}

I don't understand why this is needed at all.
Conceptually, what you're doing is identical to the
--nfqueue-bypass feature, so it should be enough to change

> @@ -199,10 +226,18 @@ next_hook:
>  			if (err == -ESRCH &&
>  			   (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
>  				goto next_hook;
> +			if (err == -ENOSPC) {
> +				failopen = 1;
> +				goto next_hook;
> +			}
>  			kfree_skb(skb);

to

 if (err == -ENOSPC || (err == -ESRCH && (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
  	goto next_hook;

[ or, take advantage of the existing -ECANCELED and have the queueing
  backend return that if queue is full and fail-open is enabled ]

Yes, that means that if the userspace ruleset is
-j NFQUEUE
-j DROP

then your packets will be dropped even if the userspace application
enables failopen.

But thats a feature, since you could also do
-j NFQUEUE
-m limt ... -j LOG --log-prefix "queue overflow"

or play extra games wrt. "drop if established, CONNMARK
for future bypass if --ctstate NEW", etc.

If its a requirment for you that userspace can force ACCEPT
regardless of ruleset, then perhaps it might be better to
add a separate "default verdict" option and invoke
nf_reinject() directly from the qeueueing backend instead
of passing the fail-open information back to nf_hook_slow?

> +		flags = nla_data(nfqa[NFQA_CFG_FLAGS]);
> +		mask = nla_data(nfqa[NFQA_CFG_MASK]);

nla_get_be32()?
[ _u32 would make more sense to me, but other attributes are be32 too,
  so I'm ok with it ]

> -		if (err == 0)
> +
> +		if (err == 0) {
>  			queued++;
> -		else
> +		} else if (err == -ENOSPC) {
> +			/* Queue failed due to queue-full and handler is
> +			 * in "fail-open" mode.
> +			 */
> +			segs->next = nskb;
> +			skb->next = segs;
> +			break;
> +		} else {
>  			kfree_skb(segs);
> +		}
>  		segs = nskb;
>  	} while (segs);
>  
> -	if (queued) {
> +	if (queued && err != -ENOSPC) {
>  		kfree_skb(skb);
>  		return 0;
>  	}

Similarily, this shouldn't be needed either any more since you
no longer need to check for -ENOSPC (existing --queue-bypass behaviour
should handle your case, too).

Re: [v3 PATCH 1/1] netfilter: Add fail-open support.

[Krishna Kumar2]

Florian Westphal <fw@strlen.de> wrote on 05/22/2012 08:08:58 PM:

Thanks for your review.

> >
> >     NFQA_CFG_QUEUE_MAXLEN,      /* __u32 */
> > +   NFQA_CFG_MASK,         /* identify which flags to change */
> > +   NFQA_CFG_FLAGS,         /* value of these flags (__u32) */
>
> __be32?

Yes, I will change this.

> I see that QUEUE_MAXLEN gets ntohl treatment, too....
>
> > +static void handle_fail_open(struct sk_buff *skb,
> > +              int (*okfn)(struct sk_buff *))
> > +{
> > +   struct sk_buff *segs;
> > +   bool gso;
> > +
> > +   segs = skb->next ? : skb;
> > +   gso = skb->next != NULL;
> > +
> > +   do {
> > +      struct sk_buff *nskb = segs->next;
> > +
> > +      segs->next = NULL;
> > +      okfn(segs);
> > +      segs = nskb;
> > +   } while (segs);
> > +
> > +   if (gso)
> > +      kfree_skb(skb);
> > +}
>
> I don't understand why this is needed at all.
> Conceptually, what you're doing is identical to the
> --nfqueue-bypass feature, so it should be enough to change

Yes, I could use the same code. However I am not clear about the
ESRCH path since it doesn't seem to handle GSO skb correctly? See
nf_queue below where it calls kfree_skb even for -ESRCH:

	if (err == 0) {
		nf_bridge_adjust_segmented_data(segs);
		err = __nf_queue(segs, elem, pf, hook, indev,
					outdev, okfn, queuenum);
	}
	if (err == 0)
		queued++;
	else
		kfree_skb(segs);

Maybe this needs a check for ESRCH and return back to hook_slow
to work correctly? If this is the case, I can submit a patch to
fix this, and piggy-back ESRCH for fail-open too.

> > @@ -199,10 +226,18 @@ next_hook:
> >           if (err == -ESRCH &&
> >              (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
> >              goto next_hook;
> > +         if (err == -ENOSPC) {
> > +            failopen = 1;
> > +            goto next_hook;
> > +         }
> >           kfree_skb(skb);
>
> to
>
>  if (err == -ENOSPC || (err == -ESRCH && (verdict &
> NF_VERDICT_FLAG_QUEUE_BYPASS))
>      goto next_hook;

This would work if the skb was not GSO. So maybe the above
fix for ESRCH will let me share the code for fail-open too?

> [ or, take advantage of the existing -ECANCELED and have the queueing
>   backend return that if queue is full and fail-open is enabled ]
>
> Yes, that means that if the userspace ruleset is
> -j NFQUEUE
> -j DROP
>
> then your packets will be dropped even if the userspace application
> enables failopen.
>
> But thats a feature, since you could also do
> -j NFQUEUE
> -m limt ... -j LOG --log-prefix "queue overflow"
>
> or play extra games wrt. "drop if established, CONNMARK
> for future bypass if --ctstate NEW", etc.
>
> If its a requirment for you that userspace can force ACCEPT
> regardless of ruleset, then perhaps it might be better to
> add a separate "default verdict" option and invoke
> nf_reinject() directly from the qeueueing backend instead
> of passing the fail-open information back to nf_hook_slow?
>
> > +      flags = nla_data(nfqa[NFQA_CFG_FLAGS]);
> > +      mask = nla_data(nfqa[NFQA_CFG_MASK]);
>
> nla_get_be32()?
> [ _u32 would make more sense to me, but other attributes are be32 too,
>   so I'm ok with it ]

Yes, I will change to be32.

> > -      if (err == 0)
> > +
> > +      if (err == 0) {
> >           queued++;
> > -      else
> > +      } else if (err == -ENOSPC) {
> > +         /* Queue failed due to queue-full and handler is
> > +          * in "fail-open" mode.
> > +          */
> > +         segs->next = nskb;
> > +         skb->next = segs;
> > +         break;
> > +      } else {
> >           kfree_skb(segs);
> > +      }
> >        segs = nskb;
> >     } while (segs);
> >
> > -   if (queued) {
> > +   if (queued && err != -ENOSPC) {
> >        kfree_skb(skb);
> >        return 0;
> >     }
>
> Similarily, this shouldn't be needed either any more since you
> no longer need to check for -ENOSPC (existing --queue-bypass behaviour
> should handle your case, too).

Slight change could be needed here, since queued is 0 for --queue-bypass
case (and hence return -ESRCH), while it could be >0 for fail-open,
where we still want to return error as some segments were not queued.

Thanks,
- KK

Re: [v3 PATCH 1/1] netfilter: Add fail-open support.

[Florian Westphal]

Krishna Kumar2 <krkumar2@in.ibm.com> wrote:
> Florian Westphal <fw@strlen.de> wrote on 05/22/2012 08:08:58 PM:
> > I don't understand why this is needed at all.
> > Conceptually, what you're doing is identical to the
> > --nfqueue-bypass feature, so it should be enough to change
> 
> Yes, I could use the same code. However I am not clear about the
> ESRCH path since it doesn't seem to handle GSO skb correctly? See
> nf_queue below where it calls kfree_skb even for -ESRCH:
> 
> 	if (err == 0) {
> 		nf_bridge_adjust_segmented_data(segs);
> 		err = __nf_queue(segs, elem, pf, hook, indev,
> 					outdev, okfn, queuenum);
> 	}
> 	if (err == 0)
> 		queued++;
> 	else
> 		kfree_skb(segs);
> 
> Maybe this needs a check for ESRCH and return back to hook_slow
> to work correctly? If this is the case, I can submit a patch to
> fix this, and piggy-back ESRCH for fail-open too.

The idea for queue-bypass was to free the original (gso) skb
if we were able to queue at least one packet, i.e. the original
skb only continues traversal if queue bypassing is enabled
and no single segment could be queued.

If it is a requirement for you that any remaining segments
that could not be queued continue traversal, then yes,
the existing code won't work for you.

I don't think its necessary to piggyback ESRCH too; since
userspace should not bind/unbind the queue continuously
(i.e., a -ESRCH after some segments have been queued should
 be a rare condition).

However, if the code sharing is not too much of a burden
then it would be good to do it anyway.

> > > @@ -199,10 +226,18 @@ next_hook:
> > >           if (err == -ESRCH &&
> > >              (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
> > >              goto next_hook;
> > > +         if (err == -ENOSPC) {
> > > +            failopen = 1;
> > > +            goto next_hook;
> > > +         }
> > >           kfree_skb(skb);
> >
> > to
> >
> >  if (err == -ENOSPC || (err == -ESRCH && (verdict &
> > NF_VERDICT_FLAG_QUEUE_BYPASS))
> >      goto next_hook;
> 
> This would work if the skb was not GSO. So maybe the above
> fix for ESRCH will let me share the code for fail-open too?

See above; it works for GSO too if we could not queue a single segment.
I think that iff you need to handle the "some segments queued" case
and put that in here then handling -ESRCH with it too would be good.

> > [ or, take advantage of the existing -ECANCELED and have the queueing
> >   backend return that if queue is full and fail-open is enabled ]
> >
> > Yes, that means that if the userspace ruleset is
> > -j NFQUEUE
> > -j DROP
> >
> > then your packets will be dropped even if the userspace application
> > enables failopen.
> >
> > But thats a feature, since you could also do
> > -j NFQUEUE
> > -m limt ... -j LOG --log-prefix "queue overflow"
> >
> > or play extra games wrt. "drop if established, CONNMARK
> > for future bypass if --ctstate NEW", etc.
> >
> > If its a requirment for you that userspace can force ACCEPT
> > regardless of ruleset, then perhaps it might be better to
> > add a separate "default verdict" option and invoke
> > nf_reinject() directly from the qeueueing backend instead
> > of passing the fail-open information back to nf_hook_slow?
> >
> > > +      flags = nla_data(nfqa[NFQA_CFG_FLAGS]);
> > > +      mask = nla_data(nfqa[NFQA_CFG_MASK]);
> >
> > nla_get_be32()?
> > [ _u32 would make more sense to me, but other attributes are be32 too,
> >   so I'm ok with it ]
> 
> Yes, I will change to be32.
> 
> > > -      if (err == 0)
> > > +
> > > +      if (err == 0) {
> > >           queued++;
> > > -      else
> > > +      } else if (err == -ENOSPC) {
> > > +         /* Queue failed due to queue-full and handler is
> > > +          * in "fail-open" mode.
> > > +          */
> > > +         segs->next = nskb;
> > > +         skb->next = segs;
> > > +         break;
> > > +      } else {
> > >           kfree_skb(segs);
> > > +      }
> > >        segs = nskb;
> > >     } while (segs);
> > >
> > > -   if (queued) {
> > > +   if (queued && err != -ENOSPC) {
> > >        kfree_skb(skb);
> > >        return 0;
> > >     }
> >
> > Similarily, this shouldn't be needed either any more since you
> > no longer need to check for -ENOSPC (existing --queue-bypass behaviour
> > should handle your case, too).
> 
> Slight change could be needed here, since queued is 0 for --queue-bypass
> case (and hence return -ESRCH), while it could be >0 for fail-open,
> where we still want to return error as some segments were not queued.

Right, queued will almost certainly be 0 if the userspace listener is
not there anymore.

I wonder if it would be possible to just nf_reinject() directly
from nfqnl_enqueue_packet().

[ This means we loose the ability to add rules after NFQUEUE to detect
  the overflow case, but it would avoid adding code to nf_hook_slow et
  al. ]

Lets wait what others think.

Re: [v3 PATCH 1/1] netfilter: Add fail-open support.

[Krishna Kumar2]

Florian Westphal <fw@strlen.de> wrote on 05/23/2012 01:24:56 PM:

>
> > Maybe this needs a check for ESRCH and return back to hook_slow
> > to work correctly? If this is the case, I can submit a patch to
> > fix this, and piggy-back ESRCH for fail-open too.
>
> The idea for queue-bypass was to free the original (gso) skb
> if we were able to queue at least one packet, i.e. the original
> skb only continues traversal if queue bypassing is enabled
> and no single segment could be queued.
>
> If it is a requirement for you that any remaining segments
> that could not be queued continue traversal, then yes,
> the existing code won't work for you.

Yes, all segments need to be processed for this option. I will
check if it is possible to do any code-sharing, and post the
patches tomorrow with your other feedback incorporated.

Thanks for your review,
- KK