| Message ID | 20240104142501.81092-1-nbd@nbd.name (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: bridge: do not send arp replies if src and target hw addr is the same | expand |
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
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
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
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
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 >
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
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>
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
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(-)