[PATCH] bridge: Remove BR_PROXYARP flooding check code

[PATCH] bridge: Remove BR_PROXYARP flooding check code

[Jouni Malinen]

From: Kyeyoon Park <kyeyoonp@codeaurora.org>

Because dropping broadcast packets for IEEE 802.11 Proxy ARP is more
selective than previously thought, it is better to remove the direct
dropping logic in the bridge code in favor of using the netfilter
infrastructure to provide more control on which frames get dropped. This
code was added in commit 958501163ddd ("bridge: Add support for IEEE
802.11 Proxy ARP").

Signed-off-by: Kyeyoon Park <kyeyoonp@codeaurora.org>
Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
---
 net/bridge/br_forward.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index f96933a..8a025a7 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -185,10 +185,6 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb,
 		if (unicast && !(p->flags & BR_FLOOD))
 			continue;
 
-		/* Do not flood to ports that enable proxy ARP */
-		if (p->flags & BR_PROXYARP)
-			continue;
-
 		prev = maybe_deliver(prev, p, skb, __packet_hook);
 		if (IS_ERR(prev))
 			goto out;
-- 
1.9.1

Re: [PATCH] bridge: Remove BR_PROXYARP flooding check code

[Stephen Hemminger]

On Mon,  8 Dec 2014 17:27:40 +0200
Jouni Malinen <jouni@codeaurora.org> wrote:

> From: Kyeyoon Park <kyeyoonp@codeaurora.org>
> 
> Because dropping broadcast packets for IEEE 802.11 Proxy ARP is more
> selective than previously thought, it is better to remove the direct
> dropping logic in the bridge code in favor of using the netfilter
> infrastructure to provide more control on which frames get dropped. This
> code was added in commit 958501163ddd ("bridge: Add support for IEEE
> 802.11 Proxy ARP").
> 
> Signed-off-by: Kyeyoon Park <kyeyoonp@codeaurora.org>
> Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
> ---
>  net/bridge/br_forward.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index f96933a..8a025a7 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -185,10 +185,6 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  		if (unicast && !(p->flags & BR_FLOOD))
>  			continue;
>  
> -		/* Do not flood to ports that enable proxy ARP */
> -		if (p->flags & BR_PROXYARP)
> -			continue;
> -
>  		prev = maybe_deliver(prev, p, skb, __packet_hook);
>  		if (IS_ERR(prev))
>  			goto out;

Aren't you at risk of duplicate ARP responses in some cases.
You can't assume user will run netfilter.

Re: [PATCH] bridge: Remove BR_PROXYARP flooding check code

[Jouni Malinen]

On Tue, Dec 09, 2014 at 02:21:58PM -0800, Stephen Hemminger wrote:
> On Mon,  8 Dec 2014 17:27:40 +0200
> Jouni Malinen <jouni@codeaurora.org> wrote:
> > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > @@ -185,10 +185,6 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb,
> >  		if (unicast && !(p->flags & BR_FLOOD))
> >  			continue;
> >  
> > -		/* Do not flood to ports that enable proxy ARP */
> > -		if (p->flags & BR_PROXYARP)
> > -			continue;
> > -
> >  		prev = maybe_deliver(prev, p, skb, __packet_hook);

> Aren't you at risk of duplicate ARP responses in some cases.
> You can't assume user will run netfilter.

This is only for the case where BR_PROXYARP has been enabled by the
user, but yes, it would be convenient to handle cases better without
requiring netfilter and skip flooding to BR_PROXYARP port more
selectively here. Would there be some convenient means for
br_do_proxy_arp() to mark the skb that it has replied to in br_input.c
and then use that here in br_forward.c to not flood an ARP request that
has already been replied to? Or should this simply skip flooding of all
ARP packets with something like following?

 		if (unicast && !(p->flags & BR_FLOOD))
 			continue;
 
-		/* Do not flood to ports that enable proxy ARP */
-		if (p->flags & BR_PROXYARP)
+		/* Do not flood ARP to ports that enable proxy ARP */
+		if (p->flags & BR_PROXYARP &&
+		    skb->protocol == htons(ETH_P_ARP))
 			continue;
 
 		prev = maybe_deliver(prev, p, skb, __packet_hook);

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] bridge: Remove BR_PROXYARP flooding check code

[Jouni Malinen]

On Tue, Dec 09, 2014 at 02:21:58PM -0800, Stephen Hemminger wrote:
> On Mon,  8 Dec 2014 17:27:40 +0200
> > From: Kyeyoon Park <kyeyoonp@codeaurora.org>
> > Because dropping broadcast packets for IEEE 802.11 Proxy ARP is more
> > selective than previously thought, it is better to remove the direct
> > dropping logic in the bridge code in favor of using the netfilter
> > infrastructure to provide more control on which frames get dropped. This
> > code was added in commit 958501163ddd ("bridge: Add support for IEEE
> > 802.11 Proxy ARP").

> Aren't you at risk of duplicate ARP responses in some cases.
> You can't assume user will run netfilter.

The 'bridge: Selectively prevent bridge port flooding for proxy ARP'
patch that I posted earlier today addresses this by keeping record of
whether the proxyarp functionality has replied to a packet and skipping
flooding conditionally on that. In other words, this 'bridge: Remove
BR_PROXYARP flooding check code' can be dropped.

-- 
Jouni Malinen                                            PGP id EFC895FA