Re: [RFC PATCH] convert ebt_ulog to nfnetlink_log

On Fri, Aug 05, 2005 at 06:37:09PM +0200, Harald Welte wrote:

Ok, I'll extend nfnetlink_log.c to add TLV's for those two fields in
case CONFIG_BRIDGE_NF is set.  I'll send a patch later tonight.

What about the following (only compile-tested) patch ?

[NETFILTER] add phys{in,out}dev support to nfnetlink_log and nfnetlink_queue

Since bridges don't have a 'reasonable' input and output net_device
(e.g. 'br0'), we need to add seperate TLV's for the 'physindev' and 'physoutdev'
to get to the real underlying device (e.g. 'eth0').

I really hate to have those CONFIG_BRIDGE_NETFILTER ifdef's all over the
code - but for now we have them almost everywhere, so two more doesn't
hurt.  At some point we need to get rid of this ugliness and have something
like an 'input device stack' that can be traversed.

Signed-off-by: Harald Welte <redacted>

---
commit 9f7972a4e1af559a1aeecfacfb1d6ca87f99f762
tree 36b9459801b3e4a2c61d1238a32bddbb4b5946e0
parent ed8612c164a21e71cf6139c4e67a98b7a417b1cb
author Harald Welte [off-list ref] Fr, 05 Aug 2005 19:07:28 +0200
committer Harald Welte [off-list ref] Fr, 05 Aug 2005 19:07:28 +0200

 include/linux/netfilter/nfnetlink_log.h   |    2 ++
 include/linux/netfilter/nfnetlink_queue.h |    2 ++
 net/netfilter/nfnetlink_log.c             |   22 ++++++++++++++++++++++
 net/netfilter/nfnetlink_queue.c           |   22 ++++++++++++++++++++++
 4 files changed, 48 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 */

@@ -414,12 +418,26 @@ __build_packet_message(struct nfulnl_ins
 		tmp_uint = htonl(indev->ifindex);
 		NFA_PUT(inst->skb, NFULA_IFINDEX_INDEV, sizeof(tmp_uint),
 			&tmp_uint);
+#ifdef CONFIG_BRIDGE_NETFILTER
+		if (indev->br_port) {
+			tmp_uint = htonl(indev->br_port->br->dev->ifindex);
+			NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
+				sizeof(tmp_uint), &tmp_uint);
+		}
+#endif
 	}
 
 	if (outdev) {
 		tmp_uint = htonl(outdev->ifindex);
 		NFA_PUT(inst->skb, NFULA_IFINDEX_OUTDEV, sizeof(tmp_uint),
 			&tmp_uint);
+#ifdef CONFIG_BRIDGE_NETFILTER
+		if (outdev->br_port) {
+			tmp_uint = htonl(outdev->br_port->br->dev->ifindex);
+			NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSOUTDEV,
+				sizeof(tmp_uint), &tmp_uint);
+		}
+#endif
 	}
 
 	if (skb->nfmark) {

@@ -536,6 +554,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));

@@ -413,11 +421,25 @@ nfqnl_build_packet_message(struct nfqnl_
 	if (entry->info->indev) {
 		tmp_uint = htonl(entry->info->indev->ifindex);
 		NFA_PUT(skb, NFQA_IFINDEX_INDEV, sizeof(tmp_uint), &tmp_uint);
+#ifdef CONFIG_BRIDGE_NETFILTER
+		if (entry->info->indev->br_port) {
+			tmp_uint = htonl(entry->info->indev->br_port->br->dev->ifindex);
+			NFA_PUT(skb, NFQA_IFINDEX_PHYSINDEV, 
+				sizeof(tmp_uint), &tmp_uint);
+		}
+#endif
 	}
 
 	if (entry->info->outdev) {
 		tmp_uint = htonl(entry->info->outdev->ifindex);
 		NFA_PUT(skb, NFQA_IFINDEX_OUTDEV, sizeof(tmp_uint), &tmp_uint);
+#ifdef CONFIG_BRIDGE_NETFILTER
+		if (entry->info->outdev->br_port) {
+			tmp_uint = htonl(entry->info->outdev->br_port->br->dev->ifindex);
+			NFA_PUT(skb, NFQA_IFINDEX_PHYSOUTDEV, 
+				sizeof(tmp_uint), &tmp_uint);
+		}
+#endif
 	}
 
 	if (entry->skb->nfmark) {

-- 
- Harald Welte <laforge@gnumonks.org>          	        http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Op vr, 05-08-2005 te 19:10 +0200, schreef Harald Welte:

What about the following (only compile-tested) patch ?

[NETFILTER] add phys{in,out}dev support to nfnetlink_log and nfnetlink_queue

Since bridges don't have a 'reasonable' input and output net_device
(e.g. 'br0'), we need to add seperate TLV's for the 'physindev' and 'physoutdev'
to get to the real underlying device (e.g. 'eth0').

I really hate to have those CONFIG_BRIDGE_NETFILTER ifdef's all over the
code - but for now we have them almost everywhere, so two more doesn't
hurt.  At some point we need to get rid of this ugliness and have something
like an 'input device stack' that can be traversed.

I'm not sure if I understand that, how could this be used to know if
BRIDGE_NETFILTER was configured?

+#ifdef CONFIG_BRIDGE_NETFILTER
+		if (indev->br_port) {
+			tmp_uint = htonl(indev->br_port->br->dev->ifindex);
+			NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
+				sizeof(tmp_uint), &tmp_uint);
+		}

indev->br_port->br->dev->ifindex points to the ifindex of the bridge br0
when indev is a bridge port (eth0). So you need this to fill in the
input device (assuming we agree that the bridge is called the input
device, and the bridge port is called the physical input device).
Filling in the physical input device is done with indev (in case
indev->br_port is not NULL).

I can't easily get access to net-2.6.14, so I can't have a full view on
things. If you were to change ebt_ulog, you must also make sure that the
physindev is used for NFULA_IFINDEX_PHYSINDEV. The input_dev member of
the skb will be eth0 if the physindev is known.

Analogous comments hold for the other similar parts of your patch.

cheers,
Bart

On Sun, Aug 07, 2005 at 09:27:56PM +0000, Bart De Schuymer wrote:

quoted

I really hate to have those CONFIG_BRIDGE_NETFILTER ifdef's all over the
code - but for now we have them almost everywhere, so two more doesn't
hurt.  At some point we need to get rid of this ugliness and have something
like an 'input device stack' that can be traversed.

I'm not sure if I understand that, how could this be used to know if
BRIDGE_NETFILTER was configured?

not at all.  It was just my extremely vague idea about something more
generic, like a stack of input devices.  Those ifdefs would basically
disappear, and the history of input devices (let's say the individual
decapsulation layers for tunneling, ...) would be visible via some
common structure that could also be used by bridging.  You can ignore
that comment/rant, it has no practical relevance in the current
discussion ;)

