Re: [PATCH net-next 7/9] 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] Commit message mischaracterizes a memory-safety fix as
  preparatory refactoring, leaving a transient use-after-free in the…

--

vxlan: convert configuration to RCU protection

This converts vxlan->cfg into an __rcu pointer, switches the readers to
rcu_dereference()/rtnl_dereference() and publishes new configurations with
rcu_assign_pointer() while freeing the old one via kfree_rcu().  The TX path
in vxlan_xmit() also grows an rcu_read_lock() section covering all of its
exit paths.

> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index af320173a1e7..c49d55de020f 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) {

[ ... ]

> @@ -2781,7 +2781,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;

[ ... ]

> @@ -4034,9 +4045,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]
Is this change really just preparation, or is it the patch that closes a
use-after-free opened by the preceding patch in the series?

The changelog says:

    In order to allow lockless readers in future patches, convert 'vxlan->cfg'
    to an RCU protected pointer.

but the lockless readers seem to exist already.  At the parent commit
5a2b432e8977 ("vxlan: dynamically allocate struct vxlan_config"),
vxlan_config_apply() does:

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

with no grace period, while vxlan_xmit() (softirq) does a plain
cfg = vxlan->cfg; and hands that pointer down to vxlan_xmit_one(),
arp_reduce(), neigh_reduce() and vxlan_xmit_nhid(), and vxlan_rcv() (UDP
encap softirq) does the same before dereferencing cfg->reserved_bits.
vxlan_vs_find_vni(), the vxlan_cleanup() age timer and
vxlan_fill_metadata_dst() read it the same way.

So at that intermediate commit:

CPU0 (RTNL, RTM_NEWLINK changelink)
  vxlan_changelink()
    vxlan_config_apply()
      kfree(old_cfg);

CPU1 (softirq)
  vxlan_xmit()
    cfg = vxlan->cfg;        /* may already be the freed old_cfg */
    flags = cfg->flags;

Can an "ip link set ... type vxlan ..." concurrent with traffic free the
config under a live TX or RX reader at that commit?

If so, would it make sense to squash the two patches, or at least say in
this changelog that the previous patch opens that window and this one closes
it, so a partial backport or a bisect does not land on the unsafe commit?

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