ebtables_nfqueue: missing structure afinfo

ebtables_nfqueue: missing structure afinfo

[Pierre Chifflier]

Hi,

I'm still working on a module to add the NFQUEUE target to ebtables.

So far, I've been able to split the work on two parts: sending the
packets to nfqueue, and re-inject them after a verdict. I'll split
questions on separate emails.

Adding the NFQUEUE target is not a difficult task (making the
ebt_do_tables return NF_QUEUE_NR(queue_num)), however this won't work
since the __nf_queue() function [1] requires an afinfo structure :
132         afinfo = nf_get_afinfo(pf);
133         if (!afinfo)
134                 goto err_unlock;

Since there is no afinfo structure for AF_BRIDGE, this won't work.

I tried adding a dummy (empty) afinfo structure and registering it in
ebtables_init(), but that does not look like a clean solution ...

What would be the best way (in order to be proposed mainstream) to do
that ? The afinfo structure seems to be called to save the route (and
then restored when re-injecting), I'm not sure yet if this is necessary
for AF_BRIDGE packets.

Thanks,
Pierre

[1]
http://www.cs.fsu.edu/~baker/devices/lxr/http/source/linux/net/netfilter/nf_queue.c#L108

Re: ebtables_nfqueue: missing structure afinfo

[Patrick McHardy]

On 24.01.2011 21:55, Pierre Chifflier wrote:
> Hi,
> 
> I'm still working on a module to add the NFQUEUE target to ebtables.
> 
> So far, I've been able to split the work on two parts: sending the
> packets to nfqueue, and re-inject them after a verdict. I'll split
> questions on separate emails.
> 
> Adding the NFQUEUE target is not a difficult task (making the
> ebt_do_tables return NF_QUEUE_NR(queue_num)), however this won't work
> since the __nf_queue() function [1] requires an afinfo structure :
> 132         afinfo = nf_get_afinfo(pf);
> 133         if (!afinfo)
> 134                 goto err_unlock;
> 
> Since there is no afinfo structure for AF_BRIDGE, this won't work.
> 
> I tried adding a dummy (empty) afinfo structure and registering it in
> ebtables_init(), but that does not look like a clean solution ...
> 
> What would be the best way (in order to be proposed mainstream) to do
> that ? The afinfo structure seems to be called to save the route (and
> then restored when re-injecting), I'm not sure yet if this is necessary
> for AF_BRIDGE packets.

No, I don't think so either. I'd simply make the afinfo handling
in nf_queue.c optional, we can't get packets for non-existant
address families anyways.

Re: ebtables_nfqueue: missing structure afinfo

[Pierre Chifflier]

On 01/25/2011 11:34 AM, Patrick McHardy wrote:
> On 24.01.2011 21:55, Pierre Chifflier wrote:
>> Hi,
>>
>> I'm still working on a module to add the NFQUEUE target to ebtables.
>>
>> So far, I've been able to split the work on two parts: sending the
>> packets to nfqueue, and re-inject them after a verdict. I'll split
>> questions on separate emails.
>>
>> Adding the NFQUEUE target is not a difficult task (making the
>> ebt_do_tables return NF_QUEUE_NR(queue_num)), however this won't work
>> since the __nf_queue() function [1] requires an afinfo structure :
>> 132         afinfo = nf_get_afinfo(pf);
>> 133         if (!afinfo)
>> 134                 goto err_unlock;
>>
>> Since there is no afinfo structure for AF_BRIDGE, this won't work.
>>
>> I tried adding a dummy (empty) afinfo structure and registering it in
>> ebtables_init(), but that does not look like a clean solution ...
>>
>> What would be the best way (in order to be proposed mainstream) to do
>> that ? The afinfo structure seems to be called to save the route (and
>> then restored when re-injecting), I'm not sure yet if this is necessary
>> for AF_BRIDGE packets.
> 
> No, I don't think so either. I'd simply make the afinfo handling
> in nf_queue.c optional, we can't get packets for non-existant
> address families anyways.

Thanks Patrick,

I've spent a few days trying to make it optional (in nf_queue.c,
function __nf_queue), however I have a weird problem:
If I remove the test for afinfo (and ensure that the pointer is not used
if null), everything seems to work fine.
However, when I use a userspace program (always returning NF_ACCEPT), I
got a fatal kernel oops (or panic) regarding packets.
According to the info displayed, it seems to be sometimes in
br_handle_frame, at
this point:
	case BR_STATE_FORWARDING:
		rhook = rcu_dereference(br_should_route_hook);
		if (rhook) {
			if ((*rhook)(skb))

inside the (*rhook) call.

Sometimes it's in netlink_unicast (unknown location), always when
reinjecting packet.

I tried to get more info with kgdb, but without much success:
#2  0xc11d1adc in skb_release_all (skb=0xdf1ed600) at net/core/skbuff.c:403
#3  __kfree_skb (skb=0xdf1ed600) at net/core/skbuff.c:417
#4  0xc11d1bdb in kfree_skb (skb=0xdf1ed600) at net/core/skbuff.c:438
#5  0xc11ee4d1 in netlink_unicast (ssk=0xdbf15c00, skb=<value optimized
out>,
    pid=<value optimized out>, nonblock=0) at net/netlink/af_netlink.c:921

Any idea on what could cause this ? AFAICT, it was not happening when I
used a fake afinfo structure.

Thanks,
Pierre

Re: ebtables_nfqueue: missing structure afinfo

[Patrick McHardy]

