[PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Bart De Schuymer]

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

Hi,

The attached patch does the following:
1. fix a bug introduced in commit 
9d02002d2dc2c7423e5891b97727fde4d667adf1 (2/10/2006) which made 
ipt_REJECT stop work for bridged traffic (use of nskb instead of oldskb)
2. use the correct source MAC address for the response (bug reported in 
bug 531 of netfilter's bugzilla)

Tested for plain IP traffic and IP traffic encapsulated inside a VLAN 
header (should also work for PPPoE encapsulated IP traffic).


Signed-off-by: Bart De Schuymer <bdschuym@pandora.be>

cheers,
Bart

-- 
Bart De Schuymer
www.artinalgorithms.be


[-- Attachment #2: ipt_REJECT_use_correct_SMAC.diff --]
[-- Type: text/plain, Size: 2814 bytes --]

--- linux-2.6.31-uml/include/linux/netfilter_bridge.h.ori	2009-11-02 20:58:59.000000000 +0100
+++ linux-2.6.31-uml/include/linux/netfilter_bridge.h	2009-11-02 19:58:09.000000000 +0100
@@ -44,6 +44,7 @@ enum nf_br_hook_priorities {
 #define BRNF_DONT_TAKE_PARENT		0x04
 #define BRNF_BRIDGED			0x08
 #define BRNF_NF_BRIDGE_PREROUTING	0x10
+#define BRNF_COPY_MAC_SADDR		0x20
 
 
 /* Only used in br_forward.c */
@@ -77,6 +78,15 @@ static inline unsigned int nf_bridge_pad
 	return 0;
 }
 
+static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
+{
+	skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
+	if (likely(skb->nf_bridge))
+		atomic_set(&(skb->nf_bridge->use), 1);
+
+	return skb->nf_bridge;
+}
+
 struct bridge_skb_cb {
 	union {
 		__be32 ipv4;
--- linux-2.6.31-uml/net/bridge/br_netfilter.c.fixed	2009-11-02 21:22:00.000000000 +0100
+++ linux-2.6.31-uml/net/bridge/br_netfilter.c	2009-11-03 22:18:41.000000000 +0100
@@ -145,15 +145,6 @@ static inline struct net_device *bridge_
 	return port ? port->br->dev : NULL;
 }
 
-static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
-{
-	skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
-	if (likely(skb->nf_bridge))
-		atomic_set(&(skb->nf_bridge->use), 1);
-
-	return skb->nf_bridge;
-}
-
 static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
 {
 	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
@@ -775,6 +766,13 @@ static unsigned int br_nf_local_out(unsi
 		return NF_DROP;
 
 	nf_bridge = skb->nf_bridge;
+	/* Enable complete transparency for e.g. ipt_REJECT */
+	if (nf_bridge->mask & BRNF_COPY_MAC_SADDR) {
+		skb_copy_to_linear_data_offset(skb, -8, nf_bridge->data, 6);
+		nf_bridge_put(nf_bridge);
+		skb->nf_bridge = NULL;
+		return NF_ACCEPT;
+	}
 	if (!(nf_bridge->mask & BRNF_BRIDGED_DNAT))
 		return NF_ACCEPT;
 
--- linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c.ori	2009-10-31 19:31:54.000000000 +0100
+++ linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c	2009-11-03 21:55:08.000000000 +0100
@@ -100,11 +100,19 @@ static void send_reset(struct sk_buff *o
 						    sizeof(struct tcphdr), 0));
 
 	addr_type = RTN_UNSPEC;
-	if (hook != NF_INET_FORWARD
 #ifdef CONFIG_BRIDGE_NETFILTER
-	    || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
+	if (oldskb->nf_bridge && oldskb->nf_bridge->mask & BRNF_BRIDGED) {
+		int daddr_offset = -14 - nf_bridge_encap_header_len(oldskb);
+
+		addr_type = RTN_LOCAL;
+		if (!nf_bridge_alloc(nskb))
+			goto free_nskb;
+		nskb->nf_bridge->mask |= BRNF_COPY_MAC_SADDR;
+		skb_copy_from_linear_data_offset(oldskb, daddr_offset,
+						 nskb->nf_bridge->data, 6);
+	} else
 #endif
-	   )
+	if (hook != NF_INET_FORWARD)
 		addr_type = RTN_LOCAL;
 
 	/* ip_route_me_harder expects skb->dst to be set */

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Patrick McHardy]

Bart De Schuymer wrote:
> Hi,
> 
> The attached patch does the following:
> 1. fix a bug introduced in commit
> 9d02002d2dc2c7423e5891b97727fde4d667adf1 (2/10/2006) which made
> ipt_REJECT stop work for bridged traffic (use of nskb instead of oldskb)
> 2. use the correct source MAC address for the response (bug reported in
> bug 531 of netfilter's bugzilla)
> 
> Tested for plain IP traffic and IP traffic encapsulated inside a VLAN
> header (should also work for PPPoE encapsulated IP traffic).
> 
> 
> --- linux-2.6.31-uml/net/bridge/br_netfilter.c.fixed	2009-11-02 21:22:00.000000000 +0100
> +++ linux-2.6.31-uml/net/bridge/br_netfilter.c	2009-11-03 22:18:41.000000000 +0100
> @@ -775,6 +766,13 @@ static unsigned int br_nf_local_out(unsi
>  		return NF_DROP;
>  
>  	nf_bridge = skb->nf_bridge;
> +	/* Enable complete transparency for e.g. ipt_REJECT */
> +	if (nf_bridge->mask & BRNF_COPY_MAC_SADDR) {
> +		skb_copy_to_linear_data_offset(skb, -8, nf_bridge->data, 6);

Please use the proper ETH_*LEN values. I guess that would be
skb_copy_to_linear_data_offset(skb, -(ETH_HLEN - ETH_ALEN),
                               nf_bridge->data, ETH_ALEN)

> +		nf_bridge_put(nf_bridge);
> +		skb->nf_bridge = NULL;
> +		return NF_ACCEPT;

Shouldn't packets with BRNF_BRIDGED_DNAT continue through NF_BR_FORWARD
like they used to?

> +	}
>  	if (!(nf_bridge->mask & BRNF_BRIDGED_DNAT))
>  		return NF_ACCEPT;
>  
> --- linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c.ori	2009-10-31 19:31:54.000000000 +0100
> +++ linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c	2009-11-03 21:55:08.000000000 +0100
> @@ -100,11 +100,19 @@ static void send_reset(struct sk_buff *o
>  						    sizeof(struct tcphdr), 0));
>  
>  	addr_type = RTN_UNSPEC;
> -	if (hook != NF_INET_FORWARD
>  #ifdef CONFIG_BRIDGE_NETFILTER
> -	    || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
> +	if (oldskb->nf_bridge && oldskb->nf_bridge->mask & BRNF_BRIDGED) {
> +		int daddr_offset = -14 - nf_bridge_encap_header_len(oldskb);
> +
> +		addr_type = RTN_LOCAL;
> +		if (!nf_bridge_alloc(nskb))
> +			goto free_nskb;
> +		nskb->nf_bridge->mask |= BRNF_COPY_MAC_SADDR;
> +		skb_copy_from_linear_data_offset(oldskb, daddr_offset,
> +						 nskb->nf_bridge->data, 6);

Also proper ETH_* values please. But I'm wondering, we already save
the entire header in br_nf_post_routing(). Can't that be done earlier
so the upper layers don't have to care about this stuff and can simply
attach the original nf_bridge reference?

I'm also wondering - how are ICMP rejects handled?

> +	} else
>  #endif
> -	   )
> +	if (hook != NF_INET_FORWARD)
>  		addr_type = RTN_LOCAL;

We used to route all bridged packets as RTN_LOCAL for some reason
which I'm not sure of. This is not necessary anymore?

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Bart De Schuymer]

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

Patrick McHardy schreef:
> Bart De Schuymer wrote:
>   
>> Hi,
>>
>> The attached patch does the following:
>> 1. fix a bug introduced in commit
>> 9d02002d2dc2c7423e5891b97727fde4d667adf1 (2/10/2006) which made
>> ipt_REJECT stop work for bridged traffic (use of nskb instead of oldskb)
>> 2. use the correct source MAC address for the response (bug reported in
>> bug 531 of netfilter's bugzilla)
>>
>> Tested for plain IP traffic and IP traffic encapsulated inside a VLAN
>> header (should also work for PPPoE encapsulated IP traffic).
>>
>>
>> --- linux-2.6.31-uml/net/bridge/br_netfilter.c.fixed	2009-11-02 21:22:00.000000000 +0100
>> +++ linux-2.6.31-uml/net/bridge/br_netfilter.c	2009-11-03 22:18:41.000000000 +0100
>> @@ -775,6 +766,13 @@ static unsigned int br_nf_local_out(unsi
>>  		return NF_DROP;
>>  
>>  	nf_bridge = skb->nf_bridge;
>> +	/* Enable complete transparency for e.g. ipt_REJECT */
>> +	if (nf_bridge->mask & BRNF_COPY_MAC_SADDR) {
>> +		skb_copy_to_linear_data_offset(skb, -8, nf_bridge->data, 6);
>>     
>
> Please use the proper ETH_*LEN values. I guess that would be
> skb_copy_to_linear_data_offset(skb, -(ETH_HLEN - ETH_ALEN),
>                                nf_bridge->data, ETH_ALEN)
>
>   
Done, see attached patch.

>> +		nf_bridge_put(nf_bridge);
>> +		skb->nf_bridge = NULL;
>> +		return NF_ACCEPT;
>>     
>
> Shouldn't packets with BRNF_BRIDGED_DNAT continue through NF_BR_FORWARD
> like they used to?
>
>   
Yes. It is impossible for an skbuff to have both flags set.

>> +	}
>>  	if (!(nf_bridge->mask & BRNF_BRIDGED_DNAT))
>>  		return NF_ACCEPT;
>>  
>> --- linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c.ori	2009-10-31 19:31:54.000000000 +0100
>> +++ linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c	2009-11-03 21:55:08.000000000 +0100
>> @@ -100,11 +100,19 @@ static void send_reset(struct sk_buff *o
>>  						    sizeof(struct tcphdr), 0));
>>  
>>  	addr_type = RTN_UNSPEC;
>> -	if (hook != NF_INET_FORWARD
>>  #ifdef CONFIG_BRIDGE_NETFILTER
>> -	    || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
>> +	if (oldskb->nf_bridge && oldskb->nf_bridge->mask & BRNF_BRIDGED) {
>> +		int daddr_offset = -14 - nf_bridge_encap_header_len(oldskb);
>> +
>> +		addr_type = RTN_LOCAL;
>> +		if (!nf_bridge_alloc(nskb))
>> +			goto free_nskb;
>> +		nskb->nf_bridge->mask |= BRNF_COPY_MAC_SADDR;
>> +		skb_copy_from_linear_data_offset(oldskb, daddr_offset,
>> +						 nskb->nf_bridge->data, 6);
>>     
>
> Also proper ETH_* values please. But I'm wondering, we already save
> the entire header in br_nf_post_routing(). Can't that be done earlier
> so the upper layers don't have to care about this stuff and can simply
> attach the original nf_bridge reference?
>
>   
If you don't save the correct MAC address for the newly created skbuff 
in ipt_REJECT, there is no way to get it back later. Furthermore, if you 
save the header too early, MAC SNAT and DNAT might have changed the 
header and you have to resave the header anyway.
> I'm also wondering - how are ICMP rejects handled?
>
>   
Good question :-)
ICMP packets currently get sent with a source IP and MAC address of the 
bridge. If the bridge doesn't have an IP address but does have a 
suitable route, the source address is 0.0.0.0. I'll look into fixing this.

>> +	} else
>>  #endif
>> -	   )
>> +	if (hook != NF_INET_FORWARD)
>>  		addr_type = RTN_LOCAL;
>>     
>
> We used to route all bridged packets as RTN_LOCAL for some reason
> which I'm not sure of. This is not necessary anymore?
>
>   
Yes it is still necessary and it's in the patch. There used to be 
special code for bridge-netfilter in send_reset because we didn't want 
to enforce ip_forward to be enabled to be able to send the reset answers 
(there used to be a comment about that in ipt_REJECT). However, the 
commit I mentioned above rewrote the send_reset function and enabling 
ip_forward isn't enough anymore: it doesn't work without RTN_LOCAL.

cheers,
Bart

-- 
Bart De Schuymer
www.artinalgorithms.be


[-- Attachment #2: ipt_REJECT_use_correct_SMAC.diff --]
[-- Type: text/plain, Size: 2867 bytes --]

--- linux-2.6.31-uml/include/linux/netfilter_bridge.h.ori	2009-11-02 20:58:59.000000000 +0100
+++ linux-2.6.31-uml/include/linux/netfilter_bridge.h	2009-11-02 19:58:09.000000000 +0100
@@ -44,6 +44,7 @@ enum nf_br_hook_priorities {
 #define BRNF_DONT_TAKE_PARENT		0x04
 #define BRNF_BRIDGED			0x08
 #define BRNF_NF_BRIDGE_PREROUTING	0x10
+#define BRNF_COPY_MAC_SADDR		0x20
 
 
 /* Only used in br_forward.c */
@@ -77,6 +78,15 @@ static inline unsigned int nf_bridge_pad
 	return 0;
 }
 
+static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
+{
+	skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
+	if (likely(skb->nf_bridge))
+		atomic_set(&(skb->nf_bridge->use), 1);
+
+	return skb->nf_bridge;
+}
+
 struct bridge_skb_cb {
 	union {
 		__be32 ipv4;
--- linux-2.6.31-uml/net/bridge/br_netfilter.c.fixed	2009-11-02 21:22:00.000000000 +0100
+++ linux-2.6.31-uml/net/bridge/br_netfilter.c	2009-11-05 19:30:17.000000000 +0100
@@ -145,15 +145,6 @@ static inline struct net_device *bridge_
 	return port ? port->br->dev : NULL;
 }
 
-static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
-{
-	skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
-	if (likely(skb->nf_bridge))
-		atomic_set(&(skb->nf_bridge->use), 1);
-
-	return skb->nf_bridge;
-}
-
 static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
 {
 	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
@@ -775,6 +766,14 @@ static unsigned int br_nf_local_out(unsi
 		return NF_DROP;
 
 	nf_bridge = skb->nf_bridge;
+	/* Enable complete transparency for e.g. ipt_REJECT */
+	if (nf_bridge->mask & BRNF_COPY_MAC_SADDR) {
+		skb_copy_to_linear_data_offset(skb, -(ETH_HLEN - ETH_ALEN),
+					       nf_bridge->data, ETH_ALEN);
+		nf_bridge_put(nf_bridge);
+		skb->nf_bridge = NULL;
+		return NF_ACCEPT;
+	}
 	if (!(nf_bridge->mask & BRNF_BRIDGED_DNAT))
 		return NF_ACCEPT;
 
--- linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c.ori	2009-10-31 19:31:54.000000000 +0100
+++ linux-2.6.31-uml/net/ipv4/netfilter/ipt_REJECT.c	2009-11-05 19:29:58.000000000 +0100
@@ -100,11 +100,19 @@ static void send_reset(struct sk_buff *o
 						    sizeof(struct tcphdr), 0));
 
 	addr_type = RTN_UNSPEC;
-	if (hook != NF_INET_FORWARD
 #ifdef CONFIG_BRIDGE_NETFILTER
-	    || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
+	if (oldskb->nf_bridge && oldskb->nf_bridge->mask & BRNF_BRIDGED) {
+		int daddr_offset = -ETH_HLEN - nf_bridge_encap_header_len(oldskb);
+
+		addr_type = RTN_LOCAL;
+		if (!nf_bridge_alloc(nskb))
+			goto free_nskb;
+		nskb->nf_bridge->mask |= BRNF_COPY_MAC_SADDR;
+		skb_copy_from_linear_data_offset(oldskb, daddr_offset,
+						 nskb->nf_bridge->data, ETH_ALEN);
+	} else
 #endif
-	   )
+	if (hook != NF_INET_FORWARD)
 		addr_type = RTN_LOCAL;
 
 	/* ip_route_me_harder expects skb->dst to be set */

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Patrick McHardy]

Bart De Schuymer wrote:
> Patrick McHardy schreef:
>> Also proper ETH_* values please. But I'm wondering, we already save
>> the entire header in br_nf_post_routing(). Can't that be done earlier
>> so the upper layers don't have to care about this stuff and can simply
>> attach the original nf_bridge reference?
>>
>>   
> If you don't save the correct MAC address for the newly created skbuff
> in ipt_REJECT, there is no way to get it back later. Furthermore, if you
> save the header too early, MAC SNAT and DNAT might have changed the
> header and you have to resave the header anyway.

Yes, we need to save it at some point. My idea was that we might be able
to save it in PREROUTING instead of POSTROUTING and only do

nskb->nf_bridge = nf_bridge_get(oskb->nf_bridge)

in ipt_REJECT and probably also the ICMP code. MAC NAT could be handled
by updating the bridge info simultaneously.

>> I'm also wondering - how are ICMP rejects handled?
>>
>>   
> Good question :-)
> ICMP packets currently get sent with a source IP and MAC address of the
> bridge. If the bridge doesn't have an IP address but does have a
> suitable route, the source address is 0.0.0.0. I'll look into fixing this.

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Bart De Schuymer]

Patrick McHardy schreef:
> Bart De Schuymer wrote:
>   
>> Patrick McHardy schreef:
>>     
>>> Also proper ETH_* values please. But I'm wondering, we already save
>>> the entire header in br_nf_post_routing(). Can't that be done earlier
>>> so the upper layers don't have to care about this stuff and can simply
>>> attach the original nf_bridge reference?
>>>
>>>   
>>>       
>> If you don't save the correct MAC address for the newly created skbuff
>> in ipt_REJECT, there is no way to get it back later. Furthermore, if you
>> save the header too early, MAC SNAT and DNAT might have changed the
>> header and you have to resave the header anyway.
>>     
>
> Yes, we need to save it at some point. My idea was that we might be able
> to save it in PREROUTING instead of POSTROUTING and only do
>
> nskb->nf_bridge = nf_bridge_get(oskb->nf_bridge)
>
> in ipt_REJECT and probably also the ICMP code. MAC NAT could be handled
> by updating the bridge info simultaneously.
>
>   
Patrick,

The code creates a new skbuf and the correct source MAC address is lost 
if you don't attach it to the skbuf at that time. How will you know in 
PREROUTING what SMAC to use if you didn't save it when you created the 
skbuf?

cheers,
Bart

-- 
Bart De Schuymer
www.artinalgorithms.be

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Patrick McHardy]

Bart De Schuymer wrote:
> Patrick McHardy schreef:
>> Yes, we need to save it at some point. My idea was that we might be able
>> to save it in PREROUTING instead of POSTROUTING and only do
>>
>> nskb->nf_bridge = nf_bridge_get(oskb->nf_bridge)
>>
>> in ipt_REJECT and probably also the ICMP code. MAC NAT could be handled
>> by updating the bridge info simultaneously.
>>
>>   
> The code creates a new skbuf and the correct source MAC address is lost
> if you don't attach it to the skbuf at that time.

That's what I'm doing above.

> How will you know in
> PREROUTING what SMAC to use if you didn't save it when you created the
> skbuf?

I'm not sure I understand what you're getting at. The above
line of code would do exactly that, attach the nf_bridge
data from the original packet to the newly created one.
But for this to work we need to make sure its valid in all
hooks, hence my suggestion to save it in PREROUTING instead
of POSTROUTING.

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Patrick McHardy]

Patrick McHardy wrote:
> Bart De Schuymer wrote:
>> Patrick McHardy schreef:
>>> Yes, we need to save it at some point. My idea was that we might be able
>>> to save it in PREROUTING instead of POSTROUTING and only do
>>>
>>> nskb->nf_bridge = nf_bridge_get(oskb->nf_bridge)
>>>
>>> in ipt_REJECT and probably also the ICMP code. MAC NAT could be handled
>>> by updating the bridge info simultaneously.
>>>
>>>   
>> The code creates a new skbuf and the correct source MAC address is lost
>> if you don't attach it to the skbuf at that time.
> 
> That's what I'm doing above.
> 
>> How will you know in
>> PREROUTING what SMAC to use if you didn't save it when you created the
>> skbuf?
> 
> I'm not sure I understand what you're getting at. The above
> line of code would do exactly that, attach the nf_bridge
> data from the original packet to the newly created one.
> But for this to work we need to make sure its valid in all
> hooks, hence my suggestion to save it in PREROUTING instead
> of POSTROUTING.

This patchTo demonstrate the idea

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Patrick McHardy]

