Re: [PATCH RFC net-next 1/3] vxlan: vxlan_vs_find_vni(): Find vxlan_dev according to vni and remote_ip

[Ted Chen <znscnchen@gmail.com>]

On Sun, Feb 02, 2025 at 01:56:36PM +0200, Ido Schimmel wrote:
> On Sat, Feb 01, 2025 at 07:34:00PM +0800, Ted Chen wrote:
> > vxlan_vs_find_vni() currently searches the vni hash table in a vs and
> > returns a vxlan_dev associated with the specified "vni". While this works
> > when the remote_ips are stored in the vxlan fdb, it fails to handle the
> > case where the remote_ip is just configured in the vxlan device outside of
> > the vxlan fdb, because multiple vxlan devices with different remote_ip may
> > share a single vni when the remote_ip is configured in the vxlan device
> > (i.e., not stored in the vxlan fdb). In that case, further check of
> > remote_ip to identify vxlan_dev more precisely.
> > 
> > Signed-off-by: Ted Chen <znscnchen@gmail.com>
> > ---
> >  drivers/net/vxlan/vxlan_core.c | 32 ++++++++++++++++++++++++++------
> >  1 file changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > index 05c10acb2a57..3ca74a97c44f 100644
> > --- a/drivers/net/vxlan/vxlan_core.c
> > +++ b/drivers/net/vxlan/vxlan_core.c
> > @@ -94,8 +94,10 @@ static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
> >  
> >  static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs,
> >  					   int ifindex, __be32 vni,
> > +					   const struct sk_buff *skb,
> >  					   struct vxlan_vni_node **vninode)
> >  {
> > +	union vxlan_addr saddr;
> >  	struct vxlan_vni_node *vnode;
> >  	struct vxlan_dev_node *node;
> >  
> > @@ -116,14 +118,31 @@ static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs,
> >  			continue;
> >  		}
> >  
> > -		if (IS_ENABLED(CONFIG_IPV6)) {
> > -			const struct vxlan_config *cfg = &node->vxlan->cfg;
> > +		const struct vxlan_config *cfg = &node->vxlan->cfg;
> >  
> > +		if (IS_ENABLED(CONFIG_IPV6)) {
> >  			if ((cfg->flags & VXLAN_F_IPV6_LINKLOCAL) &&
> >  			    cfg->remote_ifindex != ifindex)
> >  				continue;
> >  		}
> >  
> > +		if (vni && !vxlan_addr_any(&cfg->remote_ip) &&
> > +		    !vxlan_addr_multicast(&cfg->remote_ip)) {
> > +			/* 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;
> > +				saddr.sa.sa_family = AF_INET;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +			} else {
> > +				saddr.sin6.sin6_addr = ipv6_hdr(skb)->saddr;
> > +				saddr.sa.sa_family = AF_INET6;
> > +#endif
> > +			}
> > +
> > +			if (!vxlan_addr_equal(&cfg->remote_ip, &saddr))
> > +				continue;
> 
> This breaks existing behavior. Before this patch, a VXLAN device with a
> remote address could receive traffic from any VTEP (in the same
> broadcast domain).
> 
> I think this patch misinterprets the "remote" keyword as P2P when it is
> not the case. It is merely the VTEP to which packets are sent when no
Yes. Thanks for pointing that out. 
I didn't see target addresses were appended into the FDB when an unicast
remote_ip had been configured.

e.g.
Usually when (2)(3) are invoked, (1) is not called to configure a unicast
remote_ip to the VTEP (though it's allowed to call (1)).

(1) ip link add vxlan42 type vxlan id 42 \
                local 10.0.0.1 remote 10.0.0.2 dstport 4789
(2) bridge fdb append to 00:00:00:00:00:00 dst 10.0.0.3 dev vxlan42
(3) bridge fdb append to 00:00:00:00:00:00 dst 10.0.0.4 dev vxlan42

So, this patch just leverages the case when remote_ip is configured in the
VTEP to stand for P2P.

Do you think there's a better way to identify P2P more precisely?

> other VTEP was found in the FDB. A VXLAN device that was configured with
> the "remote" keyword can still send packets to other VTEPs and it should
> therefore be able to receive packets from them.
> 
> > +		}
> > +
> >  		if (vninode)
> >  			*vninode = vnode;
> >  		return node->vxlan;
> > @@ -134,6 +153,7 @@ static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs,
> >  
> >  /* Look up VNI in a per net namespace table */
> >  static struct vxlan_dev *vxlan_find_vni(struct net *net, int ifindex,
> > +					const struct sk_buff *skb,
> >  					__be32 vni, sa_family_t family,
> >  					__be16 port, u32 flags)
> >  {
> > @@ -143,7 +163,7 @@ static struct vxlan_dev *vxlan_find_vni(struct net *net, int ifindex,
> >  	if (!vs)
> >  		return NULL;
> >  
> > -	return vxlan_vs_find_vni(vs, ifindex, vni, NULL);
> > +	return vxlan_vs_find_vni(vs, ifindex, vni, skb, NULL);
> >  }
> >  
> >  /* Fill in neighbour message in skbuff. */
> > @@ -1701,7 +1721,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >  
> >  	vni = vxlan_vni(vh->vx_vni);
> >  
> > -	vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode);
> > +	vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, skb, &vninode);
> >  	if (!vxlan) {
> >  		reason = SKB_DROP_REASON_VXLAN_VNI_NOT_FOUND;
> >  		goto drop;
> > @@ -1855,7 +1875,7 @@ static int vxlan_err_lookup(struct sock *sk, struct sk_buff *skb)
> >  		return -ENOENT;
> >  
> >  	vni = vxlan_vni(hdr->vx_vni);
> > -	vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, NULL);
> > +	vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, skb, NULL);
> >  	if (!vxlan)
> >  		return -ENOENT;
> >  
> > @@ -2330,7 +2350,7 @@ static int encap_bypass_if_local(struct sk_buff *skb, struct net_device *dev,
> >  		struct vxlan_dev *dst_vxlan;
> >  
> >  		dst_release(dst);
> > -		dst_vxlan = vxlan_find_vni(vxlan->net, dst_ifindex, vni,
> > +		dst_vxlan = vxlan_find_vni(vxlan->net, dst_ifindex, skb, vni,
> >  					   addr_family, dst_port,
> >  					   vxlan->cfg.flags);
> >  		if (!dst_vxlan) {
> > -- 
> > 2.39.2
> > 
> >