Re: [PATCH net] net: udp_tunnel: fix use-after-free by refcounting udp_tunnel_nic
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-06-24 21:57:24
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Wed, 24 Jun 2026 17:10:34 +0000 Eric Dumazet wrote:
Yue Sun reported a use-after-free and debugobjects warning in udp_tunnel_nic_device_sync_work() during concurrent device operations. The state flags of struct udp_tunnel_nic were originally bitfields sharing a byte, modified concurrently without locking (RCU vs worker).
Can you clarify the path where the bits are modified without locks??
My mental model is that this is basically all under rtnl_lock, and
Stan added _another_ lock so that drivers can call "sync" / reply
without needing rtnl lock, but any changes are still under rtnl_lock.
The gap seems to be that we don't check pending under Stan's new lock,
since commit 1ead7501094c6 ("udp_tunnel: remove rtnl_lock dependency")
did:
+++ b/drivers/net/netdevsim/udp_tunnels.c@@ -112,12 +112,10 @@ nsim_udp_tunnels_info_reset_write(struct file *file, const char __user *data, struct net_device *dev = file->private_data; struct netdevsim *ns = netdev_priv(dev); - rtnl_lock(); if (dev->reg_state == NETREG_REGISTERED) { memset(ns->udp_ports.ports, 0, sizeof(ns->udp_ports.__ports)); udp_tunnel_nic_reset_ntf(dev); } - rtnl_unlock();
so we just need:
diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
index 9944ed923ddf..d7db89a222f8 100644
--- a/net/ipv4/udp_tunnel_nic.c
+++ b/net/ipv4/udp_tunnel_nic.c@@ -863,6 +863,7 @@ static void udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn) { const struct udp_tunnel_nic_info *info = dev->udp_tunnel_nic_info; + bool pending; udp_tunnel_nic_lock(dev);
@@ -899,12 +900,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn) * from the work which we will boot immediately. */ udp_tunnel_nic_flush(dev, utn); + + pending = utn->work_pending; udp_tunnel_nic_unlock(dev); /* Wait for the work to be done using the state, netdev core will * retry unregister until we give up our reference on this device. */ - if (utn->work_pending) + if (pending) return; udp_tunnel_nic_free(utn);
Even after converting to atomic bitops, a single WORK_PENDING flag races: the workqueue core clears the pending bit before running the worker. A concurrent queueing sets the flag, but the running worker clears it, leading to premature freeing in unregister() while the re-queued work is still active.
+ if (utn->dev) + dev_put(utn->dev);
nit: cocci complains that null check is not needed here.