[PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Felix Fietkau]

There are broken devices in the wild that handle duplicate IP address
detection by sending out ARP requests for the IP that they received from a
DHCP server and refuse the address if they get a reply.
When proxyarp is enabled, they would go into a loop of requesting an address
and then NAKing it again.

Link: https://github.com/openwrt/openwrt/issues/14309
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/bridge/br_arp_nd_proxy.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index c7869a286df4..3a2770938374 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -204,7 +204,10 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 			if ((p && (p->flags & BR_PROXYARP)) ||
 			    (f->dst && (f->dst->flags & BR_PROXYARP_WIFI)) ||
 			    br_is_neigh_suppress_enabled(f->dst, vid)) {
-				if (!vid)
+				replied = true;
+				if (!memcmp(n->ha, sha, dev->addr_len))
+					replied = false;
+				else if (!vid)
 					br_arp_send(br, p, skb->dev, sip, tip,
 						    sha, n->ha, sha, 0, 0);
 				else
@@ -212,7 +215,6 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 						    sha, n->ha, sha,
 						    skb->vlan_proto,
 						    skb_vlan_tag_get(skb));
-				replied = true;
 			}
 
 			/* If we have replied or as long as we know the
-- 
2.43.0

Re: [PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Paolo Abeni]

On Thu, 2024-01-04 at 15:25 +0100, Felix Fietkau wrote:
> There are broken devices in the wild that handle duplicate IP address
> detection by sending out ARP requests for the IP that they received from a
> DHCP server and refuse the address if they get a reply.
> When proxyarp is enabled, they would go into a loop of requesting an address
> and then NAKing it again.

Can you instead provide the same functionality with some nft/tc
ingress/ebpf filter?

I feel uneasy to hard code this kind of policy, even if it looks
sensible. I suspect it could break some other currently working weird
device behavior.

Otherwise it could be nice provide some arpfilter flag to
enable/disable this kind filtering.

Cheers,

Paolo

Re: [PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Felix Fietkau]

On 09.01.24 12:36, Paolo Abeni wrote:
> On Thu, 2024-01-04 at 15:25 +0100, Felix Fietkau wrote:
>> There are broken devices in the wild that handle duplicate IP address
>> detection by sending out ARP requests for the IP that they received from a
>> DHCP server and refuse the address if they get a reply.
>> When proxyarp is enabled, they would go into a loop of requesting an address
>> and then NAKing it again.
> 
> Can you instead provide the same functionality with some nft/tc
> ingress/ebpf filter?
> 
> I feel uneasy to hard code this kind of policy, even if it looks
> sensible. I suspect it could break some other currently working weird
> device behavior.
> 
> Otherwise it could be nice provide some arpfilter flag to
> enable/disable this kind filtering.

I don't see how it could break anything, because it wouldn't suppress 
non-proxied responses. nft/arpfilter is just too expensive, and I don't 
think it makes sense to force the use of tc filters to suppress 
nonsensical responses generated by the bridge layer.

- Felix

Re: [PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Nikolay Aleksandrov]

On 09/01/2024 13:58, Felix Fietkau wrote:
> On 09.01.24 12:36, Paolo Abeni wrote:
>> On Thu, 2024-01-04 at 15:25 +0100, Felix Fietkau wrote:
>>> There are broken devices in the wild that handle duplicate IP address
>>> detection by sending out ARP requests for the IP that they received from a
>>> DHCP server and refuse the address if they get a reply.
>>> When proxyarp is enabled, they would go into a loop of requesting an address
>>> and then NAKing it again.
>>
>> Can you instead provide the same functionality with some nft/tc
>> ingress/ebpf filter?
>>
>> I feel uneasy to hard code this kind of policy, even if it looks
>> sensible. I suspect it could break some other currently working weird
>> device behavior.
>>
>> Otherwise it could be nice provide some arpfilter flag to
>> enable/disable this kind filtering.
> 
> I don't see how it could break anything, because it wouldn't suppress non-proxied responses. nft/arpfilter is just too expensive, and I don't think it makes sense to force the use of tc filters to suppress nonsensical responses generated by the bridge layer.
> 
> - Felix
> 

I also share Paolo's concerns, and I don't think such specific policy
should be hardcoded in the bridge. It can already be achieved via tc/nft/ebpf
as mentioned. Also please CC bridge maintainers for bridge patches, I saw this
one because of Paolo's earlier reply.

Thanks,
 Nik

Re: [PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Felix Fietkau]

On 09.01.24 13:02, Nikolay Aleksandrov wrote:
> On 09/01/2024 13:58, Felix Fietkau wrote:
>> On 09.01.24 12:36, Paolo Abeni wrote:
>>> On Thu, 2024-01-04 at 15:25 +0100, Felix Fietkau wrote:
>>>> There are broken devices in the wild that handle duplicate IP address
>>>> detection by sending out ARP requests for the IP that they received from a
>>>> DHCP server and refuse the address if they get a reply.
>>>> When proxyarp is enabled, they would go into a loop of requesting an address
>>>> and then NAKing it again.
>>>
>>> Can you instead provide the same functionality with some nft/tc
>>> ingress/ebpf filter?
>>>
>>> I feel uneasy to hard code this kind of policy, even if it looks
>>> sensible. I suspect it could break some other currently working weird
>>> device behavior.
>>>
>>> Otherwise it could be nice provide some arpfilter flag to
>>> enable/disable this kind filtering.
>> 
>> I don't see how it could break anything, because it wouldn't suppress non-proxied responses. nft/arpfilter is just too expensive, and I don't think it makes sense to force the use of tc filters to suppress nonsensical responses generated by the bridge layer.
>> 
>> - Felix
>> 
> 
> I also share Paolo's concerns, and I don't think such specific policy
> should be hardcoded in the bridge. It can already be achieved via tc/nft/ebpf
> as mentioned. Also please CC bridge maintainers for bridge patches, I saw this
> one because of Paolo's earlier reply.

Why is this 'specific policy'? I'm not changing the bridge to filter 
quirky ARP responses generated elsewhere. I'm simply changing the bridge 
code to avoid generating nonsensical ARP responses by itself.

Also, I can't replicate the exact behavior with a nft/tc filter, because 
a filter can't differentiate between forwarded ARP responses and bogus 
fake responses generated by the bridge code itself.

The concern regarding breaking existing devices also makes no sense to 
me at all. From my perspective, proxyarp is an optimization that you 
should be able to turn on without breaking breaking existing devices.
The fact that the current code violates that expectation is something 
I'm trying to fix.

- Felix

Re: [PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Paolo Abeni]

On Tue, 2024-01-09 at 12:58 +0100, Felix Fietkau wrote:
> On 09.01.24 12:36, Paolo Abeni wrote:
> > On Thu, 2024-01-04 at 15:25 +0100, Felix Fietkau wrote:
> > > There are broken devices in the wild that handle duplicate IP address
> > > detection by sending out ARP requests for the IP that they received from a
> > > DHCP server and refuse the address if they get a reply.
> > > When proxyarp is enabled, they would go into a loop of requesting an address
> > > and then NAKing it again.
> > 
> > Can you instead provide the same functionality with some nft/tc
> > ingress/ebpf filter?
> > 
> > I feel uneasy to hard code this kind of policy, even if it looks
> > sensible. I suspect it could break some other currently working weird
> > device behavior.
> > 
> > Otherwise it could be nice provide some arpfilter flag to
> > enable/disable this kind filtering.
> 
> I don't see how it could break anything, 

FTR, I don't either. But I've been surprised too much times from
extremely weird expectations from random devices, broken by "obviously
correct" behaviors change.

> because it wouldn't suppress 
> non-proxied responses. nft/arpfilter is just too expensive, and I don't 
> think it makes sense to force the use of tc filters to suppress 
> nonsensical responses generated by the bridge layer.

Then what about adding a flag to enable/disable this new behavior?

Cheers,

Paolo

Re: [PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Nikolay Aleksandrov]

On 09/01/2024 14:14, Paolo Abeni wrote:
> On Tue, 2024-01-09 at 12:58 +0100, Felix Fietkau wrote:
>> On 09.01.24 12:36, Paolo Abeni wrote:
>>> On Thu, 2024-01-04 at 15:25 +0100, Felix Fietkau wrote:
>>>> There are broken devices in the wild that handle duplicate IP address
>>>> detection by sending out ARP requests for the IP that they received from a
>>>> DHCP server and refuse the address if they get a reply.
>>>> When proxyarp is enabled, they would go into a loop of requesting an address
>>>> and then NAKing it again.
>>>
>>> Can you instead provide the same functionality with some nft/tc
>>> ingress/ebpf filter?
>>>
>>> I feel uneasy to hard code this kind of policy, even if it looks
>>> sensible. I suspect it could break some other currently working weird
>>> device behavior.
>>>
>>> Otherwise it could be nice provide some arpfilter flag to
>>> enable/disable this kind filtering.
>>
>> I don't see how it could break anything, 
> 
> FTR, I don't either. But I've been surprised too much times from
> extremely weird expectations from random devices, broken by "obviously
> correct" behaviors change.
> 
>> because it wouldn't suppress 
>> non-proxied responses. nft/arpfilter is just too expensive, and I don't 
>> think it makes sense to force the use of tc filters to suppress 
>> nonsensical responses generated by the bridge layer.
> 
> Then what about adding a flag to enable/disable this new behavior?
> 

If you decide to go down this path consider using bridge's boolopts. Personally
I still prefer to avoid adding such policies in the bridge, but instead to implement
them via other means, it's not a hard "no" so an option with default to current
behaviour would be acceptable.

> Cheers,
> 
> Paolo
> 

Re: [PATCH net-next] net: bridge: do not send arp replies if src and target hw addr is the same

[Dave Taht]

On Thu, Jan 4, 2024 at 9:54 AM Felix Fietkau <nbd@nbd.name> wrote:
>
> There are broken devices in the wild that handle duplicate IP address
> detection by sending out ARP requests for the IP that they received from a
> DHCP server and refuse the address if they get a reply.
> When proxyarp is enabled, they would go into a loop of requesting an address
> and then NAKing it again.
>
> Link: https://github.com/openwrt/openwrt/issues/14309
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  net/bridge/br_arp_nd_proxy.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
> index c7869a286df4..3a2770938374 100644
> --- a/net/bridge/br_arp_nd_proxy.c
> +++ b/net/bridge/br_arp_nd_proxy.c
> @@ -204,7 +204,10 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
>                         if ((p && (p->flags & BR_PROXYARP)) ||
>                             (f->dst && (f->dst->flags & BR_PROXYARP_WIFI)) ||
>                             br_is_neigh_suppress_enabled(f->dst, vid)) {
> -                               if (!vid)
> +                               replied = true;
> +                               if (!memcmp(n->ha, sha, dev->addr_len))
> +                                       replied = false;
> +                               else if (!vid)
>                                         br_arp_send(br, p, skb->dev, sip, tip,
>                                                     sha, n->ha, sha, 0, 0);
>                                 else
> @@ -212,7 +215,6 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
>                                                     sha, n->ha, sha,
>                                                     skb->vlan_proto,
>                                                     skb_vlan_tag_get(skb));
> -                               replied = true;
>                         }
>
>                         /* If we have replied or as long as we know the
> --
> 2.43.0
>
>

Acked-by: Dave Taht <dave.taht@gmail.com>

-- 
40 years of net history, a couple songs:
https://www.youtube.com/watch?v=D9RGX6QFm5E
Dave Täht CSO, LibreQos