Patrick McHardy wrote:
> Patrick McHardy wrote:
>> Bart De Schuymer wrote:
>>> Patrick McHardy schreef:
>>>> Yes, we need to save it at some point. My idea was that we might be able
>>>> to save it in PREROUTING instead of POSTROUTING and only do
>>>>
>>>> nskb->nf_bridge = nf_bridge_get(oskb->nf_bridge)
>>>>
>>>> in ipt_REJECT and probably also the ICMP code. MAC NAT could be handled
>>>> by updating the bridge info simultaneously.
>>>>
>>>>   
>>> The code creates a new skbuf and the correct source MAC address is lost
>>> if you don't attach it to the skbuf at that time.
>> That's what I'm doing above.
>>
>>> How will you know in
>>> PREROUTING what SMAC to use if you didn't save it when you created the
>>> skbuf?
>> I'm not sure I understand what you're getting at. The above
>> line of code would do exactly that, attach the nf_bridge
>> data from the original packet to the newly created one.
>> But for this to work we need to make sure its valid in all
>> hooks, hence my suggestion to save it in PREROUTING instead
>> of POSTROUTING.
> 
> This patchTo demonstrate the idea

Please ignore, hit the wrong key :)

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Patrick McHardy]

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

Patrick McHardy wrote:
> Bart De Schuymer wrote:
>> Patrick McHardy schreef:
>>> Yes, we need to save it at some point. My idea was that we might be able
>>> to save it in PREROUTING instead of POSTROUTING and only do
>>>
>>> nskb->nf_bridge = nf_bridge_get(oskb->nf_bridge)
>>>
>>> in ipt_REJECT and probably also the ICMP code. MAC NAT could be handled
>>> by updating the bridge info simultaneously.
>>>
>>>   
>> The code creates a new skbuf and the correct source MAC address is lost
>> if you don't attach it to the skbuf at that time.
> 
> That's what I'm doing above.
> 
>> How will you know in
>> PREROUTING what SMAC to use if you didn't save it when you created the
>> skbuf?
> 
> I'm not sure I understand what you're getting at. The above
> line of code would do exactly that, attach the nf_bridge
> data from the original packet to the newly created one.
> But for this to work we need to make sure its valid in all
> hooks, hence my suggestion to save it in PREROUTING instead
> of POSTROUTING.

