[v2 PATCH 0/6] netfilter: "fail-open" feature support for NFQUEUE

[v2 PATCH 0/6] 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. Increasing the
queue size delays the problem and also worsens latency.

This patch set 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 alive. Customers have
expressed preference for this option as similar feature is available
on other systems.

Failopen is enabled/disabled using nfq_set_failopen() call, which
uses netlink's NFQA_CFG_FAIL_OPEN attribute (will submit the library
code to turn fail-open on/off later). During a stalled scp session,
application can call this new API with fail-open set to continue the
session, as can be seen below:

		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 dynamically
	calls nfq_set_failopen(qh, val).

		Client:
# scp LARGE_FILE 10.0.4.1:/tmp
LARGE_FILE                           2% 2832KB 183.7KB/s   09:38 ETA
LARGE_FILE                           2% 3088KB  67.3KB/s   26:14 ETA
	<Set failopen=1 on server at this time>
LARGE_FILE                           11%   12MB   1.0MB/s   01:35 ETA
LARGE_FILE                           70%   75MB   7.2MB/s   00:04 ETA
LARGE_FILE                           100%  107MB   2.5MB/s  00:42


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

[v2 PATCH 1/6] netfilter: Add new netlink NFQA_CFG_FAIL_OPEN

[Krishna Kumar]

Define a new netlink message: NFQA_CFG_FAIL_OPEN

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 |    1 +
 1 file changed, 1 insertion(+)

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-08 09:16:41.969050136 +0530
+++ new/include/linux/netfilter/nfnetlink_queue.h	2012-05-08 09:39:10.334761077 +0530
@@ -84,6 +84,7 @@ 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_FAIL_OPEN,		/* __u8 */
 	__NFQA_CFG_MAX
 };
 #define NFQA_CFG_MAX (__NFQA_CFG_MAX-1)

[v2 PATCH 2/6] netfilter: Change enqueue handlers return values

[Krishna Kumar]

Since fail-open is designed to return >0 to upper layer,
change ipqueue to return 0 as success, instead of skb->len.

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>
---
 net/ipv4/netfilter/ip_queue.c  |    2 +-
 net/ipv6/netfilter/ip6_queue.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff -ruNp org/net/ipv4/netfilter/ip_queue.c new/net/ipv4/netfilter/ip_queue.c
