| Message ID | 20240531154137.26797-1-daniel@iogearbox.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] vxlan: Fix regression when dropping packets due to invalid src addresses | expand |
On Fri, May 31, 2024 at 05:41:37PM +0200, Daniel Borkmann wrote: > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c > index f78dd0438843..7353f27b02dc 100644 > --- a/drivers/net/vxlan/vxlan_core.c > +++ b/drivers/net/vxlan/vxlan_core.c > @@ -1605,6 +1605,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, > struct vxlan_sock *vs, > struct sk_buff *skb, __be32 vni) > { > + bool learning = vxlan->cfg.flags & VXLAN_F_LEARN; > union vxlan_addr saddr; > u32 ifindex = skb->dev->ifindex; > > @@ -1616,8 +1617,11 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, > if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr)) > return false; > > - /* Ignore packets from invalid src-address */ > - if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) > + /* Ignore packets from invalid src-address when in learning mode, > + * otherwise let them through e.g. when originating from NOARP > + * devices with all-zero mac, etc. > + */ > + if (learning && !is_valid_ether_addr(eth_hdr(skb)->h_source)) > return false; > > /* Get address from the outer IP header */ > @@ -1631,7 +1635,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, > #endif > } > > - if ((vxlan->cfg.flags & VXLAN_F_LEARN) && > + if (learning && > vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni)) > return false; Daniel, I think we can simply move this check out of the main path to vxlan_snoop() which is only called when learning is enabled: diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c index 7496c14e8329..89f3945b448f 100644 --- a/drivers/net/vxlan/vxlan_core.c +++ b/drivers/net/vxlan/vxlan_core.c @@ -1446,6 +1446,10 @@ static bool vxlan_snoop(struct net_device *dev, struct vxlan_fdb *f; u32 ifindex = 0; + /* Ignore packets from invalid src-address */ + if (!is_valid_ether_addr(src_mac)) + return true; + #if IS_ENABLED(CONFIG_IPV6) if (src_ip->sa.sa_family == AF_INET6 && (ipv6_addr_type(&src_ip->sin6.sin6_addr) & IPV6_ADDR_LINKLOCAL)) @@ -1616,10 +1620,6 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr)) return false; - /* Ignore packets from invalid src-address */ - if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) - return false; - /* Get address from the outer IP header */ if (vxlan_get_sk_family(vs) == AF_INET) { saddr.sin.sin_addr.s_addr = ip_hdr(skb)->saddr; WDYT?
> Commit f58f45c1e5b9 ("vxlan: drop packets from invalid src-address") has > been recently added to vxlan mainly in the context of source address > snooping/learning so that when it is enabled, an entry in the FDB is not being > created for an invalid address for the tunnel endpoint. > > Before commit f58f45c1e5b9 vxlan was similarly behaving as geneve in that it > passed through whichever macs were set in the L2 header. It turns out that > this change in behavior breaks setups, for example, Cilium with netkit in L3 > mode for Pods as well as tunnel mode has been passing before the change in > f58f45c1e5b9 for both vxlan and geneve. > After mentioned change it is only passing for geneve as in case of vxlan > packets are dropped due to vxlan_set_mac() returning false as source and > destination macs are zero which for E/W traffic via tunnel is totally fine. > > Fix it by only opting into the is_valid_ether_addr() check in > vxlan_set_mac() when in fact source address snooping/learning is actually > enabled in vxlan. With this change, the Cilium connectivity test suite passes > again for both tunnel flavors. > > Fixes: f58f45c1e5b9 ("vxlan: drop packets from invalid src-address") > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: David Bauer <mail@david-bauer.net> > Cc: Ido Schimmel <idosch@nvidia.com> > Cc: Nikolay Aleksandrov <razor@blackwall.org> > Cc: Martin KaFai Lau <martin.lau@kernel.org> > --- > drivers/net/vxlan/vxlan_core.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c > index f78dd0438843..7353f27b02dc 100644 > --- a/drivers/net/vxlan/vxlan_core.c > +++ b/drivers/net/vxlan/vxlan_core.c > @@ -1605,6 +1605,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, > struct vxlan_sock *vs, > struct sk_buff *skb, __be32 vni) > { > + bool learning = vxlan->cfg.flags & VXLAN_F_LEARN; > union vxlan_addr saddr; > u32 ifindex = skb->dev->ifindex; > Not related to this change, can you adjust existing declaration align to reverse X-mas tree? Thanks, Hariprasad k > @@ -1616,8 +1617,11 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, > if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev- > >dev_addr)) > return false; > > - /* Ignore packets from invalid src-address */ > - if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) > + /* Ignore packets from invalid src-address when in learning mode, > + * otherwise let them through e.g. when originating from NOARP > + * devices with all-zero mac, etc. > + */ > + if (learning && !is_valid_ether_addr(eth_hdr(skb)->h_source)) > return false; > > /* Get address from the outer IP header */ @@ -1631,7 +1635,7 @@ > static bool vxlan_set_mac(struct vxlan_dev *vxlan, #endif > } > > - if ((vxlan->cfg.flags & VXLAN_F_LEARN) && > + if (learning && > vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, > vni)) > return false; > > -- > 2.34.1 >
On 6/2/24 10:37 AM, Ido Schimmel wrote: > On Fri, May 31, 2024 at 05:41:37PM +0200, Daniel Borkmann wrote: >> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c >> index f78dd0438843..7353f27b02dc 100644 >> --- a/drivers/net/vxlan/vxlan_core.c >> +++ b/drivers/net/vxlan/vxlan_core.c >> @@ -1605,6 +1605,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, >> struct vxlan_sock *vs, >> struct sk_buff *skb, __be32 vni) >> { >> + bool learning = vxlan->cfg.flags & VXLAN_F_LEARN; >> union vxlan_addr saddr; >> u32 ifindex = skb->dev->ifindex; >> >> @@ -1616,8 +1617,11 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, >> if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr)) >> return false; >> >> - /* Ignore packets from invalid src-address */ >> - if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) >> + /* Ignore packets from invalid src-address when in learning mode, >> + * otherwise let them through e.g. when originating from NOARP >> + * devices with all-zero mac, etc. >> + */ >> + if (learning && !is_valid_ether_addr(eth_hdr(skb)->h_source)) >> return false; >> >> /* Get address from the outer IP header */ >> @@ -1631,7 +1635,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, >> #endif >> } >> >> - if ((vxlan->cfg.flags & VXLAN_F_LEARN) && >> + if (learning && >> vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni)) >> return false; > > Daniel, I think we can simply move this check out of the main path to > vxlan_snoop() which is only called when learning is enabled: > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c > index 7496c14e8329..89f3945b448f 100644 > --- a/drivers/net/vxlan/vxlan_core.c > +++ b/drivers/net/vxlan/vxlan_core.c > @@ -1446,6 +1446,10 @@ static bool vxlan_snoop(struct net_device *dev, > struct vxlan_fdb *f; > u32 ifindex = 0; > > + /* Ignore packets from invalid src-address */ > + if (!is_valid_ether_addr(src_mac)) > + return true; > + > #if IS_ENABLED(CONFIG_IPV6) > if (src_ip->sa.sa_family == AF_INET6 && > (ipv6_addr_type(&src_ip->sin6.sin6_addr) & IPV6_ADDR_LINKLOCAL)) > @@ -1616,10 +1620,6 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, > if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr)) > return false; > > - /* Ignore packets from invalid src-address */ > - if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) > - return false; > - > /* Get address from the outer IP header */ > if (vxlan_get_sk_family(vs) == AF_INET) { > saddr.sin.sin_addr.s_addr = ip_hdr(skb)->saddr; > > WDYT? Makes sense, that looks better. Will send a v2, thanks Ido!
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c index f78dd0438843..7353f27b02dc 100644 --- a/drivers/net/vxlan/vxlan_core.c +++ b/drivers/net/vxlan/vxlan_core.c @@ -1605,6 +1605,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, struct vxlan_sock *vs, struct sk_buff *skb, __be32 vni) { + bool learning = vxlan->cfg.flags & VXLAN_F_LEARN; union vxlan_addr saddr; u32 ifindex = skb->dev->ifindex; @@ -1616,8 +1617,11 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr)) return false; - /* Ignore packets from invalid src-address */ - if (!is_valid_ether_addr(eth_hdr(skb)->h_source)) + /* Ignore packets from invalid src-address when in learning mode, + * otherwise let them through e.g. when originating from NOARP + * devices with all-zero mac, etc. + */ + if (learning && !is_valid_ether_addr(eth_hdr(skb)->h_source)) return false; /* Get address from the outer IP header */ @@ -1631,7 +1635,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan, #endif } - if ((vxlan->cfg.flags & VXLAN_F_LEARN) && + if (learning && vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni)) return false;
Commit f58f45c1e5b9 ("vxlan: drop packets from invalid src-address") has been recently added to vxlan mainly in the context of source address snooping/learning so that when it is enabled, an entry in the FDB is not being created for an invalid address for the tunnel endpoint. Before commit f58f45c1e5b9 vxlan was similarly behaving as geneve in that it passed through whichever macs were set in the L2 header. It turns out that this change in behavior breaks setups, for example, Cilium with netkit in L3 mode for Pods as well as tunnel mode has been passing before the change in f58f45c1e5b9 for both vxlan and geneve. After mentioned change it is only passing for geneve as in case of vxlan packets are dropped due to vxlan_set_mac() returning false as source and destination macs are zero which for E/W traffic via tunnel is totally fine. Fix it by only opting into the is_valid_ether_addr() check in vxlan_set_mac() when in fact source address snooping/learning is actually enabled in vxlan. With this change, the Cilium connectivity test suite passes again for both tunnel flavors. Fixes: f58f45c1e5b9 ("vxlan: drop packets from invalid src-address") Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Cc: David Bauer <mail@david-bauer.net> Cc: Ido Schimmel <idosch@nvidia.com> Cc: Nikolay Aleksandrov <razor@blackwall.org> Cc: Martin KaFai Lau <martin.lau@kernel.org> --- drivers/net/vxlan/vxlan_core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)