quoted

+#ifdef CONFIG_BRIDGE_NETFILTER
+		if (indev->br_port) {
+			tmp_uint = htonl(indev->br_port->br->dev->ifindex);
+			NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
+				sizeof(tmp_uint), &tmp_uint);
+		}

indev->br_port->br->dev->ifindex points to the ifindex of the bridge br0
when indev is a bridge port (eth0). So you need this to fill in the
input device (assuming we agree that the bridge is called the input
device, and the bridge port is called the physical input device).
Filling in the physical input device is done with indev (in case
indev->br_port is not NULL).

I'm not sure if I can follow you.  What basically happens is:

ebt_ulog calls nf_log_packet(PF_BRIDGE, hooknr, skb, in, out, ...) where
"in" and "out" are struct net_device as passed to the ebt_ulog function
(which is part of the ebt_watcher).  

[...]

the code ends up at the function you are quoting above.  "indev"
is "in" and "outdev" is "out", transparently passed from
nf_log_packet().

So I think it should reflect exactly what you currently do in ebt_ulog.
The only diference is that you now get the ifindex and not the device
name string.

Cheers,
	Harald
-- 
- Harald Welte [off-list ref]                 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

Op zo, 07-08-2005 te 23:33 +0200, schreef Harald Welte: 

quoted

indev->br_port->br->dev->ifindex points to the ifindex of the bridge br0
when indev is a bridge port (eth0). So you need this to fill in the
input device (assuming we agree that the bridge is called the input
device, and the bridge port is called the physical input device).
Filling in the physical input device is done with indev (in case
indev->br_port is not NULL).

I'm not sure if I can follow you.  What basically happens is:

ebt_ulog calls nf_log_packet(PF_BRIDGE, hooknr, skb, in, out, ...) where
"in" and "out" are struct net_device as passed to the ebt_ulog function
(which is part of the ebt_watcher).  

[...]

the code ends up at the function you are quoting above.  "indev"
is "in" and "outdev" is "out", transparently passed from
nf_log_packet().

So I think it should reflect exactly what you currently do in ebt_ulog.
The only diference is that you now get the ifindex and not the device
name string.

ebt_ulog code snippet:
strcpy(pm->physindev, in->name);
/* If in isn't a bridge, then physindev==indev */
if (in->br_port)
strcpy(pm->indev, in->br_port->br->dev->name);
else
strcpy(pm->indev, in->name);

Your code snippet:
if (indev->br_port) {
tmp_uint = htonl(indev->br_port->br->dev->ifindex);
NFA_PUT(inst->skb, NFULA_IFINDEX_PHYSINDEV,
        sizeof(tmp_uint), &tmp_uint);
}

These two code fragments do not do the same thing. If indev is a bridge
port (meaning indev->br_port != NULL), then the NFULA_IFINDEX_PHYSINDEV
must be indev, while the NFULA_IFINDEX_INDEV must be in->br_port->br-

dev->name.

If indev is not a bridge port, the ebt_ulog code makes
NFULA_IFINDEX_PHYSINDEV and NFULA_IFINDEX_INDEV the same (indev).

cheers,
Bart

[removing kaber and davem from cc list]

On Mon, Aug 08, 2005 at 06:57:40AM +0000, Bart De Schuymer wrote:

quoted

So I think it should reflect exactly what you currently do in ebt_ulog.
The only diference is that you now get the ifindex and not the device
name string.

These two code fragments do not do the same thing. If indev is a bridge
port (meaning indev->br_port != NULL), then the NFULA_IFINDEX_PHYSINDEV
must be indev, while the NFULA_IFINDEX_INDEV must be in->br_port->br-

quoted

dev->name.

If indev is not a bridge port, the ebt_ulog code makes
NFULA_IFINDEX_PHYSINDEV and NFULA_IFINDEX_INDEV the same (indev).

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

-- 
- Harald Welte [off-list ref]                 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