Re: [PATCH v2 net-next 6/8] vxlan: convert configuration to RCU protection

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The changelog of "vxlan: convert configuration to RCU
  protection" presents the patch as pure groundwork ("In order to allow…

--

vxlan: convert configuration to RCU protection

This converts vxlan->cfg into an __rcu pointer, switches readers to
rcu_dereference()/rtnl_dereference(), publishes new configurations with
rcu_assign_pointer() and frees the old one with kfree_rcu().  Callers in
mlxsw and the vxlan mdb/multicast/vnifilter code are updated accordingly.

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 5bc3475886b09..b79a15985a994 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c

[ ... ]

> @@ -1704,7 +1704,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  		goto drop;
>  	}
>  
> -	cfg = vxlan->cfg;
> +	cfg = rcu_dereference(vxlan->cfg);
>  
>  	if (vh->vx_flags & cfg->reserved_bits.vx_flags ||
>  	    vh->vx_vni & cfg->reserved_bits.vx_vni) {

[ ... ]

> @@ -2787,7 +2787,8 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>  	u32 nhid = 0;
>  	u32 flags;
>  
> -	cfg = vxlan->cfg;
> +	rcu_read_lock();
> +	cfg = rcu_dereference(vxlan->cfg);
>  	flags = cfg->flags;
>  	default_vni = cfg->vni;
>  	saddr_family = cfg->saddr.sa.sa_family;
> @@ -2806,14 +2807,19 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>  				vxlan_xmit_one(skb, dev, cfg, vni, NULL, false);
>  			else
>  				kfree_skb_reason(skb, SKB_DROP_REASON_TUNNEL_TXINFO);
> +			rcu_read_unlock();
>  			return NETDEV_TX_OK;
>  		}
>  	}
>  
>  	if (flags & VXLAN_F_PROXY) {
>  		eth = eth_hdr(skb);
> -		if (ntohs(eth->h_proto) == ETH_P_ARP)
> -			return arp_reduce(dev, skb, cfg, vni);
> +		if (ntohs(eth->h_proto) == ETH_P_ARP) {
> +			netdev_tx_t res = arp_reduce(dev, skb, cfg, vni);
> +
> +			rcu_read_unlock();
> +			return res;
> +		}

[ ... ]

> @@ -4040,9 +4051,10 @@ static void vxlan_config_apply(struct net_device *dev,
>  	needed_headroom += vxlan_headroom(flags);
>  	dev->needed_headroom = needed_headroom;
>  
> -	old_cfg = vxlan->cfg;
> -	vxlan->cfg = new_cfg;
> -	kfree(old_cfg);
> +	old_cfg = rtnl_dereference(vxlan->cfg);
> +	rcu_assign_pointer(vxlan->cfg, new_cfg);
> +	if (old_cfg)
> +		kfree_rcu(old_cfg, rcu);
>  }

[Severity: Medium]
Could the changelog mention that this patch also closes a use-after-free
window opened by the previous patch in the series, "vxlan: dynamically
allocate struct vxlan_config" (ae42529dbdc6)?

At that parent commit, vxlan_config_apply() frees the old configuration
immediately:

	old_cfg = vxlan->cfg;
	vxlan->cfg = new_cfg;
	kfree(old_cfg);

and there is no synchronize_net()/synchronize_rcu() anywhere in
vxlan_core.c at that revision, while softirq readers hold a plain pointer
to the object being freed.  vxlan_xmit() reads it before taking any RCU
read lock:

	cfg = vxlan->cfg;
	flags = cfg->flags;
	default_vni = cfg->vni;
	saddr_family = cfg->saddr.sa.sa_family;

and then passes cfg into arp_reduce()/neigh_reduce()/vxlan_xmit_nhid()/
vxlan_xmit_one().  vxlan_rcv() does the same and dereferences
cfg->reserved_bits.vx_flags.

So with the parent commit alone, running "ip link set <vxlan> type vxlan
..." (vxlan_changelink() -> vxlan_config_apply(), RTNL only) while traffic
flows frees the config under concurrent TX/RX softirq readers.  Only this
patch makes that safe, via kfree_rcu(old_cfg, rcu) plus the reader-side
rcu_dereference()/rcu_read_lock().

Would it make sense to either use kfree_rcu() already in the previous
patch, squash the two, or at least note in the changelog that the two must
not be reordered, split or partially backported, since a bisect landing on
the previous commit hits a real use-after-free in the TX/RX paths?  No
Fixes: tag looks needed, as struct vxlan_config is still embedded in struct
vxlan_dev at the series baseline, so no released kernel is affected.

Separately, could the changelog also mention the vxlan_xmit()
restructuring?  A single rcu_read_lock() now spans the whole function and
five new rcu_read_unlock() calls are added on the metadata, ARP proxy, ND
proxy, nhid and MDB early return paths, which is a hot path change readers
currently have to infer from the diff.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906180111.1973188-1-edumazet%40google.com