Re: [RFC PATCH] convert ebt_ulog to nfnetlink_log

Re: [RFC PATCH] convert ebt_ulog to nfnetlink_log

[bdschuym@pandora.be]

(using webmail)

>----- Oorspronkelijk bericht -----
>Van: Harald Welte [mailto:laforge@netfilter.org]
>Ok, so this is actually the opposite problem of the ipt_physdev.c
>problem.  Here we have the physical device, but not the bridge group.
>
>This means we have to handle two separate cases:
>
>1) when nf_log_packet() gets called from the bridging code
>	indev == eth0
>	outdev == eth1
>		-> we need to resolve br0 from dev->br_port member
>
>2) when nf_log_packet() gets called from the ipv4 code:
>	indev == br0
>	outdev == br0
>		-> we need to resolve ethX from skb->nf_bridge member
>
>If I'm now correct, what about the following [reverse] patch (also
>attached the whole resulting file for your reference, since you don't
>have access to the tree).

There is one case missing: the brouter case. If br0=eth0+eth1 and a packet arrives at eth0 (not br0) in the IP code (not the bridge code), then the indev must be eth0, not br0.
How about something like this?

if (pf == PF_BRIDGE) { /* Called from ebtables */
	NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
		sizeof(tmp_uint), &tmp_uint);
	tmp_uint = htonl(indev->br_port->br->dev->ifindex);
	NFA_PUT(inst->skb, NFULA_IFINDEX_INDEV,
		sizeof(tmp_uint), &tmp_uint);
} else {
	NFA_PUT(inst->skb, NFULA_IFINDEX_INDEV,
		sizeof(tmp_uint), &tmp_uint);

	if (skb->nf_bridge && skb->nf_bridge->physindev)
		/* Called inside bridge code, but not from ebtables. */
		tmp_uint = htonl(skb->nf_bridge->physindev->ifindex);
	NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
		sizeof(tmp_uint), &tmp_uint);
}

cheers,
Bart

[PATCH] add bridging support to nfnetlink_{log,queue}

[Harald Welte]

[-- Attachment #1.1: Type: text/plain, Size: 704 bytes --]

> There is one case missing: the brouter case. If br0=eth0+eth1 and a
> packet arrives at eth0 (not br0) in the IP code (not the bridge code),
> then the indev must be eth0, not br0.  How about something like this?

Ok, I've implemented your suggested modifications now.

Dave: Please apply to your net-2.6.14 tree. Thanks!

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #1.2: 38-nfnetlink_queue_log-bridge.patch --]
[-- Type: text/plain, Size: 8495 bytes --]

[NETFILTER] add correct bridging support to nfnetlink_{queue,log}

This patch adds support for passing the real 'physical' device ifindex down
to userspace via nfnetlink_log and nfnetlink_queue.

This feature basically obsoletes net/bridge/netfilter/ebt_ulog.c, and it
is likely ebt_ulog.c will die with one of the next couple of patches.

Signed-off-by: Harald Welte <laforge@netfilter.org>

---
commit 5f748d529c9bf1f0f52453d02a5e3aa9eca351e3
tree f337de87d43b721ba0653e759c7d54d5daa27c52
parent e9d42778324d0432fe18b1a26d915d342a067351
author Harald Welte <laforge@netfilter.org> Mo, 08 Aug 2005 15:20:14 +0200
committer Harald Welte <laforge@netfilter.org> Mo, 08 Aug 2005 15:20:14 +0200

 include/linux/netfilter/nfnetlink_log.h   |    2 +
 include/linux/netfilter/nfnetlink_queue.h |    2 +
 net/netfilter/nfnetlink_log.c             |   58 +++++++++++++++++++++++++++++
 net/netfilter/nfnetlink_queue.c           |   58 +++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+), 0 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink_log.h b/include/linux/netfilter/nfnetlink_log.h
