[PATCH] Fix ip_queue for bridged packets

[PATCH] Fix ip_queue for bridged packets

[Bart De Schuymer]

Hi Dave,

When ip_queue copies an old skbuff to a new one (because the tailroom is
too small), it uses skb_copy_expand(). This function doesn't copy the
Ethernet header, which is not needed for normal IP traffic. Normally, the
Ethernet header is filled in later before doing dev_queue_xmit.
When ip_queue does this to a bridged IP packet, it has to copy the Ethernet
header, because the Ethernet header is already filled in and won't be filled
in again.
The patch below makes this happen. It puts the code that actually copies the
header inside netfilter_bridge.h, so that it can be reused and altered
without touching other code.

The patch is already approved by Stephen Hemminger (in private mail).
Since Harald Welte excels in silence, I'm sending netfilter stuff directly
to you from now on.

The patch is vs test7 but applies fine vs test8.

cheers,
Bart

--- linux-2.6.0-test7/net/ipv4/netfilter/ip_queue.c.old	2003-10-12 21:23:01.000000000 +0200
+++ linux-2.6.0-test7/net/ipv4/netfilter/ip_queue.c	2003-10-12 22:13:45.000000000 +0200
@@ -21,6 +21,9 @@
 #include <linux/netfilter.h>
 #include <linux/netfilter_ipv4/ip_queue.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
+#ifdef CONFIG_BRIDGE_NETFILTER
+#include <linux/netfilter_bridge.h>
+#endif
 #include <linux/netlink.h>
 #include <linux/spinlock.h>
 #include <linux/sysctl.h>
@@ -353,6 +356,10 @@ ipq_mangle_ipv4(ipq_verdict_msg_t *v, st
 			}
 			if (e->skb->sk)
 				skb_set_owner_w(newskb, e->skb->sk);
+#ifdef CONFIG_BRIDGE_NETFILTER
+			/* bridged packets already have their Ethernet header */
+			br_nf_maybe_copy_header2(newskb, e->skb);
+#endif
 			kfree_skb(e->skb);
 			e->skb = newskb;
 		}
--- linux-2.6.0-test7/include/linux/netfilter_bridge.h.old	2003-10-12 21:44:28.000000000 +0200
+++ linux-2.6.0-test7/include/linux/netfilter_bridge.h	2003-10-12 22:21:16.000000000 +0200
@@ -81,6 +81,22 @@ void nf_bridge_maybe_copy_header(struct 
 	}
 }
 
