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). 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. Fix this introducing reference counting for struct udp_tunnel_nic. Increment the refcount on successful queue_work(), and decrement it at the end of the worker. Defer the dev_put() call for the last device to the free path to ensure the net_device remains valid as long as the structure is alive. Additionally, convert concurrent modifications of the 'missed' bitmap to atomic operations (set_bit, bitmap_zero) to prevent data races there. Fixes: cc4e3835eff4 ("udp_tunnel: add central NIC RX port offload infrastructure") Reported-by: Yue Sun Closes: https://lore.kernel.org/netdev/20260624090135.95763-1-samsun1006219@gmail.com/ Signed-off-by: Eric Dumazet --- net/ipv4/udp_tunnel_nic.c | 75 +++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c index 9944ed923ddfd10f9adf6ad788c0740daeaf2adb..884b5d93b7b39f7f20855ff8ca2ec4d7ef5a9ef6 100644 --- a/net/ipv4/udp_tunnel_nic.c +++ b/net/ipv4/udp_tunnel_nic.c @@ -30,9 +30,8 @@ struct udp_tunnel_nic_table_entry { * @work: async work for talking to hardware from process context * @dev: netdev pointer * @lock: protects all fields - * @need_sync: at least one port start changed - * @need_replay: space was freed, we need a replay of all ports - * @work_pending: @work is currently scheduled + * @flags: sync, replay flags + * @refcnt: reference count * @n_tables: number of tables under @entries * @missed: bitmap of tables which overflown * @entries: table of tables of ports currently offloaded @@ -44,9 +43,11 @@ struct udp_tunnel_nic { struct mutex lock; - u8 need_sync:1; - u8 need_replay:1; - u8 work_pending:1; + unsigned long flags; +#define UDP_TUNNEL_NIC_NEED_SYNC 0 +#define UDP_TUNNEL_NIC_NEED_REPLAY 1 + + refcount_t refcnt; unsigned int n_tables; unsigned long missed; @@ -116,7 +117,7 @@ udp_tunnel_nic_entry_queue(struct udp_tunnel_nic *utn, unsigned int flag) { entry->flags |= flag; - utn->need_sync = 1; + set_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags); } static void @@ -283,7 +284,7 @@ udp_tunnel_nic_device_sync_by_table(struct net_device *dev, static void __udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn) { - if (!utn->need_sync) + if (!test_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags)) return; if (dev->udp_tunnel_nic_info->sync_table) @@ -291,21 +292,24 @@ __udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn) else udp_tunnel_nic_device_sync_by_port(dev, utn); - utn->need_sync = 0; + clear_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags); /* Can't replay directly here, in case we come from the tunnel driver's * notification - trying to replay may deadlock inside tunnel driver. */ - utn->need_replay = udp_tunnel_nic_should_replay(dev, utn); + if (udp_tunnel_nic_should_replay(dev, utn)) + set_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags); + else + clear_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags); } static void udp_tunnel_nic_device_sync(struct net_device *dev, struct udp_tunnel_nic *utn) { - if (!utn->need_sync) + if (!test_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags)) return; - queue_work(udp_tunnel_nic_workqueue, &utn->work); - utn->work_pending = 1; + if (queue_work(udp_tunnel_nic_workqueue, &utn->work)) + refcount_inc(&utn->refcnt); } static bool @@ -348,7 +352,7 @@ udp_tunnel_nic_has_collision(struct net_device *dev, struct udp_tunnel_nic *utn, if (!udp_tunnel_nic_entry_is_free(entry) && entry->port == ti->port && entry->type != ti->type) { - __set_bit(i, &utn->missed); + set_bit(i, &utn->missed); return true; } } @@ -483,7 +487,7 @@ udp_tunnel_nic_add_new(struct net_device *dev, struct udp_tunnel_nic *utn, * are no devices currently which have multiple tables accepting * the same tunnel type, and false positives are okay. */ - __set_bit(i, &utn->missed); + set_bit(i, &utn->missed); } return false; @@ -552,7 +556,7 @@ static void __udp_tunnel_nic_reset_ntf(struct net_device *dev) mutex_lock(&utn->lock); - utn->need_sync = false; + clear_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags); for (i = 0; i < utn->n_tables; i++) for (j = 0; j < info->tables[i].n_entries; j++) { struct udp_tunnel_nic_table_entry *entry; @@ -696,8 +700,8 @@ udp_tunnel_nic_flush(struct net_device *dev, struct udp_tunnel_nic *utn) for (i = 0; i < utn->n_tables; i++) memset(utn->entries[i], 0, array_size(info->tables[i].n_entries, sizeof(**utn->entries))); - WARN_ON(utn->need_sync); - utn->need_replay = 0; + WARN_ON(test_bit(UDP_TUNNEL_NIC_NEED_SYNC, &utn->flags)); + clear_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags); } static void @@ -713,8 +717,8 @@ udp_tunnel_nic_replay(struct net_device *dev, struct udp_tunnel_nic *utn) for (i = 0; i < utn->n_tables; i++) for (j = 0; j < info->tables[i].n_entries; j++) udp_tunnel_nic_entry_freeze_used(&utn->entries[i][j]); - utn->missed = 0; - utn->need_replay = 0; + bitmap_zero(&utn->missed, UDP_TUNNEL_NIC_MAX_TABLES); + clear_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags); if (!info->shared) { udp_tunnel_get_rx_info(dev); @@ -728,6 +732,8 @@ udp_tunnel_nic_replay(struct net_device *dev, struct udp_tunnel_nic *utn) udp_tunnel_nic_entry_unfreeze(&utn->entries[i][j]); } +static void udp_tunnel_nic_put(struct udp_tunnel_nic *utn); + static void udp_tunnel_nic_device_sync_work(struct work_struct *work) { struct udp_tunnel_nic *utn = @@ -736,14 +742,15 @@ static void udp_tunnel_nic_device_sync_work(struct work_struct *work) rtnl_lock(); mutex_lock(&utn->lock); - utn->work_pending = 0; __udp_tunnel_nic_device_sync(utn->dev, utn); - if (utn->need_replay) + if (test_bit(UDP_TUNNEL_NIC_NEED_REPLAY, &utn->flags)) udp_tunnel_nic_replay(utn->dev, utn); mutex_unlock(&utn->lock); rtnl_unlock(); + + udp_tunnel_nic_put(utn); } static struct udp_tunnel_nic * @@ -759,6 +766,7 @@ udp_tunnel_nic_alloc(const struct udp_tunnel_nic_info *info, utn->n_tables = n_tables; INIT_WORK(&utn->work, udp_tunnel_nic_device_sync_work); mutex_init(&utn->lock); + refcount_set(&utn->refcnt, 1); for (i = 0; i < n_tables; i++) { utn->entries[i] = kzalloc_objs(*utn->entries[i], @@ -782,9 +790,19 @@ static void udp_tunnel_nic_free(struct udp_tunnel_nic *utn) for (i = 0; i < utn->n_tables; i++) kfree(utn->entries[i]); + + if (utn->dev) + dev_put(utn->dev); + kfree(utn); } +static void udp_tunnel_nic_put(struct udp_tunnel_nic *utn) +{ + if (refcount_dec_and_test(&utn->refcnt)) + udp_tunnel_nic_free(utn); +} + static int udp_tunnel_nic_register(struct net_device *dev) { const struct udp_tunnel_nic_info *info = dev->udp_tunnel_nic_info; @@ -863,6 +881,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 last = true; udp_tunnel_nic_lock(dev); @@ -889,6 +908,7 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn) udp_tunnel_drop_rx_info(dev); utn->dev = first->dev; udp_tunnel_nic_unlock(dev); + last = false; goto release_dev; } @@ -901,16 +921,11 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn) udp_tunnel_nic_flush(dev, utn); 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) - return; - - udp_tunnel_nic_free(utn); + udp_tunnel_nic_put(utn); release_dev: dev->udp_tunnel_nic = NULL; - dev_put(dev); + if (!last) + dev_put(dev); } static int -- 2.55.0.rc0.799.gd6f94ed593-goog