--- org/net/ipv4/netfilter/ip_queue.c	2012-05-08 09:15:36.435049552 +0530
+++ new/net/ipv4/netfilter/ip_queue.c	2012-05-08 09:39:53.808091036 +0530
@@ -262,7 +262,7 @@ ipq_enqueue_packet(struct nf_queue_entry
 	__ipq_enqueue_entry(entry);
 
 	spin_unlock_bh(&queue_lock);
-	return status;
+	return 0;
 
 err_out_free_nskb:
 	kfree_skb(nskb);
diff -ruNp org/net/ipv6/netfilter/ip6_queue.c new/net/ipv6/netfilter/ip6_queue.c
--- org/net/ipv6/netfilter/ip6_queue.c	2012-05-08 09:15:36.433050383 +0530
+++ new/net/ipv6/netfilter/ip6_queue.c	2012-05-08 09:39:45.828858720 +0530
@@ -262,7 +262,7 @@ ipq_enqueue_packet(struct nf_queue_entry
 	__ipq_enqueue_entry(entry);
 
 	spin_unlock_bh(&queue_lock);
-	return status;
+	return 0;
 
 err_out_free_nskb:
 	kfree_skb(nskb);

[v2 PATCH 3/6] netfilter: Add support for per-queue fail-open

[Krishna Kumar]

Add support for per-queue fail-open

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>
---
 net/netfilter/nfnetlink_queue.c |    1 +
 1 file changed, 1 insertion(+)

diff -ruNp org/net/netfilter/nfnetlink_queue.c new/net/netfilter/nfnetlink_queue.c
--- org/net/netfilter/nfnetlink_queue.c	2012-05-08 09:15:36.442049628 +0530
+++ new/net/netfilter/nfnetlink_queue.c	2012-05-08 09:56:41.274129649 +0530
@@ -52,6 +52,7 @@ struct nfqnl_instance {
 
 	u_int16_t queue_num;			/* number of this queue */
 	u_int8_t copy_mode;
+	u_int8_t fail_open;			/* whether fail-open enabled */
 /*
  * Following fields are dirtied for each queued packet,
  * keep them in same cache line if possible.

[v2 PATCH 4/6] netfilter: Add fail-open support to handler

[Krishna Kumar]

Change NFQUEUE handler to return >0 value on queue full
to signify "fail-open".

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>
---
 net/netfilter/nfnetlink_queue.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff -ruNp org/net/netfilter/nfnetlink_queue.c new/net/netfilter/nfnetlink_queue.c
--- org/net/netfilter/nfnetlink_queue.c	2012-05-08 12:57:39.225755227 +0530
+++ new/net/netfilter/nfnetlink_queue.c	2012-05-08 12:57:55.515816567 +0530
@@ -433,11 +433,16 @@ nfqnl_enqueue_packet(struct nf_queue_ent
 		goto err_out_free_nskb;
 	}
 	if (queue->queue_total >= queue->queue_maxlen) {
-		queue->queue_dropped++;
-		if (net_ratelimit())
-			  printk(KERN_WARNING "nf_queue: full at %d entries, "
-				 "dropping packets(s).\n",
-				 queue->queue_total);
+		if (queue->fail_open) {
+			/* Accept the packet temporarily skipping rules */
+			err = 1;
+		} else {
+			queue->queue_dropped++;
+			if (net_ratelimit())
+				  printk(KERN_WARNING "nf_queue: full at %d "
+					 "entries, dropping packets(s).\n",
+					 queue->queue_total);
+		}
 		goto err_out_free_nskb;
 	}
 	entry->id = ++queue->id_sequence;

[v2 PATCH 5/6] netfilter: GSO packet handling

[Krishna Kumar]

Handle >0 return value from outfn in __nf_queue(). This value
is not passed up the stack but intercepted by nf_queue(), which
returns 0 to upper layers.

Also add support for GSO skb. If __nf_queue() returns >0 to
indicate fail-open, we call okfn() immediately.

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>
---
 net/netfilter/nf_queue.c |   33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff -ruNp org/net/netfilter/nf_queue.c new/net/netfilter/nf_queue.c
--- org/net/netfilter/nf_queue.c	2012-05-08 13:02:18.163816400 +0530
+++ new/net/netfilter/nf_queue.c	2012-05-08 15:08:11.028555335 +0530
@@ -189,7 +189,7 @@ static int __nf_queue(struct sk_buff *sk
 
 	rcu_read_unlock();
 
-	if (status < 0) {
+	if (status) {
 		nf_queue_entry_release_refs(entry);
 		goto err;
 	}
@@ -236,9 +236,18 @@ int nf_queue(struct sk_buff *skb,
 	int err = -EINVAL;
 	unsigned int queued;
 
-	if (!skb_is_gso(skb))
-		return __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
+	if (!skb_is_gso(skb)) {
+		err = __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
 				  queuenum);
+		if (err > 0) {
+			/* Queue failed due to queue-full and handler
+			 * returned >0 indicating fail-open - temporarily
+			 * accept packets.
+			 */
+			err = okfn(skb);
+		}
+		return err;
+	}
 
 	switch (pf) {
 	case NFPROTO_IPV4:
@@ -268,14 +277,26 @@ 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 > 0) {
+			/* Queue failed due to queue-full and handler
+			 * returned >0 indicating fail-open - accept
+			 * this and remaining segments.
+			 */
+			okfn(segs);
+		} else {
+			/* Queue failed due to queue-full and handler
+			 * returned <0 - free this and remaining skb
+			 * segments.
+			 */
 			kfree_skb(segs);
+		}
 		segs = nskb;
 	} while (segs);
 
-	if (queued) {
+	if (queued || err > 0) {
 		kfree_skb(skb);
 		return 0;
 	}

[v2 PATCH 6/6] netfilter: Enable fail-open support.

[Krishna Kumar]

Process NFQA_CFG_FAIL_OPEN and enable fail-open support.

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>
---
 net/netfilter/nfnetlink_queue.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff -ruNp org/net/netfilter/nfnetlink_queue.c new/net/netfilter/nfnetlink_queue.c
--- org/net/netfilter/nfnetlink_queue.c	2012-05-08 12:58:49.844754291 +0530
+++ new/net/netfilter/nfnetlink_queue.c	2012-05-08 12:59:03.280923680 +0530
@@ -867,6 +867,19 @@ nfqnl_recv_config(struct sock *ctnl, str
 		spin_unlock_bh(&queue->lock);
 	}
 
+	if (nfqa[NFQA_CFG_FAIL_OPEN]) {
+		u_int8_t *fail_open;
+
+		if (!queue) {
+			ret = -ENODEV;
+			goto err_out_unlock;
+		}
+		fail_open = nla_data(nfqa[NFQA_CFG_FAIL_OPEN]);
+		spin_lock_bh(&queue->lock);
+		queue->fail_open = *fail_open;
+		spin_unlock_bh(&queue->lock);
+	}
+
 err_out_unlock:
 	rcu_read_unlock();
 	return ret;

Re: [v2 PATCH 1/6] netfilter: Add new netlink NFQA_CFG_FAIL_OPEN

[Pablo Neira Ayuso]

On Tue, May 08, 2012 at 03:13:54PM +0530, Krishna Kumar wrote:
> Define a new netlink message: NFQA_CFG_FAIL_OPEN
> 
> 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 |    1 +
>  1 file changed, 1 insertion(+)
> 
> 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-08 09:16:41.969050136 +0530
> +++ new/include/linux/netfilter/nfnetlink_queue.h	2012-05-08 09:39:10.334761077 +0530
> @@ -84,6 +84,7 @@ 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_FAIL_OPEN,		/* __u8 */
>  	__NFQA_CFG_MAX
>  };
>  #define NFQA_CFG_MAX (__NFQA_CFG_MAX-1)

The patch logic that you use is not correct. One new feature, one
patch, please.

In case it gets big, then you can split changes following this logic:
patches that prepare the new feature first, then the patches that
contain the feature.

In this case, you have to put everything into one single patch.

Please, send one single patch for this new feature.

Moreover, rename NFQA_CFG_FAIL_OPEN to NFAQ_CFG_FLAGS and declare:

#define NFAQ_CFG_F_FAIL_OPEN  (1 << 0)

Then, check for that flag in the code to enable the fail open
behaviour.

I have another feature here that will use those flags for another
option.

Re: [v2 PATCH 4/6] netfilter: Add fail-open support to handler

[Pablo Neira Ayuso]

On Tue, May 08, 2012 at 03:14:30PM +0530, Krishna Kumar wrote:
> Change NFQUEUE handler to return >0 value on queue full
> to signify "fail-open".
> 
> 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>
> ---
>  net/netfilter/nfnetlink_queue.c |   15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff -ruNp org/net/netfilter/nfnetlink_queue.c new/net/netfilter/nfnetlink_queue.c
> --- org/net/netfilter/nfnetlink_queue.c	2012-05-08 12:57:39.225755227 +0530
> +++ new/net/netfilter/nfnetlink_queue.c	2012-05-08 12:57:55.515816567 +0530
> @@ -433,11 +433,16 @@ nfqnl_enqueue_packet(struct nf_queue_ent
>  		goto err_out_free_nskb;
>  	}
>  	if (queue->queue_total >= queue->queue_maxlen) {
> -		queue->queue_dropped++;
> -		if (net_ratelimit())
> -			  printk(KERN_WARNING "nf_queue: full at %d entries, "
> -				 "dropping packets(s).\n",
> -				 queue->queue_total);
> +		if (queue->fail_open) {
> +			/* Accept the packet temporarily skipping rules */
> +			err = 1;

Please, return -ENOSPC and handle this special case in nf_queue.

> +		} else {
> +			queue->queue_dropped++;
> +			if (net_ratelimit())
> +				  printk(KERN_WARNING "nf_queue: full at %d "
> +					 "entries, dropping packets(s).\n",
> +					 queue->queue_total);
> +		}
>  		goto err_out_free_nskb;
>  	}
>  	entry->id = ++queue->id_sequence;
> 
> --
> 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: [v2 PATCH 5/6] netfilter: GSO packet handling

[Pablo Neira Ayuso]

On Tue, May 08, 2012 at 03:14:42PM +0530, Krishna Kumar wrote:
> Handle >0 return value from outfn in __nf_queue(). This value
> is not passed up the stack but intercepted by nf_queue(), which
> returns 0 to upper layers.
> 
> Also add support for GSO skb. If __nf_queue() returns >0 to
> indicate fail-open, we call okfn() immediately.
> 
> 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>
> ---
>  net/netfilter/nf_queue.c |   33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff -ruNp org/net/netfilter/nf_queue.c new/net/netfilter/nf_queue.c
> --- org/net/netfilter/nf_queue.c	2012-05-08 13:02:18.163816400 +0530
> +++ new/net/netfilter/nf_queue.c	2012-05-08 15:08:11.028555335 +0530
> @@ -189,7 +189,7 @@ static int __nf_queue(struct sk_buff *sk
>  
>  	rcu_read_unlock();
>  
> -	if (status < 0) {
> +	if (status) {
>  		nf_queue_entry_release_refs(entry);
>  		goto err;
>  	}
> @@ -236,9 +236,18 @@ int nf_queue(struct sk_buff *skb,
>  	int err = -EINVAL;
>  	unsigned int queued;
>  
> -	if (!skb_is_gso(skb))
> -		return __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> +	if (!skb_is_gso(skb)) {
> +		err = __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
>  				  queuenum);
> +		if (err > 0) {
> +			/* Queue failed due to queue-full and handler
> +			 * returned >0 indicating fail-open - temporarily
> +			 * accept packets.
> +			 */
> +			err = okfn(skb);

You cannot invoke okfn here. If you do so, you'll skip the remaining
hooks.

Return the error and handle the -ENOSPC in nf_hook_slow.

(Note that the -ENOSPC is the error value I'm proposing you to use for
this special case).

> +		}
> +		return err;
> +	}
>  
>  	switch (pf) {
>  	case NFPROTO_IPV4:
> @@ -268,14 +277,26 @@ 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 > 0) {
> +			/* Queue failed due to queue-full and handler
> +			 * returned >0 indicating fail-open - accept
> +			 * this and remaining segments.
> +			 */
> +			okfn(segs);
> +		} else {
> +			/* Queue failed due to queue-full and handler
> +			 * returned <0 - free this and remaining skb
> +			 * segments.
> +			 */
>  			kfree_skb(segs);
> +		}
>  		segs = nskb;
>  	} while (segs);
>  
> -	if (queued) {
> +	if (queued || err > 0) {
>  		kfree_skb(skb);
>  		return 0;
>  	}
> 
> --
> 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: [v2 PATCH 5/6] netfilter: GSO packet handling

[Krishna Kumar2]

Pablo Neira Ayuso <pablo@netfilter.org> wrote on 05/08/2012 05:58:28 PM:


> > +   if (!skb_is_gso(skb)) {
> > +      err = __nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> >                queuenum);
> > +      if (err > 0) {
> > +         /* Queue failed due to queue-full and handler
> > +          * returned >0 indicating fail-open - temporarily
> > +          * accept packets.
> > +          */
> > +         err = okfn(skb);
>
> You cannot invoke okfn here. If you do so, you'll skip the remaining
> hooks.
>
> Return the error and handle the -ENOSPC in nf_hook_slow.
>
> (Note that the -ENOSPC is the error value I'm proposing you to use for
> this special case).

Are you suggesting to change nf_hook_slow to:

	if (err == -ENOSPC || err == -ECANCELED)
		goto next_hook;

In that case, how should the GSO case be handled (code below)?

Thanks for your feedback,

- KK

> > @@ -268,14 +277,26 @@ 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 > 0) {
> > +         /* Queue failed due to queue-full and handler
> > +          * returned >0 indicating fail-open - accept
> > +          * this and remaining segments.
> > +          */
> > +         okfn(segs);
> > +      } else {
> > +         /* Queue failed due to queue-full and handler
> > +          * returned <0 - free this and remaining skb
> > +          * segments.
> > +          */
> >           kfree_skb(segs);
> > +      }
> >        segs = nskb;
> >     } while (segs);
> >
> > -   if (queued) {
> > +   if (queued || err > 0) {
> >        kfree_skb(skb);
> >        return 0;
> >     }