+/* needed in IP netfilter modules that can copy bridged skbuff's,
+ * but don't copy the Ethernet header by default. */
+static inline
+void nf_bridge_maybe_copy_header2(struct sk_buff *d,
+				  struct sk_buff *s)
+{
+	if (s->nf_bridge) {
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+		if (s->protocol == __constant_htons(ETH_P_8021Q))
+			memcpy(d->data - 18, s->data - 18, 18);
+		else
+#endif
+			memcpy(d->data - 16, s->data - 16, 16);
+	}
+}
+
 static inline
 void nf_bridge_save_header(struct sk_buff *skb)
 {

Re: [PATCH] Fix ip_queue for bridged packets

[David S. Miller]

On Sat, 25 Oct 2003 17:09:13 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> When ip_queue copies an old skbuff to a new one (because the tailroom is
> too small), it uses skb_copy_expand(). This function doesn't copy the
> Ethernet header, which is not needed for normal IP traffic. Normally, the
> Ethernet header is filled in later before doing dev_queue_xmit.
> When ip_queue does this to a bridged IP packet, it has to copy the Ethernet
> header, because the Ethernet header is already filled in and won't be filled
> in again.
> The patch below makes this happen. It puts the code that actually copies the
> header inside netfilter_bridge.h, so that it can be reused and altered
> without touching other code.
> 
> The patch is already approved by Stephen Hemminger (in private mail).
> Since Harald Welte excels in silence, I'm sending netfilter stuff directly
> to you from now on.

I thought we were taking care of this stuff via other means?

This problem feels like an old one to me, and it's why we added all
the bridge netfilter objects to struct sk_buff, right?  Why doesn't
that take care of this instance of the problem?

Re: [PATCH] Fix ip_queue for bridged packets

[Bart De Schuymer]

On Monday 27 October 2003 09:04, David S. Miller wrote:
> I thought we were taking care of this stuff via other means?
>
> This problem feels like an old one to me, and it's why we added all
> the bridge netfilter objects to struct sk_buff, right?  Why doesn't
> that take care of this instance of the problem?

The thing is, we currently only copy the Ethernet header to 
skb->nf_bridge->data at PF_BRIDGE/POST_ROUTING. So, if the queue happens 
earlier, the Ethernet header is lost.
Another solution is not trusting the iptables modules and doing the copy to 
skb->nf_bridge->data earlier at PF_BRIDGE/PRE_ROUTING and 
PF_BRIDGE/LOCAL_OUT. After each faked PF_INET hook we'd have to copy the 
skb->nf_bridge->data back to the Ethernet header since we don't trust the 
iptables modules.
This is doable, but kind of ugly and I'd prefer the explicit copy by the 
iptables module itself. Most iptables modules don't copy skb's around anyway. 
One could argue that this makes the iptables ip_queue module a tiny bit slower 
when dealing with non-bridged traffic, but if the user compiled her kernel 
with bridge_netfilter support one would think she needs it.
But I'll change it if anyone objects to the easy fix.

cheers,
Bart

Re: [PATCH] Fix ip_queue for bridged packets

[David S. Miller]

On Tue, 28 Oct 2003 00:02:11 +0100
Bart De Schuymer <bdschuym@pandora.be> wrote:

> On Monday 27 October 2003 09:04, David S. Miller wrote:
> > I thought we were taking care of this stuff via other means?
> >
> > This problem feels like an old one to me, and it's why we added all
> > the bridge netfilter objects to struct sk_buff, right?  Why doesn't
> > that take care of this instance of the problem?
> 
> The thing is, we currently only copy the Ethernet header to 
> skb->nf_bridge->data at PF_BRIDGE/POST_ROUTING. So, if the queue happens 
> earlier, the Ethernet header is lost.

Ok, let me think about this a little more.  I may just apply
your original patch.

Re: [PATCH] Fix ip_queue for bridged packets

[David S. Miller]

On Tue, 28 Oct 2003 00:02:11 +0100
Bart De Schuymer <bdschuym@pandora.be> wrote:

> One could argue that this makes the iptables ip_queue module a tiny bit slower 
> when dealing with non-bridged traffic, but if the user compiled her kernel 
> with bridge_netfilter support one would think she needs it.

Ok, there are other parts of netfilter (both ipv4 and ipv6) that
call skb_copy_expand() like this and I suspect we'll be copying
this bridging hack over to those places as well.  Not good.

skb_copy_expand() is not exactly a fast path, so let's just make
it copy the header area too.

Bart, does this fix your bug too?  Please test it, thanks.

--- net/core/skbuff.c.~1~	Tue Oct 28 06:54:49 2003
+++ net/core/skbuff.c	Tue Oct 28 06:58:38 2003
@@ -591,8 +591,8 @@
 	/* Set the tail pointer and length */
 	skb_put(n, skb->len);
 
-	/* Copy the data only. */
-	if (skb_copy_bits(skb, 0, n->data, skb->len))
+	/* Copy the linear data and header. */
+	if (skb_copy_bits(skb, -newheadroom, n->head, newheadroom + skb->len))
 		BUG();
 
 	copy_skb_header(n, skb);

Re: [PATCH] Fix ip_queue for bridged packets

[Bart De Schuymer]

On Tuesday 28 October 2003 15:56, David S. Miller wrote:
> skb_copy_expand() is not exactly a fast path, so let's just make
> it copy the header area too.
>
> Bart, does this fix your bug too?  Please test it, thanks.

I've actually never triggered the bug myself, it was just a fix for someone 
else's bug report. I'll get back to you.

cheers,
Bart

Re: [PATCH] Fix ip_queue for bridged packets

[Bart De Schuymer]

On Tuesday 28 October 2003 15:56, David S. Miller wrote:
> Ok, there are other parts of netfilter (both ipv4 and ipv6) that
> call skb_copy_expand() like this and I suspect we'll be copying
> this bridging hack over to those places as well.  Not good.

Note that bridged ipv6 is currently not seen by iptables6.

> skb_copy_expand() is not exactly a fast path, so let's just make
> it copy the header area too.
>
> Bart, does this fix your bug too?  Please test it, thanks.

It fixes the bug.

Thanks,
Bart

Re: [PATCH] Fix ip_queue for bridged packets

[David S. Miller]

On Wed, 29 Oct 2003 23:25:44 +0100
Bart De Schuymer <bdschuym@pandora.be> wrote:

> On Tuesday 28 October 2003 15:56, David S. Miller wrote:
> > skb_copy_expand() is not exactly a fast path, so let's just make
> > it copy the header area too.
> >
> > Bart, does this fix your bug too?  Please test it, thanks.
> 
> It fixes the bug.

Thanks a lot for getting it tested.

Re: [PATCH] Fix ip_queue for bridged packets

[Scott MacKay]

Caught this in the middle, so not sure all the
details...is this concerning the ebtable bridge patch?
If I uses the QUEUE target in PREROUTING to packet
mangle, is there an issue since I think PF_BRIDGE
comes after that?
-Scott

--- Bart De Schuymer <bdschuym@pandora.be> wrote:
> On Monday 27 October 2003 09:04, David S. Miller
> wrote:
> > I thought we were taking care of this stuff via
> other means?
> >
> > This problem feels like an old one to me, and it's
> why we added all
> > the bridge netfilter objects to struct sk_buff,
> right?  Why doesn't
> > that take care of this instance of the problem?
> 
> The thing is, we currently only copy the Ethernet
> header to 
> skb->nf_bridge->data at PF_BRIDGE/POST_ROUTING. So,
> if the queue happens 
> earlier, the Ethernet header is lost.
> Another solution is not trusting the iptables
> modules and doing the copy to 
> skb->nf_bridge->data earlier at
> PF_BRIDGE/PRE_ROUTING and 
> PF_BRIDGE/LOCAL_OUT. After each faked PF_INET hook
> we'd have to copy the 
> skb->nf_bridge->data back to the Ethernet header
> since we don't trust the 
> iptables modules.
> This is doable, but kind of ugly and I'd prefer the
> explicit copy by the 
> iptables module itself. Most iptables modules don't
> copy skb's around anyway. 
> One could argue that this makes the iptables
> ip_queue module a tiny bit slower 
> when dealing with non-bridged traffic, but if the
> user compiled her kernel 
> with bridge_netfilter support one would think she
> needs it.
> But I'll change it if anyone objects to the easy
> fix.
> 
> cheers,
> Bart
> 
> 


__________________________________
Do you Yahoo!?
Exclusive Video Premiere - Britney Spears
http://launch.yahoo.com/promos/britneyspears/

Re: [PATCH] Fix ip_queue for bridged packets

[Scott MacKay]

I had a question on this item and was wondering if it
is caused by the topic of this thread.

I have a QUEUE target, a custom C app running off of
ip_queue in userspace.  I am using the ebtables
bridging software under linux 2.4.20.  My QUEUE target
does some packet alteration (data content of TCP/UDP &
re-checksums, otherwise does not change header).  In
bridge mode, I cannot seem to place the QUEUE in
PREROUTING mangle or raw tables.  When I try, it seems
to not do any traffic.  If it is in POSTROUTING or
FORWARD or whatever, it is OK.  It also is OK if it is
routing in PREROUTING as opposed to bridging.  Are the
issued addressed by this patch discussion possibly the
cause of my problems?

-Scott

__________________________________
Do you Yahoo!?
Exclusive Video Premiere - Britney Spears
http://launch.yahoo.com/promos/britneyspears/

Re: [PATCH] Fix ip_queue for bridged packets

[Bart De Schuymer]

On Friday 31 October 2003 13:53, Scott MacKay wrote:
> I had a question on this item and was wondering if it
> is caused by the topic of this thread.

Oh, sorry, I forgot you mailed that question. I was going to mail back after I 
tested Dave's fix, but then I forgot...

> I have a QUEUE target, a custom C app running off of
> ip_queue in userspace.  I am using the ebtables
> bridging software under linux 2.4.20.  My QUEUE target
> does some packet alteration (data content of TCP/UDP &
> re-checksums, otherwise does not change header).  In
> bridge mode, I cannot seem to place the QUEUE in
> PREROUTING mangle or raw tables.  When I try, it seems
> to not do any traffic.  If it is in POSTROUTING or
> FORWARD or whatever, it is OK.  It also is OK if it is
> routing in PREROUTING as opposed to bridging.  Are the
> issued addressed by this patch discussion possibly the
> cause of my problems?

Sounds like that's the problem. It's probably just a coincidence that it 
worked in the FORWARD chain.
I'll cook up a new patch (for 2.4.22) this weekend. You can manually make 
Dave's changes if you don't want to use 2.4.22.

cheers,
Bart

[PATCH] Fix ip_queue for bridged packets

[Bart De Schuymer]

Hi Harald,

When ip_queue copies an old skbuff to a new one (because the tailroom is
too small), it uses skb_copy_expand(). This function doesn't copy the
Ethernet header, which is not needed for normal IP traffic. Normally, the
Ethernet header is filled in later before doing dev_queue_xmit.
When ip_queue does this to a bridged IP packet, it has to copy the Ethernet
header, because the Ethernet header was already filled in earlier and won't
be filled in again.
The patch below makes this happen. It puts the code that actually copies the
header inside netfilter_bridge.h, so that it can be reused and altered
without touching other code.

cheers,
Bart


--- linux-2.6.0-test7/net/ipv4/netfilter/ip_queue.c.old	2003-10-12 21:23:01.000000000 +0200
+++ linux-2.6.0-test7/net/ipv4/netfilter/ip_queue.c	2003-10-12 22:13:45.000000000 +0200
@@ -21,6 +21,9 @@
 #include <linux/netfilter.h>
 #include <linux/netfilter_ipv4/ip_queue.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
+#ifdef CONFIG_BRIDGE_NETFILTER
+#include <linux/netfilter_bridge.h>
+#endif
 #include <linux/netlink.h>
 #include <linux/spinlock.h>
 #include <linux/sysctl.h>
@@ -353,6 +356,10 @@ ipq_mangle_ipv4(ipq_verdict_msg_t *v, st
 			}
 			if (e->skb->sk)
 				skb_set_owner_w(newskb, e->skb->sk);
+#ifdef CONFIG_BRIDGE_NETFILTER
+			/* bridged packets already have their Ethernet header */
+			br_nf_maybe_copy_header2(newskb, e->skb);
+#endif
 			kfree_skb(e->skb);
 			e->skb = newskb;
 		}
