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

[Bart De Schuymer <bdschuym@pandora.be>]

[-- 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 */