Am 02.02.2011 20:22, schrieb Pierre Chifflier:
> I've spent a few days trying to make it optional (in nf_queue.c,
> function __nf_queue), however I have a weird problem:
> If I remove the test for afinfo (and ensure that the pointer is not used
> if null), everything seems to work fine.
> However, when I use a userspace program (always returning NF_ACCEPT), I
> got a fatal kernel oops (or panic) regarding packets.
> According to the info displayed, it seems to be sometimes in
> br_handle_frame, at
> this point:
> 	case BR_STATE_FORWARDING:
> 		rhook = rcu_dereference(br_should_route_hook);
> 		if (rhook) {
> 			if ((*rhook)(skb))
> 
> inside the (*rhook) call.

It shouldn't even get to this function since this is before the
NF_HOOK calls.

> Sometimes it's in netlink_unicast (unknown location), always when
> reinjecting packet.
> 
> I tried to get more info with kgdb, but without much success:
> #2  0xc11d1adc in skb_release_all (skb=0xdf1ed600) at net/core/skbuff.c:403
> #3  __kfree_skb (skb=0xdf1ed600) at net/core/skbuff.c:417
> #4  0xc11d1bdb in kfree_skb (skb=0xdf1ed600) at net/core/skbuff.c:438
> #5  0xc11ee4d1 in netlink_unicast (ssk=0xdbf15c00, skb=<value optimized
> out>,
>     pid=<value optimized out>, nonblock=0) at net/netlink/af_netlink.c:921
> 
> Any idea on what could cause this ? AFAICT, it was not happening when I
> used a fake afinfo structure.

No. Please send over the code you're using and I'll have a closer look.

WIP/RFC: add new module ebt_NFQUEUE for ebtables

[Pierre Chifflier]

Hi,

Here is the code as requested for the module I'm trying to add.
Basically, the code is split in two parts:
- patch 1 and 2 prepare the ebtables and nfqueue existing code to allow
  sending packets from ebtables. Since these packets are labelled PF_BRIDGE
  they have no afinfo structure, so we just ignore (instead of exiting)
- patch 3 adds the new module, which just always returns EBT_QUEUE

This is of course not meant for inclusion, but comments would be welcome.

Note: to be tested, this requires a patched version of the ebtables
userspace tool to support adding rules.

The current problems I'm facing are described in the previous mail,
and specifically a fault in skb_free, deep inside the br_handle_frame code
after a reinject.
I'll try to isolate a backtrace when it happens.

Thanks,
Pierre

[PATCH 1/3] Make the afinfo structure optional in nf_queue and nf_reinject

[Pierre Chifflier]

Packets coming from the bridge do not have an afinfo registered,
so make the call to get the structure optional.

