[PATCH v2 1/3] bridge: Base the BR_PROXYARP decision on the target port flag

[PATCH v2 1/3] bridge: Base the BR_PROXYARP decision on the target port flag

[Jouni Malinen]

From: Kyeyoon Park <kyeyoonp@codeaurora.org>

When doing proxy ARP, instead of checking the bridge port flag for
BR_PROXYARP on the bridge port on which the frame was received, check
the bridge port flag of the bridge port to which the target device
belongs.

Signed-off-by: Kyeyoon Park <kyeyoonp@codeaurora.org>
Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
---
v2: Address Stephen's comment on mixing && and & without parens
    and check for f->dst != NULL before dereferencing it

 net/bridge/br_input.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index e2aa7be..b2afa17 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -105,7 +105,7 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct net_bridge *br,
 		}
 
 		f = __br_fdb_get(br, n->ha, vid);
-		if (f)
+		if (f && f->dst && (f->dst->flags & BR_PROXYARP))
 			arp_send(ARPOP_REPLY, ETH_P_ARP, sip, skb->dev, tip,
 				 sha, n->ha, sha);
 
@@ -155,7 +155,6 @@ int br_handle_frame_finish(struct sk_buff *skb)
 
 	if (is_broadcast_ether_addr(dest)) {
 		if (IS_ENABLED(CONFIG_INET) &&
-		    p->flags & BR_PROXYARP &&
 		    skb->protocol == htons(ETH_P_ARP))
 			br_do_proxy_arp(skb, br, vid);
 
-- 
1.9.1

[PATCH v2 2/3] bridge: Selectively prevent bridge port flooding for proxy ARP

[Jouni Malinen]

From: Kyeyoon Park <kyeyoonp@codeaurora.org>

Rather than completely blocking the bridge port flooding when
BR_PROXYARP is enabled, selectively prevent bridge port flooding for the
proxy ARP relevant frames. This is done by marking the "skb" with this
info when proxy ARP code executes.

Signed-off-by: Kyeyoon Park <kyeyoonp@codeaurora.org>
Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
---
v2: Address Stephen's comment on mixing && and & without parens

 net/bridge/br_forward.c | 3 ++-
 net/bridge/br_input.c   | 6 +++++-
 net/bridge/br_private.h | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index f96933a..0b156aa 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -186,7 +186,8 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb,
 			continue;
 
 		/* Do not flood to ports that enable proxy ARP */
-		if (p->flags & BR_PROXYARP)
+		if ((p->flags & BR_PROXYARP) &&
+		    BR_INPUT_SKB_CB(skb)->proxyarp_replied)
 			continue;
 
 		prev = maybe_deliver(prev, p, skb, __packet_hook);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index b2afa17..c3640d8 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -68,6 +68,8 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct net_bridge *br,
 	u8 *arpptr, *sha;
 	__be32 sip, tip;
 
+	BR_INPUT_SKB_CB(skb)->proxyarp_replied = false;
+
 	if (dev->flags & IFF_NOARP)
 		return;
 
@@ -105,9 +107,11 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct net_bridge *br,
 		}
 
 		f = __br_fdb_get(br, n->ha, vid);
-		if (f && f->dst && (f->dst->flags & BR_PROXYARP))
+		if (f && f->dst && (f->dst->flags & BR_PROXYARP)) {
 			arp_send(ARPOP_REPLY, ETH_P_ARP, sip, skb->dev, tip,
 				 sha, n->ha, sha);
+			BR_INPUT_SKB_CB(skb)->proxyarp_replied = true;
+		}
 
 		neigh_release(n);
 	}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index de09199..c32e279 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -305,6 +305,7 @@ struct br_input_skb_cb {
 #endif
 
 	u16 frag_max_size;
+	bool proxyarp_replied;
 
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 	bool vlan_filtered;
-- 
1.9.1

[PATCH v2 3/3] bridge: Allow proxy ARP for unicast ARP requests

[Jouni Malinen]

From: Kyeyoon Park <kyeyoonp@codeaurora.org>

It is possible for unicast ARP requests to be used and the same proxyarp
mechanism should work for them as well as for the broadcast case.

Signed-off-by: Kyeyoon Park <kyeyoonp@codeaurora.org>
Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
---
v2: rebased, but identical to v1

 net/bridge/br_input.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index c3640d8..41d7cfa 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -157,11 +157,11 @@ int br_handle_frame_finish(struct sk_buff *skb)
 
 	dst = NULL;
 
-	if (is_broadcast_ether_addr(dest)) {
-		if (IS_ENABLED(CONFIG_INET) &&
-		    skb->protocol == htons(ETH_P_ARP))
-			br_do_proxy_arp(skb, br, vid);
+	if (IS_ENABLED(CONFIG_INET) &&
+	    skb->protocol == htons(ETH_P_ARP))
+		br_do_proxy_arp(skb, br, vid);
 
+	if (is_broadcast_ether_addr(dest)) {
 		skb2 = skb;
 		unicast = false;
 	} else if (is_multicast_ether_addr(dest)) {
-- 
1.9.1

Re: [PATCH v2 1/3] bridge: Base the BR_PROXYARP decision on the target port flag

[David Miller]

From: Jouni Malinen <jouni@codeaurora.org>
Date: Thu,  5 Feb 2015 11:54:21 +0200

> From: Kyeyoon Park <kyeyoonp@codeaurora.org>
> 
> When doing proxy ARP, instead of checking the bridge port flag for
> BR_PROXYARP on the bridge port on which the frame was received, check
> the bridge port flag of the bridge port to which the target device
> belongs.
> 
> Signed-off-by: Kyeyoon Park <kyeyoonp@codeaurora.org>
> Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
> ---
> v2: Address Stephen's comment on mixing && and & without parens
>     and check for f->dst != NULL before dereferencing it

This is a semantic change, what if the administrator wanted the
current behavior?

I'm not applying this.  If you want the new behavior, it will have
to be at a minimum turned on via a sysctl that defaults to OFF.

Re: [PATCH v2 1/3] bridge: Base the BR_PROXYARP decision on the target port flag

[Jouni Malinen]

On Sat, Feb 07, 2015 at 09:59:22PM -0800, David Miller wrote:
> From: Jouni Malinen <jouni@codeaurora.org>
> > When doing proxy ARP, instead of checking the bridge port flag for
> > BR_PROXYARP on the bridge port on which the frame was received, check
> > the bridge port flag of the bridge port to which the target device
> > belongs.
> 
> This is a semantic change, what if the administrator wanted the
> current behavior?
> 
> I'm not applying this.  If you want the new behavior, it will have
> to be at a minimum turned on via a sysctl that defaults to OFF.

Agreed, this does change the semantics of the commit 958501163ddd
("bridge: Add support for IEEE 802.11 Proxy ARP"). However, I see this
as a bug fix rather than new behavior. In other words, this set of three
patches came up from extended testing coverage and issues found during
implementation of automated protocol tests. Taken into account that this
BR_PROXYARP functionality was added recently and I'm not aware of a use
case that would use the current design, I was hoping to avoid the added
complexity of configuration parameters for something that I do not
expect to be needed.

This IEEE 802.11 Proxy ARP functionality is still work in progress in
the sense that the IPv6 patch has not yet been contributed (RFC version
would be ready, but I'm waiting for the IPv4 side to get fully
completed).


If a configuration parameter is required for this, what would the
preferred way of adding it in net/bridge? It looks like only
br_netfilter.c is currently using sysctl. Would netlink with another
BRPORT_ATTR_FLAG similarly to BR_PROXYARP be a suitable alternative?

-- 
Jouni Malinen                                            PGP id EFC895FA