--- a/include/linux/netfilter/nfnetlink_log.h
+++ b/include/linux/netfilter/nfnetlink_log.h
@@ -40,6 +40,8 @@ enum nfulnl_attr_type {
 	NFULA_TIMESTAMP,		/* nfulnl_msg_packet_timestamp */
 	NFULA_IFINDEX_INDEV,		/* u_int32_t ifindex */
 	NFULA_IFINDEX_OUTDEV,		/* u_int32_t ifindex */
+	NFULA_IFINDEX_PHYSINDEV,	/* u_int32_t ifindex */
+	NFULA_IFINDEX_PHYSOUTDEV,	/* u_int32_t ifindex */
 	NFULA_HWADDR,			/* nfulnl_msg_packet_hw */
 	NFULA_PAYLOAD,			/* opaque data payload */
 	NFULA_PREFIX,			/* string prefix */
diff --git a/include/linux/netfilter/nfnetlink_queue.h b/include/linux/netfilter/nfnetlink_queue.h
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ b/include/linux/netfilter/nfnetlink_queue.h
@@ -36,6 +36,8 @@ enum nfqnl_attr_type {
 	NFQA_TIMESTAMP,			/* nfqnl_msg_packet_timestamp */
 	NFQA_IFINDEX_INDEV,		/* u_int32_t ifindex */
 	NFQA_IFINDEX_OUTDEV,		/* u_int32_t ifindex */
+	NFQA_IFINDEX_PHYSINDEV,		/* u_int32_t ifindex */
+	NFQA_IFINDEX_PHYSOUTDEV,	/* u_int32_t ifindex */
 	NFQA_HWADDR,			/* nfqnl_msg_packet_hw */
 	NFQA_PAYLOAD,			/* opaque data payload */
 
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -33,6 +33,10 @@
 
 #include <asm/atomic.h>
 
+#ifdef CONFIG_BRIDGE_NETFILTER
+#include "../bridge/br_private.h"
+#endif
+
 #define NFULNL_NLBUFSIZ_DEFAULT	4096
 #define NFULNL_TIMEOUT_DEFAULT 	100	/* every second */
 #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
@@ -412,14 +416,64 @@ __build_packet_message(struct nfulnl_ins
 
 	if (indev) {
 		tmp_uint = htonl(indev->ifindex);
+#ifndef CONFIG_BRIDGE_NETFILTER
 		NFA_PUT(inst->skb, NFULA_IFINDEX_INDEV, sizeof(tmp_uint),
 			&tmp_uint);
+#else
+		if (pf == PF_BRIDGE) {
+			/* Case 1: outdev is physical input device, we need to
+			 * look for bridge group (when called from
+			 * netfilter_bridge) */
+			NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
+				sizeof(tmp_uint), &tmp_uint);
+			/* this is the bridge group "brX" */
+			tmp_uint = htonl(indev->br_port->br->dev->ifindex);
+			NFA_PUT(inst->skb, NFULA_IFINDEX_INDEV,
+				sizeof(tmp_uint), &tmp_uint);
+		} else {
+			/* Case 2: indev is bridge group, we need to look for
+			 * physical device (when called from ipv4) */
+			NFA_PUT(inst->skb, NFULA_IFINDEX_INDEV,
+				sizeof(tmp_uint), &tmp_uint);
+			if (skb->nf_bridge && skb->nf_bridge->physindev) {
+				tmp_uint = 
+				    htonl(skb->nf_bridge->physindev->ifindex);
+				NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
+					sizeof(tmp_uint), &tmp_uint);
+			}
+		}
+#endif
 	}
 
 	if (outdev) {
 		tmp_uint = htonl(outdev->ifindex);
+#ifndef CONFIG_BRIDGE_NETFILTER
 		NFA_PUT(inst->skb, NFULA_IFINDEX_OUTDEV, sizeof(tmp_uint),
 			&tmp_uint);
+#else
+		if (pf == PF_BRIDGE) {
+			/* Case 1: outdev is physical output device, we need to
+			 * look for bridge group (when called from
+			 * netfilter_bridge) */
+			NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSOUTDEV,
+				sizeof(tmp_uint), &tmp_uint);
+			/* this is the bridge group "brX" */
+			tmp_uint = htonl(outdev->br_port->br->dev->ifindex);
+			NFA_PUT(inst->skb, NFULA_IFINDEX_OUTDEV,
+				sizeof(tmp_uint), &tmp_uint);
+		} else {
+			/* Case 2: indev is a bridge group, we need to look
+			 * for physical device (when called from ipv4) */
+			NFA_PUT(inst->skb, NFULA_IFINDEX_OUTDEV,
+				sizeof(tmp_uint), &tmp_uint);
+			if (skb->nf_bridge) {
+				tmp_uint = 
+				    htonl(skb->nf_bridge->physoutdev->ifindex);
+				NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSOUTDEV,
+					sizeof(tmp_uint), &tmp_uint);
+			}
+		}
+#endif
 	}
 
 	if (skb->nfmark) {
@@ -536,6 +590,10 @@ nfulnl_log_packet(unsigned int pf,
 		+ NFA_SPACE(sizeof(struct nfulnl_msg_packet_hdr))
 		+ NFA_SPACE(sizeof(u_int32_t))	/* ifindex */
 		+ NFA_SPACE(sizeof(u_int32_t))	/* ifindex */
+#ifdef CONFIG_BRIDGE_NETFILTER
+		+ NFA_SPACE(sizeof(u_int32_t))	/* ifindex */
+		+ NFA_SPACE(sizeof(u_int32_t))	/* ifindex */
+#endif
 		+ NFA_SPACE(sizeof(u_int32_t))	/* mark */
 		+ NFA_SPACE(sizeof(u_int32_t))	/* uid */
 		+ NFA_SPACE(NFULNL_PREFIXLEN)	/* prefix */
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -30,6 +30,10 @@
 
 #include <asm/atomic.h>
 
+#ifdef CONFIG_BRIDGE_NETFILTER
+#include "../bridge/br_private.h"
+#endif
+
 #define NFQNL_QMAX_DEFAULT 1024
 
 #if 0
@@ -361,6 +365,10 @@ nfqnl_build_packet_message(struct nfqnl_
 	size =    NLMSG_SPACE(sizeof(struct nfqnl_msg_packet_hdr))
 		+ NLMSG_SPACE(sizeof(u_int32_t))	/* ifindex */
 		+ NLMSG_SPACE(sizeof(u_int32_t))	/* ifindex */
+#ifdef CONFIG_BRIDGE_NETFILTER
+		+ NLMSG_SPACE(sizeof(u_int32_t))	/* ifindex */
+		+ NLMSG_SPACE(sizeof(u_int32_t))	/* ifindex */
+#endif
 		+ NLMSG_SPACE(sizeof(u_int32_t))	/* mark */
 		+ NLMSG_SPACE(sizeof(struct nfqnl_msg_packet_hw))
 		+ NLMSG_SPACE(sizeof(struct nfqnl_msg_packet_timestamp));
@@ -412,12 +420,62 @@ nfqnl_build_packet_message(struct nfqnl_
 
 	if (entry->info->indev) {
 		tmp_uint = htonl(entry->info->indev->ifindex);
+#ifndef CONFIG_BRIDGE_NETFILTER
 		NFA_PUT(skb, NFQA_IFINDEX_INDEV, sizeof(tmp_uint), &tmp_uint);
+#else
+		if (entry->info->pf == PF_BRIDGE) {
+			/* Case 1: indev is physical input device, we need to
+			 * look for bridge group (when called from 
+			 * netfilter_bridge) */
+			NFA_PUT(skb, NFQA_IFINDEX_PHYSINDEV, sizeof(tmp_uint), 
+				&tmp_uint);
+			/* this is the bridge group "brX" */
+			tmp_uint = htonl(entry->info->indev->br_port->br->dev->ifindex);
+			NFA_PUT(skb, NFQA_IFINDEX_INDEV, sizeof(tmp_uint),
+				&tmp_uint);
+		} else {
+			/* Case 2: indev is bridge group, we need to look for
+			 * physical device (when called from ipv4) */
+			NFA_PUT(skb, NFQA_IFINDEX_INDEV, sizeof(tmp_uint),
+				&tmp_uint);
+			if (entry->skb->nf_bridge
+			    && entry->skb->nf_bridge->physindev) {
+				tmp_uint = htonl(entry->skb->nf_bridge->physindev->ifindex);
+				NFA_PUT(skb, NFQA_IFINDEX_PHYSINDEV,
+					sizeof(tmp_uint), &tmp_uint);
+			}
+		}
+#endif
 	}
 
 	if (entry->info->outdev) {
 		tmp_uint = htonl(entry->info->outdev->ifindex);
+#ifndef CONFIG_BRIDGE_NETFILTER
 		NFA_PUT(skb, NFQA_IFINDEX_OUTDEV, sizeof(tmp_uint), &tmp_uint);
+#else
+		if (entry->info->pf == PF_BRIDGE) {
+			/* Case 1: outdev is physical output device, we need to
+			 * look for bridge group (when called from 
+			 * netfilter_bridge) */
+			NFA_PUT(skb, NFQA_IFINDEX_PHYSOUTDEV, sizeof(tmp_uint),
+				&tmp_uint);
+			/* this is the bridge group "brX" */
+			tmp_uint = htonl(entry->info->outdev->br_port->br->dev->ifindex);
+			NFA_PUT(skb, NFQA_IFINDEX_OUTDEV, sizeof(tmp_uint),
+				&tmp_uint);
+		} else {
+			/* Case 2: outdev is bridge group, we need to look for
+			 * physical output device (when called from ipv4) */
+			NFA_PUT(skb, NFQA_IFINDEX_OUTDEV, sizeof(tmp_uint),
+				&tmp_uint);
+			if (entry->skb->nf_bridge
+			    && entry->skb->nf_bridge->physoutdev) {
+				tmp_uint = htonl(entry->skb->nf_bridge->physoutdev->ifindex);
+				NFA_PUT(skb, NFQA_IFINDEX_PHYSOUTDEV,
+					sizeof(tmp_uint), &tmp_uint);
+			}
+		}
+#endif
 	}
 
 	if (entry->skb->nfmark) {

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add bridging support to nfnetlink_{log,queue}

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On Mon, Aug 08, 2005 at 05:43:29PM +0000, Bart De Schuymer wrote:
> Op ma, 08-08-2005 te 15:24 +0200, schreef Harald Welte:
> > > There is one case missing: the brouter case. If br0=eth0+eth1 and a
> > > packet arrives at eth0 (not br0) in the IP code (not the bridge code),
> > > then the indev must be eth0, not br0.  How about something like this?
> > 
> > Ok, I've implemented your suggested modifications now.
> 
> There's still one small issue: if CONFIG_BRIDGE_NETFILTER isn't set in
> the kernel configuration but ebtables is enabled, 

Doesn't ebtables attach to netfilter hooks? Ah, BRIDGE_NETFILTER
actually only selects the {ip,ip6,arp}tables emulation, not netfilter
support in bridging.  

> then the physindev should still be filled in if ebt_ulog is used. I'm
> afraid this will result in more ugly ifdef's.

well, If you can send me a patch for those ugly ifdef's after you get
back from holidays, I'll apply it.  Until then I think we can live
without that missing bit.

> I don't mind making CONFIG_BRIDGE_NETFILTER mandatory for people wanting
> to log the logical {in,out}put device, if you feel it would uglify the
> code too much otherwise... 

No, I don't think we should force people to use certain config options
if they're technically not required.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add bridging support to nfnetlink_{log,queue}

[Bart De Schuymer]

Op ma, 08-08-2005 te 15:24 +0200, schreef Harald Welte:
> > There is one case missing: the brouter case. If br0=eth0+eth1 and a
> > packet arrives at eth0 (not br0) in the IP code (not the bridge code),
> > then the indev must be eth0, not br0.  How about something like this?
> 
> Ok, I've implemented your suggested modifications now.

There's still one small issue: if CONFIG_BRIDGE_NETFILTER isn't set in
the kernel configuration but ebtables is enabled, then the physindev
should still be filled in if ebt_ulog is used. I'm afraid this will
result in more ugly ifdef's.
I don't mind making CONFIG_BRIDGE_NETFILTER mandatory for people wanting
to log the logical {in,out}put device, if you feel it would uglify the
code too much otherwise... The {in,out}dev sent to userspace will then
be different depending on whether CONFIG_BRIDGE_NETFILTER is set or not.
People can still disable bridge-nf at runtime with the right /proc
entry. This should then be stated somewhere very clearly.

cheers,
Bart

Re: [PATCH] add bridging support to nfnetlink_{log,queue}

[David S. Miller]

From: Harald Welte <laforge@netfilter.org>
Date: Mon, 8 Aug 2005 15:24:13 +0200

> [NETFILTER] add correct bridging support to nfnetlink_{queue,log}

Applied.

Re: [PATCH] add bridging support to nfnetlink_{log,queue}

[Bart De Schuymer]

Op ma, 08-08-2005 te 19:36 +0200, schreef Harald Welte:
> > then the physindev should still be filled in if ebt_ulog is used. I'm
> > afraid this will result in more ugly ifdef's.
> 
> well, If you can send me a patch for those ugly ifdef's after you get
> back from holidays, I'll apply it.  Until then I think we can live
> without that missing bit.

Perhaps I'd better wait then until I have access to the latest changes.
Transparency never has been the greatest asset of the netfilter
development.

cheers,
Bart