--- linux-2.6.0-test7/include/linux/netfilter_bridge.h.old	2003-10-12 21:44:28.000000000 +0200
+++ linux-2.6.0-test7/include/linux/netfilter_bridge.h	2003-10-12 22:21:16.000000000 +0200
@@ -81,6 +81,22 @@ void nf_bridge_maybe_copy_header(struct 
 	}
 }
 
+/* needed in IP netfilter modules that can copy bridged skbuff's,
+ * but don't copy the Ethernet header by default. */
+static inline
+void nf_bridge_maybe_copy_header2(struct sk_buff *d,
+				  struct sk_buff *s)
+{
+	if (s->nf_bridge) {
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+		if (s->protocol == __constant_htons(ETH_P_8021Q))
+			memcpy(d->data - 18, s->data - 18, 18);
+		else
+#endif
+			memcpy(d->data - 16, s->data - 16, 16);
+	}
+}
+
 static inline
 void nf_bridge_save_header(struct sk_buff *skb)
 {

Re: [PATCH] Fix ip_queue for bridged packets

[Harald Welte]

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

On Sun, Oct 12, 2003 at 10:43:38PM +0200, Bart De Schuymer wrote:
> Hi Harald,
> 
> When ip_queue copies an old skbuff to a new one (because the tailroom is
> too small), it uses skb_copy_expand(). This function doesn't copy the
> Ethernet header, which is not needed for normal IP traffic. Normally, the
> Ethernet header is filled in later before doing dev_queue_xmit.
> When ip_queue does this to a bridged IP packet, it has to copy the Ethernet
> header, because the Ethernet header was already filled in earlier and won't
> be filled in again.

what about ip6_queue.c ?  Would you need a similar fix for ipv6
bridging  If yes, could you please merge your changes with ip6_queue.c
and send me a cumulative patch?

Thanks.

> cheers,
> Bart
-- 
- Harald Welte <laforge@netfilter.org>             http://www.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] Fix ip_queue for bridged packets

[Bart De Schuymer]

On Tuesday 14 October 2003 12:58, Harald Welte wrote:
> what about ip6_queue.c ?  Would you need a similar fix for ipv6
> bridging  If yes, could you please merge your changes with ip6_queue.c
> and send me a cumulative patch?

IPv6 isn't currently passed to ip6tables, so that code doesn't need fixing.

cheers,
Bart