Re: [PATCH net] openvswitch: vport: fix race between tunnel creation and linking

[Ilya Maximets <i.maximets@ovn.org>]

On 4/30/26 11:32 PM, Ilya Maximets wrote:
> When a tunnel vport is created it first creates the tunnel device, e.g.,
> with geneve_dev_create_fb(), then it calls ovs_netdev_link() to take a
> reference and link it to the device that represents openvswitch datapath.
> 
> The creation of the device is happening under RTNL, but then RTNL is
> released and re-acquired to find the device by name.  It is technically
> possible for the tunnel device to be re-named or deleted within that
> window while RTNL is not held, and some other device created in its
> place.  This will cause a non-tunnel device to be referenced in the
> vport and tunnel-specific functions used on it, e.g. vxlan_get_options()
> that directly casts the private netdev data into a struct vxlan_dev
> causing an invalid memory access:
> 
>  BUG: KASAN: slab-use-after-free in vxlan_get_options+0x323/0x3a0
>   vxlan_get_options+0x323/0x3a0
>   ovs_vport_cmd_new+0x6e3/0xd30
> 
> Fix that by taking a reference to the just created device before
> releasing RTNL.  This ensures that the device in the vport is always
> the one that was just created.  The search by name is only needed
> for a standard vport-netdev that links pre-existing devices, so that
> functionality and device type checks are moved to netdev_create().
> 
> It is also awkward that ovs_netdev_link() takes ownership of the vport
> and destroys it on failure.  It doesn't know the type of the port it is
> dealing with, so we need to pass down the indicator that it's a tunnel,
> so the link can be properly deleted on failure.
> 
> It's possible to refactor the logic to make the ovs_netdev_link() do
> only the linking part and let the callers perform a proper destruction,
> but it will be much more code for each legacy tunnel port type, so it
> is not worth it for the bug fix.
> 
> Fixes: 614732eaa12d ("openvswitch: Use regular VXLAN net_device device")
> Reported-by: Yuan Tan <tanyuan98@outlook.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Reported-by: Yang Yang <n05ec@lzu.edu.cn>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  net/openvswitch/vport-geneve.c |  5 ++-
>  net/openvswitch/vport-gre.c    |  5 ++-
>  net/openvswitch/vport-netdev.c | 58 ++++++++++++++++++++--------------
>  net/openvswitch/vport-netdev.h |  2 +-
>  net/openvswitch/vport-vxlan.c  |  5 ++-
>  5 files changed, 48 insertions(+), 27 deletions(-)
> 
...
> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
> index 12055af832dc0..a92ca8b37f96a 100644
> --- a/net/openvswitch/vport-netdev.c
> +++ b/net/openvswitch/vport-netdev.c
> @@ -73,37 +73,21 @@ static struct net_device *get_dpdev(const struct datapath *dp)
>  	return local->dev;
>  }
>  
> -struct vport *ovs_netdev_link(struct vport *vport, const char *name)
> +struct vport *ovs_netdev_link(struct vport *vport, bool tunnel)
>  {
>  	int err;
>  
> -	vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name);
> -	if (!vport->dev) {
> +	if (WARN_ON_ONCE(!vport->dev)) {
>  		err = -ENODEV;
>  		goto error_free_vport;
>  	}
> -	/* Ensure that the device exists and that the provided
> -	 * name is not one of its aliases.
> -	 */
> -	if (strcmp(name, ovs_vport_name(vport))) {
> -		err = -ENODEV;
> -		goto error_put;
> -	}
> -	netdev_tracker_alloc(vport->dev, &vport->dev_tracker, GFP_KERNEL);
> -	if (vport->dev->flags & IFF_LOOPBACK ||
> -	    (vport->dev->type != ARPHRD_ETHER &&
> -	     vport->dev->type != ARPHRD_NONE) ||
> -	    ovs_is_internal_dev(vport->dev)) {
> -		err = -EINVAL;
> -		goto error_put;
> -	}
>  
>  	rtnl_lock();
>  	err = netdev_master_upper_dev_link(vport->dev,
>  					   get_dpdev(vport->dp),
>  					   NULL, NULL, NULL);
>  	if (err)
> -		goto error_unlock;
> +		goto error_put_unlock;
>  
>  	err = netdev_rx_handler_register(vport->dev, netdev_frame_hook,
>  					 vport);

Sashiko-gemini reports that here we could be linking an already unregistering
device since we're not checking the registration status after re-acquiring the
lock.  Which seems like an issue, which is related, but fairly separate from
what this patch is trying to fix.  It is also not specific to the tunnel ports.
So, should be addressed separately.

Best regards, Ilya Maximets.