This patch demonstrates the idea. Its not compile tested
and incomplete, just to make more clear what I'm suggesting.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2970 bytes --]

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0c68fbd..410b0dc 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1952,10 +1952,11 @@ static inline void nf_bridge_put(struct nf_bridge_info *nf_bridge)
 	if (nf_bridge && atomic_dec_and_test(&nf_bridge->use))
 		kfree(nf_bridge);
 }
-static inline void nf_bridge_get(struct nf_bridge_info *nf_bridge)
+static inline struct nf_bridge_info *nf_bridge_get(struct nf_bridge_info *nf_bridge)
 {
 	if (nf_bridge)
 		atomic_inc(&nf_bridge->use);
+	return nf_bridge;
 }
 #endif /* CONFIG_BRIDGE_NETFILTER */
 static inline void nf_reset(struct sk_buff *skb)
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index a16a234..0732b3b 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -567,6 +567,8 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 			return NF_ACCEPT;
 #endif
 		nf_bridge_pull_encap_header_rcsum(skb);
+		nf_bridge_save_header(skb);
+
 		return br_nf_pre_routing_ipv6(hook, skb, in, out, okfn);
 	}
 #ifdef CONFIG_SYSCTL
@@ -579,6 +581,7 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 		return NF_ACCEPT;
 
 	nf_bridge_pull_encap_header_rcsum(skb);