Signed-off-by: Pierre Chifflier <chifflier@edenwall.com>
---
 net/netfilter/nf_queue.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 5ab22e2..a725487 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -131,8 +131,9 @@ static int __nf_queue(struct sk_buff *skb,
 	struct net_device *physindev;
 	struct net_device *physoutdev;
 #endif
-	const struct nf_afinfo *afinfo;
+	const struct nf_afinfo *afinfo = NULL;
 	const struct nf_queue_handler *qh;
+	int route_key_size = 0;
 
 	/* QUEUE == DROP if noone is waiting, to be safe. */
 	rcu_read_lock();
@@ -144,10 +145,10 @@ static int __nf_queue(struct sk_buff *skb,
 	}
 
 	afinfo = nf_get_afinfo(pf);
-	if (!afinfo)
-		goto err_unlock;
+	if (afinfo)
+		route_key_size = afinfo->route_key_size;
 
-	entry = kmalloc(sizeof(*entry) + afinfo->route_key_size, GFP_ATOMIC);
+	entry = kmalloc(sizeof(*entry) + route_key_size, GFP_ATOMIC);
 	if (!entry) {
 		status = -ENOMEM;
 		goto err_unlock;
@@ -184,7 +185,8 @@ static int __nf_queue(struct sk_buff *skb,
 	}
 #endif
 	skb_dst_force(skb);
-	afinfo->saveroute(skb, entry);
+	if (afinfo)
+		afinfo->saveroute(skb, entry);
 	status = qh->outfn(entry, queuenum);
 
 	rcu_read_unlock();
@@ -264,7 +266,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;
+	const struct nf_afinfo *afinfo = NULL;
 	int err;
 
 	rcu_read_lock();
@@ -279,7 +281,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 
 	if (verdict == NF_ACCEPT) {
 		afinfo = nf_get_afinfo(entry->pf);
-		if (!afinfo || afinfo->reroute(skb, entry) < 0)
+		if (afinfo && afinfo->reroute(skb, entry) < 0)
 			verdict = NF_DROP;
 	}
 
-- 
1.7.2.3

[PATCH 2/3] bridge: add support for the EBT_QUEUE target

[Pierre Chifflier]

Add the EBT_QUEUE standard target for ebtables.

Signed-off-by: Pierre Chifflier <chifflier@edenwall.com>
---
 include/linux/netfilter_bridge/ebtables.h |    3 ++-
 net/bridge/netfilter/ebtables.c           |    5 +++++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/linux/netfilter_bridge/ebtables.h b/include/linux/netfilter_bridge/ebtables.h
index 1c6f0c5..d5085eb 100644
--- a/include/linux/netfilter_bridge/ebtables.h
+++ b/include/linux/netfilter_bridge/ebtables.h
@@ -25,7 +25,8 @@
 #define EBT_DROP     -2
 #define EBT_CONTINUE -3
 #define EBT_RETURN   -4
-#define NUM_STANDARD_TARGETS   4
+#define EBT_QUEUE    -5
+#define NUM_STANDARD_TARGETS   5
 /* ebtables target modules store the verdict inside an int. We can
  * reclaim a part of this int for backwards compatible extensions.
  * The 4 lsb are more than enough to store the verdict. */
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 5f1825d..156a7d7 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -251,6 +251,11 @@ unsigned int ebt_do_table (unsigned int hook, struct sk_buff *skb,
 			read_unlock_bh(&table->lock);
 			return NF_ACCEPT;
 		}
+		if (verdict == EBT_QUEUE) {
+			u32 queue_num = 1; // XXX hardcoded
+			read_unlock_bh(&table->lock);
+			return NF_QUEUE_NR(queue_num);
+		}
 		if (verdict == EBT_DROP) {
 			read_unlock_bh(&table->lock);
 			return NF_DROP;
-- 
1.7.2.3

[PATCH 3/3] bridge: add new target NFQUEUE for ebtables

[Pierre Chifflier]

This adds support for sending bridge packets to userspace using
the NFQUEUE target with ebtables.

Signed-off-by: Pierre Chifflier <chifflier@edenwall.com>
---
 include/linux/netfilter_bridge/Kbuild        |    1 +
 include/linux/netfilter_bridge/ebt_nfqueue.h |   21 ++++++++
 net/bridge/netfilter/Kconfig                 |   12 ++++
 net/bridge/netfilter/Makefile                |    1 +
 net/bridge/netfilter/ebt_nfqueue.c           |   70 ++++++++++++++++++++++++++
 5 files changed, 105 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/netfilter_bridge/ebt_nfqueue.h
 create mode 100644 net/bridge/netfilter/ebt_nfqueue.c

diff --git a/include/linux/netfilter_bridge/Kbuild b/include/linux/netfilter_bridge/Kbuild
index e48f1a3..fa3a2e1 100644
--- a/include/linux/netfilter_bridge/Kbuild
+++ b/include/linux/netfilter_bridge/Kbuild
@@ -10,6 +10,7 @@ header-y += ebt_mark_m.h
 header-y += ebt_mark_t.h
 header-y += ebt_nat.h
 header-y += ebt_nflog.h
+header-y += ebt_nfqueue.h
 header-y += ebt_pkttype.h
 header-y += ebt_redirect.h
 header-y += ebt_stp.h
diff --git a/include/linux/netfilter_bridge/ebt_nfqueue.h b/include/linux/netfilter_bridge/ebt_nfqueue.h
new file mode 100644
index 0000000..56ff699
--- /dev/null
+++ b/include/linux/netfilter_bridge/ebt_nfqueue.h
@@ -0,0 +1,21 @@
+#ifndef __LINUX_BRIDGE_EBT_NFQUEUE_H
+#define __LINUX_BRIDGE_EBT_NFQUEUE_H
+
+#define EBT_NFQUEUE_MASK 0x0
+
+#define EBT_NFQUEUE_PREFIX_SIZE 64
+#define EBT_NFQUEUE_WATCHER "nfqueue"
+
+#define EBT_NFQUEUE_DEFAULT_GROUP		0x1
+#define EBT_NFQUEUE_DEFAULT_THRESHOLD	1
+
+struct ebt_nfqueue_info {
+	u_int32_t len;
+	u_int16_t group;
+	u_int16_t threshold;
+	u_int16_t flags;
+	u_int16_t pad;
+	char prefix[EBT_NFQUEUE_PREFIX_SIZE];
+};
+
+#endif				/* __LINUX_BRIDGE_EBT_NFQUEUE_H */
diff --git a/net/bridge/netfilter/Kconfig b/net/bridge/netfilter/Kconfig
index ba6f73e..9945b09 100644
--- a/net/bridge/netfilter/Kconfig
+++ b/net/bridge/netfilter/Kconfig
@@ -218,4 +218,16 @@ config BRIDGE_EBT_NFLOG
 
 	  To compile it as a module, choose M here.  If unsure, say N.
 
+config BRIDGE_EBT_NFQUEUE
+	tristate "ebt: NFQUEUE support"
+	select NETFILTER_NETLINK
+	help
+	  If this option is enabled, the kernel will include support
+	  for queueing packets via NFNETLINK.
+
+	  This option adds the nfqueue watcher, that you can use in any rule
+	  in any ebtables table.
+
+	  To compile it as a module, choose M here.  If unsure, say N.
+
 endif # BRIDGE_NF_EBTABLES
diff --git a/net/bridge/netfilter/Makefile b/net/bridge/netfilter/Makefile
index 0718699..b05a1a4 100644
--- a/net/bridge/netfilter/Makefile
+++ b/net/bridge/netfilter/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_BRIDGE_EBT_MARK_T) += ebt_mark.o
 obj-$(CONFIG_BRIDGE_EBT_DNAT) += ebt_dnat.o
 obj-$(CONFIG_BRIDGE_EBT_REDIRECT) += ebt_redirect.o
 obj-$(CONFIG_BRIDGE_EBT_SNAT) += ebt_snat.o
+obj-$(CONFIG_BRIDGE_EBT_NFQUEUE) += ebt_nfqueue.o
 
 # watchers
 obj-$(CONFIG_BRIDGE_EBT_LOG) += ebt_log.o
diff --git a/net/bridge/netfilter/ebt_nfqueue.c b/net/bridge/netfilter/ebt_nfqueue.c
new file mode 100644
index 0000000..505d871
--- /dev/null
+++ b/net/bridge/netfilter/ebt_nfqueue.c
@@ -0,0 +1,70 @@
+/*
+ * ebt_nfqueue
+ *
+ *	Author:
+ *	Pierre Chifflier <chifflier@edenwall.com>
+ *
+ *  January, 2010
+ *
+ * Based on:
+ *  xt_NFLOG.c, (C) 2006 by Patrick McHardy <kaber@trash.net>
+ *  ebt_ulog.c, (C) 2004 by Bart De Schuymer <bdschuym@pandora.be>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/netfilter/x_tables.h>
+#include <linux/netfilter_bridge/ebtables.h>
+#include <linux/netfilter_bridge/ebt_nflog.h>
+#include <linux/netfilter_bridge/ebt_nfqueue.h>
+#include <net/netfilter/nf_log.h>
+#include <net/netfilter/nf_queue.h>
+
+static unsigned int
+ebt_nfqueue_tg(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	const struct ebt_nfqueue_info *info = par->targinfo;
+	struct nf_loginfo li;
+	unsigned int verdict = NF_ACCEPT;
+
+	printk(KERN_NOTICE "ebt_NFQUEUE: returning EBT_QUEUE\n");
+	return EBT_QUEUE;
+}
+
+static int ebt_nfqueue_tg_check(const struct xt_tgchk_param *par)
+{
+	struct ebt_nfqueue_info *info = par->targinfo;
+
+	if (info->flags & ~EBT_NFQUEUE_MASK)
+		return -EINVAL;
+	info->prefix[EBT_NFQUEUE_PREFIX_SIZE - 1] = '\0';
+	return 0;
+}
+
+static struct xt_target ebt_nfqueue_tg_reg __read_mostly = {
+	.name       = "nfqueue",
+	.revision   = 0,
+	.family     = NFPROTO_BRIDGE,
+	.target     = ebt_nfqueue_tg,
+	.checkentry = ebt_nfqueue_tg_check,
+	.targetsize = sizeof(struct ebt_nfqueue_info),
+	.me         = THIS_MODULE,
+};
+
+static int __init ebt_nfqueue_init(void)
+{
+	printk(KERN_NOTICE "ebt_NFQUEUE: init\n");
+	return xt_register_target(&ebt_nfqueue_tg_reg);
+}
+
+static void __exit ebt_nfqueue_fini(void)
+{
+	xt_unregister_target(&ebt_nfqueue_tg_reg);
+}
+
+module_init(ebt_nfqueue_init);
+module_exit(ebt_nfqueue_fini);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Pierre Chifflier <chifflier@edenwall.com>");
+MODULE_DESCRIPTION("ebtables NFQUEUE netfilter filter module");
-- 
1.7.2.3

Re: WIP/RFC: add new module ebt_NFQUEUE for ebtables

[Bart De Schuymer]

Op 3/02/2011 15:32, Pierre Chifflier schreef:
> Hi,
>
> Here is the code as requested for the module I'm trying to add.
> Basically, the code is split in two parts:
> - patch 1 and 2 prepare the ebtables and nfqueue existing code to allow
>    sending packets from ebtables. Since these packets are labelled PF_BRIDGE
>    they have no afinfo structure, so we just ignore (instead of exiting)
> - patch 3 adds the new module, which just always returns EBT_QUEUE
>
> This is of course not meant for inclusion, but comments would be welcome.
>
I'm not too familiar with nf_queue, but I don't see you register a queue 
handler for NFPROTO_BRIDGE.
Why does __nf_queue even queue the packet to userspace if you didn't 
register a queue handler for pf=NFPROTO_BRIDGE?
For debugging, you could experiment with iptables' queue target for 
bridged IP traffic (iptables can be enabled to see bridged traffic)... 
Since the queueing is basically done in the same bridge context for this 
traffic, it should help you.

Bart

-- 
Bart De Schuymer
www.artinalgorithms.be

Re: WIP/RFC: add new module ebt_NFQUEUE for ebtables

[Pierre Chifflier]

On 02/03/2011 03:32 PM, Pierre Chifflier wrote:

> The current problems I'm facing are described in the previous mail,
> and specifically a fault in skb_free, deep inside the br_handle_frame code
> after a reinject.
> I'll try to isolate a backtrace when it happens.
> 

Hi,

The backtrace is more or less the same as the one in Jan's email
<alpine.LNX.2.01.1102031605330.21536@obet.zrqbmnf.qr>
(Kernel crash with repeated NF invocation)

At the moment, I'm wondering if I'm not having several cumulated
problems, which may be triggered more easily by my modifications ...

Pierre

Re: WIP/RFC: add new module ebt_NFQUEUE for ebtables

[Patrick McHardy]

On 04.02.2011 11:05, Pierre Chifflier wrote:
> On 02/03/2011 03:32 PM, Pierre Chifflier wrote:
> 
>> The current problems I'm facing are described in the previous mail,
>> and specifically a fault in skb_free, deep inside the br_handle_frame code
>> after a reinject.
>> I'll try to isolate a backtrace when it happens.
>>
> 
> Hi,
> 
> The backtrace is more or less the same as the one in Jan's email
> <alpine.LNX.2.01.1102031605330.21536@obet.zrqbmnf.qr>
> (Kernel crash with repeated NF invocation)
> 
> At the moment, I'm wondering if I'm not having several cumulated
> problems, which may be triggered more easily by my modifications ...

We can't really help debug this without seeing the code, so please
post your current version (either privately or to the list).

Re: WIP/RFC: add new module ebt_NFQUEUE for ebtables

[Pierre Chifflier]

On 02/04/2011 02:07 PM, Patrick McHardy wrote:
> On 04.02.2011 11:05, Pierre Chifflier wrote:
>> On 02/03/2011 03:32 PM, Pierre Chifflier wrote:
>>
>>> The current problems I'm facing are described in the previous mail,
>>> and specifically a fault in skb_free, deep inside the br_handle_frame code
>>> after a reinject.
>>> I'll try to isolate a backtrace when it happens.
>>>
>>
>> Hi,
>>
>> The backtrace is more or less the same as the one in Jan's email
>> <alpine.LNX.2.01.1102031605330.21536@obet.zrqbmnf.qr>
>> (Kernel crash with repeated NF invocation)
>>
>> At the moment, I'm wondering if I'm not having several cumulated
>> problems, which may be triggered more easily by my modifications ...
> 
> We can't really help debug this without seeing the code, so please
> post your current version (either privately or to the list).

Hi Patrick,

I just realized I didn't cc: you when sending the patches, but I send
them yesterday on the list (in reply to this thread)

<1296743540-8148-2-git-send-email-chifflier@edenwall.com>
[PATCH 1/3] Make the afinfo structure optional in nf_queue and nf_reinject

And the 2 following.
Sorry for the missing cc.

Pierre

Re: WIP/RFC: add new module ebt_NFQUEUE for ebtables

[Patrick McHardy]

On 04.02.2011 14:20, Pierre Chifflier wrote:
> On 02/04/2011 02:07 PM, Patrick McHardy wrote:
>>> At the moment, I'm wondering if I'm not having several cumulated
>>> problems, which may be triggered more easily by my modifications ...
>>
>> We can't really help debug this without seeing the code, so please
>> post your current version (either privately or to the list).
> 
> Hi Patrick,
> 
> I just realized I didn't cc: you when sending the patches, but I send
> them yesterday on the list (in reply to this thread)
> 
> <1296743540-8148-2-git-send-email-chifflier@edenwall.com>
> [PATCH 1/3] Make the afinfo structure optional in nf_queue and nf_reinject
> 
> And the 2 following.
> Sorry for the missing cc.

Thanks, I just noticed them on the list.

Re: [PATCH 3/3] bridge: add new target NFQUEUE for ebtables

[Patrick McHardy]

On 03.02.2011 15:32, Pierre Chifflier wrote:
> This adds support for sending bridge packets to userspace using
> the NFQUEUE target with ebtables.

I don't think we need a new target for this (and the EBT_QUEUE
definition), just using xt_NFQUEUE should work fine.

Re: [PATCH 1/3] Make the afinfo structure optional in nf_queue and nf_reinject

[Patrick McHardy]

On 03.02.2011 15:32, Pierre Chifflier wrote:
> Packets coming from the bridge do not have an afinfo registered,
> so make the call to get the structure optional.

This patch looks fine to me.

Regarding your crash - in what chain are you queueing the packet?
We also need the exact backtrace.

Re: [PATCH 3/3] bridge: add new target NFQUEUE for ebtables

[Pierre Chifflier]

On 02/04/2011 02:25 PM, Patrick McHardy wrote:
> On 03.02.2011 15:32, Pierre Chifflier wrote:
>> This adds support for sending bridge packets to userspace using
>> the NFQUEUE target with ebtables.
> 
> I don't think we need a new target for this (and the EBT_QUEUE
> definition), just using xt_NFQUEUE should work fine.

I thought ebtables did not support sending packet to xtables ? (that's
on the TODO list for ebtables).

Re: [PATCH 3/3] bridge: add new target NFQUEUE for ebtables

[Patrick McHardy]

On 04.02.2011 14:40, Pierre Chifflier wrote:
> On 02/04/2011 02:25 PM, Patrick McHardy wrote:
>> On 03.02.2011 15:32, Pierre Chifflier wrote:
>>> This adds support for sending bridge packets to userspace using
>>> the NFQUEUE target with ebtables.
>>
>> I don't think we need a new target for this (and the EBT_QUEUE
>> definition), just using xt_NFQUEUE should work fine.
> 
> I thought ebtables did not support sending packet to xtables ? (that's
> on the TODO list for ebtables).

It can use xtables targets and matches if I'm not completely
mistaken.

Re: [PATCH 1/3] Make the afinfo structure optional in nf_queue and nf_reinject

[Pierre Chifflier]

On 02/04/2011 02:27 PM, Patrick McHardy wrote:
> On 03.02.2011 15:32, Pierre Chifflier wrote:
>> Packets coming from the bridge do not have an afinfo registered,
>> so make the call to get the structure optional.
> 
> This patch looks fine to me.
> 
> Regarding your crash - in what chain are you queueing the packet?
> We also need the exact backtrace.

The problem is that I must be running out of luck, but since today at
the moment the bridges goes in forwarding mode, I always got an oops.

Here's all the information I have, in hope it'll be useful.

Here is the backtrace (taken from kgdb):
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -2]
0x14488b55 in ?? ()
(gdb) bt
#0  0x14488b55 in ?? ()
#1  0xc128e506 in br_handle_frame (skb=0xd79aad80) at
net/bridge/br_input.c:181
#2  0xc11e3b7c in __netif_receive_skb (skb=0xd79aad80) at
net/core/dev.c:3085
#3  0xc11e754d in netif_receive_skb (skb=0xd79aad80) at net/core/dev.c:3179
#4  0xe0cbdf4d in ?? ()
#5  0xc11e7adf in net_rx_action (h=<value optimized out>)
    at net/core/dev.c:3727
#6  0xc1034319 in __do_softirq () at kernel/softirq.c:238
#7  0xc1004056 in call_on_stack () at arch/x86/kernel/irq_32.c:66
#8  do_softirq () at arch/x86/kernel/irq_32.c:176
#9  0xc10341f4 in irq_exit () at kernel/softirq.c:328
#10 0xc1003893 in do_IRQ (regs=<value optimized out>)
    at arch/x86/kernel/irq.c:247
#11 0xc1002e30 in ?? () at arch/x86/kernel/entry_32.S:825
#12 0xc101b6cf in native_safe_halt ()
    at /home/pollux/build/nf-next-2.6/arch/x86/include/asm/irqflags.h:49
#13 0xc1007ec0 in arch_safe_halt ()
    at /home/pollux/build/nf-next-2.6/arch/x86/include/asm/paravirt.h:110
#14 default_idle () at arch/x86/kernel/process.c:385
#15 0xc1001aa0 in cpu_idle () at arch/x86/kernel/process_32.c:114
#16 0xc12a4f94 in rest_init () at init/main.c:454
#17 0xc14058d3 in start_kernel () at init/main.c:704
#18 0xc14050d2 in i386_start_kernel () at arch/x86/kernel/head32.c:78
---Type <return> to continue, or q <return> to quit---
#19 0x00000000 in ?? ()

On the screen, I have only a very few information, only that bridge is
entering forwarding state, and that last sysfs file is
/sys/devices/virtual/net/lo/operstate


The commands I used to setup the bridge are:
brctl addbr br0
brctl stp br0 off
brctl addif br0 eth0
brctl addif br0 eth1

ifconfig eth0 up
ifconfig eth1 up
ifconfig br0 up


At this point, I got the problem reported in the backtrace.

If that may be important, my test setup is using 2 VMs (using KVM)
[test client (debian squeeze)] -> [ebtables (nf-next-2.6)] -> real network
I start the 2 VMs and setup the bridge, then boom.



The following lines are not related to the backtrace, since it generally
happens *before* I can even add an ebtables rule.

To add a test rule, I just use
ebtables -A FORWARD -j nfqueue

Running a Python program to listen on the queue. I can provide it, the
most important lines are

q.set_callback(cb)
q.fast_open(1, AF_BRIDGE)

The callback:
def cb(i,payload):
	payload.set_verdict(nfqueue.NF_ACCEPT)

Thanks,
Pierre

Re: [RFH] bridge: add new target NFQUEUE for ebtables

[Pierre Chifflier]

On 02/15/2011 12:52 AM, Patrick McHardy wrote:

>> That looks interesting, however I am not sure of what you meant:
>> - using the ebtables tool to add a rule with a xtables target ? (that
>> does not seem to work)
> 
> It depends on the registration, if you either register for
> NFPROTO_BRIDGE or NFPROTO_UNSPEC, this should work. ARP (the
> only additional protocol besides INET/INET6/BRIDGE) should
> work just fine with userspace queueing with your changes to
> not require an afinfo in nf_queue. So using AF_UNSPEC seems
> like the proper choice.

[CC-ing -devel]

Hi,

Thanks for your reply Patrick.
So I did the following:
- rebased on today's nf-next-2.6
- apply only the first patch (which makes afinfo optional)
- revert all other patches
- apply the recent fix on nf_iterate since it was the cause of my oops

I patched ebtables to use xt_NFQUEUE (using a struct xt_NFQ_info_v1 with
arguments queuenum 1 and queues_total 1), and removed any other change.

When I add a rule with the NFQUEUE target with ebtables, I almost
immediately get a panic (full backtrace later in this mail).

What is weird is that I got a NULL skb  in ebt_in_hook (frame 2) while
the skb was not NULL earlier - like if it was stolen by some hook. Any
idea on what could cause that ?

Thanks for your help.

Pierre


(gdb) bt
#0  0xc1292de3 in ebt_do_table (hook=<value optimized out>,
    skb=<value optimized out>, in=<value optimized out>, out=0xdcbfd000,
    table=0xdcbd8200) at net/bridge/netfilter/ebtables.c:287
#1  0xc1293753 in ebt_in_hook (hook=65539, skb=0x0, in=0x1, out=0xdcbfd000,
    okfn=0xc128cc05 <br_forward_finish>)
    at net/bridge/netfilter/ebtable_filter.c:66
#2  0xc11fc573 in nf_iterate (head=<value optimized out>,
    skb=<value optimized out>, hook=2, indev=0xd7530000, outdev=0xdcbfd000,
    i=0xdf071e58, okfn=0xc128cc05 <br_forward_finish>,
hook_thresh=-2147483648)
    at net/netfilter/core.c:137
#3  0xc11fc5fb in nf_hook_slow (pf=<value optimized out>, hook=2,
    skb=<value optimized out>, indev=0xd7530000, outdev=0xdcbfd000,
    okfn=0xc128cc05 <br_forward_finish>, hook_thresh=-2147483648)
    at net/netfilter/core.c:173
#4  0xc128cb3a in nf_hook_thresh (hook=<value optimized out>,
skb=0xd7a73c00,
    in=<value optimized out>, out=0xdcbfd000,
    okfn=0xc128cc05 <br_forward_finish>, pf=<value optimized out>)
    at include/linux/netfilter.h:185
#5  NF_HOOK_THRESH (hook=<value optimized out>, skb=0xd7a73c00,
    in=<value optimized out>, out=0xdcbfd000,
    okfn=0xc128cc05 <br_forward_finish>, pf=<value optimized out>)
    at include/linux/netfilter.h:217
#6  NF_HOOK (hook=<value optimized out>, skb=0xd7a73c00,
---Type <return> to continue, or q <return> to quit---
    in=<value optimized out>, out=0xdcbfd000,
    okfn=0xc128cc05 <br_forward_finish>, pf=<value optimized out>)
    at include/linux/netfilter.h:241
#7  0xc128cc85 in __br_forward (to=<value optimized out>, skb=0x0)
    at net/bridge/br_forward.c:94
#8  0xc128c9e8 in deliver_clone (prev=0xd79a9e00, skb=<value optimized
out>,
    __packet_hook=0xc128cc20 <__br_forward>) at net/bridge/br_forward.c:137
#9  0xc128ca71 in br_flood (br=<value optimized out>, skb=0xd759d000,
    skb0=0xd759d000, __packet_hook=0xc128cc20 <__br_forward>)
    at net/bridge/br_forward.c:184
#10 0xc128ca99 in br_flood_forward (br=0x10003, skb=0x0, skb2=0x1)
    at net/bridge/br_forward.c:205
#11 0xc128d6bf in br_handle_frame_finish (skb=0xd759d000)
    at net/bridge/br_input.c:104
#12 0xc128d5fe in NF_HOOK_THRESH (hook=<value optimized out>,
skb=0xd759d000,
    in=<value optimized out>, okfn=0xc128d605 <br_handle_frame_finish>,
    out=<value optimized out>, pf=<value optimized out>)
    at include/linux/netfilter.h:219
#13 NF_HOOK (hook=<value optimized out>, skb=0xd759d000,
    in=<value optimized out>, okfn=0xc128d605 <br_handle_frame_finish>,
    out=<value optimized out>, pf=<value optimized out>)
    at include/linux/netfilter.h:241
#14 0xc128d87a in br_handle_frame (skb=0x0) at net/bridge/br_input.c:190
---Type <return> to continue, or q <return> to quit---
#15 0xc11e3c02 in __netif_receive_skb (skb=0xd759d000) at
net/core/dev.c:3137
#16 0xc11e7524 in netif_receive_skb (skb=0xd759d000) at net/core/dev.c:3231
#17 0xe0bca898 in ?? ()
#18 0xc11e7ab6 in net_rx_action (h=<value optimized out>)
    at net/core/dev.c:3779
#19 0xc1034345 in __do_softirq () at kernel/softirq.c:238
#20 0xc1003f96 in call_on_stack () at arch/x86/kernel/irq_32.c:66
#21 do_softirq () at arch/x86/kernel/irq_32.c:173
#22 0xc1034228 in irq_exit () at kernel/softirq.c:328
#23 0xc10037d3 in do_IRQ (regs=<value optimized out>)
    at arch/x86/kernel/irq.c:248
#24 0xc1002d70 in ?? () at arch/x86/kernel/entry_32.S:825
#25 0xc101b82b in native_safe_halt ()
    at /home/pollux/build/nf-next-2.6/arch/x86/include/asm/irqflags.h:49
#26 0xc1007e3f in arch_safe_halt ()
    at /home/pollux/build/nf-next-2.6/arch/x86/include/asm/paravirt.h:110
#27 default_idle () at arch/x86/kernel/process.c:380
#28 0xc1001a66 in cpu_idle () at arch/x86/kernel/process_32.c:112
#29 0xc12af2ce in start_secondary (unused=<value optimized out>)
    at arch/x86/kernel/smpboot.c:355
#30 0x00000000 in ?? ()

Re: [RFH] bridge: add new target NFQUEUE for ebtables

[Patrick McHardy]

Am 16.02.2011 17:57, schrieb Pierre Chifflier:
> Hi,
> 
> Thanks for your reply Patrick.
> So I did the following:
> - rebased on today's nf-next-2.6
> - apply only the first patch (which makes afinfo optional)
> - revert all other patches
> - apply the recent fix on nf_iterate since it was the cause of my oops
> 
> I patched ebtables to use xt_NFQUEUE (using a struct xt_NFQ_info_v1 with
> arguments queuenum 1 and queues_total 1), and removed any other change.
> 
> When I add a rule with the NFQUEUE target with ebtables, I almost
> immediately get a panic (full backtrace later in this mail).
> 
> What is weird is that I got a NULL skb  in ebt_in_hook (frame 2) while
> the skb was not NULL earlier - like if it was stolen by some hook. Any
> idea on what could cause that ?

The backtrace doesn't seem to be fully accurate. Please also post
the full oops output corresponding to the backtrace.

Two more questions:

- is the bridge device in promiscous mode?
- do you have IGMP snooping enabled?

Re: [RFH] bridge: add new target NFQUEUE for ebtables

[Pierre Chifflier]

On 02/17/2011 11:47 AM, Patrick McHardy wrote:
> Am 16.02.2011 17:57, schrieb Pierre Chifflier:
>> Hi,
>>
>> Thanks for your reply Patrick.
>> So I did the following:
>> - rebased on today's nf-next-2.6
>> - apply only the first patch (which makes afinfo optional)
>> - revert all other patches
>> - apply the recent fix on nf_iterate since it was the cause of my oops
>>
>> I patched ebtables to use xt_NFQUEUE (using a struct xt_NFQ_info_v1 with
>> arguments queuenum 1 and queues_total 1), and removed any other change.
>>
>> When I add a rule with the NFQUEUE target with ebtables, I almost
>> immediately get a panic (full backtrace later in this mail).
>>
>> What is weird is that I got a NULL skb  in ebt_in_hook (frame 2) while
>> the skb was not NULL earlier - like if it was stolen by some hook. Any
>> idea on what could cause that ?
> 
> The backtrace doesn't seem to be fully accurate. Please also post
> the full oops output corresponding to the backtrace.
> 
> Two more questions:
> 
> - is the bridge device in promiscous mode?
> - do you have IGMP snooping enabled?
> 

Here is the most relevant part of the log I could capture on the serial
port.
- Bridge device is not in promiscuous mode
- CONFIG_BRIDGE_ICMP_SNOOPING is not set

What I do to reproduce the crash:
- setup the bridge (at this point, everything is fine)
- load an ebtables rule: ebtables -A FORWARD -j NFQUEUE
the crash happens immediately when adding the rule.

If relevant, the code for ebt_NFQUEUE.c is available at
https://www.wzdftpd.net/downloads/ebt_NFQUEUE.c

Thanks,
Pierre

[   24.581479] 8021q: adding VLAN 0 to HW filter on device eth0
[   24.592863] eth1: link up, 100Mbps, full-duplex, lpa 0x05E1
[   24.603313] br0: port 2(eth1) entering learning state
[   24.605984] br0: port 2(eth1) entering learning state
[   24.608683] br0: port 1(eth0) entering learning state
[   24.611226] br0: port 1(eth0) entering learning state
[   39.648175] br0: port 2(eth1) entering forwarding state
[   39.654425] br0: port 1(eth0) entering forwarding state
[   56.168359] BUG: unable to handle kernel NULL pointer dereference at
00000008
[   56.172005] IP: [<c1292de3>] ebt_do_table+0x420/0x4bf
[   56.172005] *pde = 00000000
[   56.172005] Oops: 0002 [#1] SMP
[   56.172005] last sysfs file: /sys/devices/virtual/net/lo/operstate
[   56.172005] Modules linked in: usbhid hid psmouse serio_raw pcspkr
evdev virtio_balloon virtio_net virtio_blk 8139too uhci_hcd ehci_hcd
usbcore 8139cp mii virtio_pci virtio_ring virtio [last unloaded:
scsi_wait_scan]
[   56.172005]
[   56.172005] Pid: 0, comm: swapper Not tainted 2.6.38-rc1+ #11 /Bochs
[   56.172005] EIP: 0060:[<c1292de3>] EFLAGS: 00010202 CPU: 0
[   56.172005] EIP is at ebt_do_table+0x420/0x4bf
[   56.172005] EAX: 00010003 EBX: e10aa060 ECX: 00000001 EDX: 00000000
[   56.172005] ESI: e10aa030 EDI: e10aa030 EBP: df023de0 ESP: df023d7c
[   56.172005]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[   56.172005] Process swapper (pid: 0, ti=df022000 task=c13daf20
task.ti=c13ae000)
[   56.172005] Stack:
[   56.172005]  e10aa000 d76481b0 d76491b0 00000001 e10a7040 e10a7040
00000000 00000003
[   56.172005]  d78dfd80 d7649000 00000000 00000000 d786a070 00000070
c14042b4 e10aa0f4
[   56.172005]  d7649000 d7648000 d78dfd80 d78dfd80 00000002 df020007
c152c240 c1405adc
[   56.172005] Call Trace:
[   56.172005]  [<c1293753>] ebt_in_hook+0x18/0x1d
[   56.172005]  [<c11fc573>] nf_iterate+0x2f/0x74
[   56.172005]  [<c128cc05>] ? br_forward_finish+0x0/0x1b
[   56.172005]  [<c128cc05>] ? br_forward_finish+0x0/0x1b
[   56.172005]  [<c11fc5fb>] nf_hook_slow+0x43/0xd0
[   56.172005]  [<c128cc05>] ? br_forward_finish+0x0/0x1b
[   56.172005]  [<c128cc05>] ? br_forward_finish+0x0/0x1b
[   56.172005]  [<c128cc20>] ? __br_forward+0x0/0x6c
[   56.172005]  [<c128cb3a>] T.922+0x22/0x35
[   56.172005]  [<c128cc05>] ? br_forward_finish+0x0/0x1b
[   56.172005]  [<c128cc85>] __br_forward+0x65/0x6c
[   56.172005]  [<c128cc05>] ? br_forward_finish+0x0/0x1b
[   56.172005]  [<c11ddcca>] ? skb_clone+0x4d/0x54
[   56.172005]  [<c128c9e8>] deliver_clone+0x30/0x37
[   56.172005]  [<c128ca71>] br_flood+0x82/0x9d
[   56.172005]  [<c128ca99>] br_flood_forward+0xd/0x10
[   56.172005]  [<c128cc20>] ? __br_forward+0x0/0x6c
[   56.172005]  [<c128d6bf>] br_handle_frame_finish+0xba/0x113
[   56.172005]  [<c128d605>] ? br_handle_frame_finish+0x0/0x113
[   56.172005]  [<c128d5fe>] T.917+0x2d/0x34
[   56.172005]  [<c128d87a>] br_handle_frame+0x162/0x178
[   56.172005]  [<c128d605>] ? br_handle_frame_finish+0x0/0x113
[   56.172005]  [<c11e3c02>] __netif_receive_skb+0x1aa/0x2eb
[   56.172005]  [<c128d718>] ? br_handle_frame+0x0/0x178
[   56.172005]  [<c11e7524>] netif_receive_skb+0x5d/0x63
[   56.172005]  [<c11df762>] ? __netdev_alloc_skb+0x16/0x34
[   56.172005]  [<e0d7cf4f>] virtnet_poll+0x3bb/0x486 [virtio_net]
[   56.172005]  [<c11e7ab6>] net_rx_action+0x98/0x1be
[   56.172005]  [<c1034345>] __do_softirq+0x9c/0x157
[   56.172005]  [<c10342a9>] ? __do_softirq+0x0/0x157
[   56.172005]  <IRQ>

Re: [RFH] bridge: add new target NFQUEUE for ebtables

[Patrick McHardy]

Am 17.02.2011 14:37, schrieb Pierre Chifflier:
> On 02/17/2011 11:47 AM, Patrick McHardy wrote:
>> Am 16.02.2011 17:57, schrieb Pierre Chifflier:
>>> Hi,
>>>
>>> Thanks for your reply Patrick.
>>> So I did the following:
>>> - rebased on today's nf-next-2.6
>>> - apply only the first patch (which makes afinfo optional)
>>> - revert all other patches
>>> - apply the recent fix on nf_iterate since it was the cause of my oops
>>>
>>> I patched ebtables to use xt_NFQUEUE (using a struct xt_NFQ_info_v1 with
>>> arguments queuenum 1 and queues_total 1), and removed any other change.
>>>
>>> When I add a rule with the NFQUEUE target with ebtables, I almost
>>> immediately get a panic (full backtrace later in this mail).
>>>
>>> What is weird is that I got a NULL skb  in ebt_in_hook (frame 2) while
>>> the skb was not NULL earlier - like if it was stolen by some hook. Any
>>> idea on what could cause that ?
>>
>> The backtrace doesn't seem to be fully accurate. Please also post
>> the full oops output corresponding to the backtrace.
>>
>> Two more questions:
>>
>> - is the bridge device in promiscous mode?
>> - do you have IGMP snooping enabled?
>>
> 
> Here is the most relevant part of the log I could capture on the serial
> port.
> - Bridge device is not in promiscuous mode
> - CONFIG_BRIDGE_ICMP_SNOOPING is not set
> 
> What I do to reproduce the crash:
> - setup the bridge (at this point, everything is fine)
> - load an ebtables rule: ebtables -A FORWARD -j NFQUEUE
> the crash happens immediately when adding the rule.
> 
> If relevant, the code for ebt_NFQUEUE.c is available at
> https://www.wzdftpd.net/downloads/ebt_NFQUEUE.c
> 

Thanks, I'm going to give this a try myself during the weekend.