+	nf_bridge_save_header(skb);
 
 	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
 		goto inhdr_error;
@@ -863,7 +866,6 @@ static unsigned int br_nf_post_routing(unsigned int hook, struct sk_buff *skb,
 	}
 
 	nf_bridge_pull_encap_header(skb);
-	nf_bridge_save_header(skb);
 
 	NF_HOOK(pf, NF_INET_POST_ROUTING, skb, NULL, realoutdev,
 		br_nf_dev_queue_xmit);
diff --git a/net/bridge/netfilter/ebt_dnat.c b/net/bridge/netfilter/ebt_dnat.c
index 6b49ea9..3f4acfa 100644
--- a/net/bridge/netfilter/ebt_dnat.c
+++ b/net/bridge/netfilter/ebt_dnat.c
@@ -23,6 +23,7 @@ ebt_dnat_tg(struct sk_buff *skb, const struct xt_target_param *par)
 		return EBT_DROP;
 
 	memcpy(eth_hdr(skb)->h_dest, info->mac, ETH_ALEN);
+	/* update nf_bridge info */
 	return info->target;
 }
 
diff --git a/net/bridge/netfilter/ebt_snat.c b/net/bridge/netfilter/ebt_snat.c
index 8d04d4c..edd4682 100644
--- a/net/bridge/netfilter/ebt_snat.c
+++ b/net/bridge/netfilter/ebt_snat.c
@@ -25,6 +25,7 @@ ebt_snat_tg(struct sk_buff *skb, const struct xt_target_param *par)
 		return EBT_DROP;
 
 	memcpy(eth_hdr(skb)->h_source, info->mac, ETH_ALEN);
+	/* update nf_bridge info */
 	if (!(info->target & NAT_ARP_BIT) &&
 	    eth_hdr(skb)->h_proto == htons(ETH_P_ARP)) {
 		const struct arphdr *ap;
diff --git a/net/ipv4/netfilter/ipt_REJECT.c b/net/ipv4/netfilter/ipt_REJECT.c
index c93ae44..f66a7cc 100644
--- a/net/ipv4/netfilter/ipt_REJECT.c
+++ b/net/ipv4/netfilter/ipt_REJECT.c
@@ -107,6 +107,8 @@ static void send_reset(struct sk_buff *oldskb, int hook)
 	   )
 		addr_type = RTN_LOCAL;
 
+	nskb->nf_bridge = nf_bridge_get(oldskb->nf_bridge);
+
 	/* ip_route_me_harder expects skb->dst to be set */
 	skb_dst_set(nskb, dst_clone(skb_dst(oldskb)));
 

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Bart De Schuymer]

Patrick McHardy schreef:
> Patrick McHardy wrote:
>   
>> Bart De Schuymer wrote:
>>     
>>> Patrick McHardy schreef:
>>>       
>>>> Yes, we need to save it at some point. My idea was that we might be able
>>>> to save it in PREROUTING instead of POSTROUTING and only do
>>>>
>>>> nskb->nf_bridge = nf_bridge_get(oskb->nf_bridge)
>>>>
>>>> in ipt_REJECT and probably also the ICMP code. MAC NAT could be handled
>>>> by updating the bridge info simultaneously.
>>>>
>>>>   
>>>>         
>>> The code creates a new skbuf and the correct source MAC address is lost
>>> if you don't attach it to the skbuf at that time.
>>>       
>> That's what I'm doing above.
>>
>>     
>>> How will you know in
>>> PREROUTING what SMAC to use if you didn't save it when you created the
>>> skbuf?
>>>       
>> I'm not sure I understand what you're getting at. The above
>> line of code would do exactly that, attach the nf_bridge
>> data from the original packet to the newly created one.
>> But for this to work we need to make sure its valid in all
>> hooks, hence my suggestion to save it in PREROUTING instead
>> of POSTROUTING.
>>     
>
> This patch demonstrates the idea. Its not compile tested
> and incomplete, just to make more clear what I'm suggesting.
>   
OK, that sounds good. Much cleaner. Sorry for misunderstanding you, it's 
Friday evening...
I'll look into it.

cheers,
Bart


-- 
Bart De Schuymer
www.artinalgorithms.be

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Patrick McHardy]

Bart De Schuymer wrote:
> Patrick McHardy schreef:
>>
>> This patch demonstrates the idea. Its not compile tested
>> and incomplete, just to make more clear what I'm suggesting.
>>   
> OK, that sounds good. Much cleaner. Sorry for misunderstanding you, it's
> Friday evening...
> I'll look into it.

Great, thanks.

Re: [PATCH][BRIDGE-NETFILTER] fix REJECT for bridged traffic

[Jan Engelhardt]

On Friday 2009-11-06 18:46, Patrick McHardy wrote:
>> 
>> I'm not sure I understand what you're getting at. The above
>> line of code would do exactly that, attach the nf_bridge
>> data from the original packet to the newly created one.
>> But for this to work we need to make sure its valid in all
>> hooks, hence my suggestion to save it in PREROUTING instead
>> of POSTROUTING.
>
>This patch demonstrates the idea. Its not compile tested
>and incomplete, just to make more clear what I'm suggesting.
>

[patch in non-inlined form]

For a final one you also want to modify ip6t_REJECT.