ibmveth: fix hangs, use-after-frees and netpoll races
v2 cover ibmveth: fix hangs, use-after-frees and netpoll races
Hi,
Eight fixes for serious bugs in the ibmveth driver: two hang the system, four are use-after-frees, one corrupts memory, and one makes ethtool -L report success when it failed.
The series is based on net-next cfb7793d1bc0 ("Merge branch 'net-lan966x-add-support-for-pcie-fdma'"). It does not depend on the two open() error-path fixes recently applied to net (af0524bf4ce1, 84bec0bf0352) and merges cleanly with them; patch 2 explains how it relates to them. It also applies to net with git am -3.
1. Netpoll races with RX replenish (memory corruption, NULL dereference): remove ndo_poll_controller, as was done for ibmvnic, skip RX replenish in netpoll's budget-0 poll, and enable NAPI only once the RX resources exist.
2. Hang after a failed internal reopen: the next close() waits in napi_disable() forever with RTNL held, and only a reboot recovers. Skip close() when open() did not succeed.
3. Use-after-free in remove(): a reset queued from NAPI could run on the freed adapter. Disable the reset work first.
4. RX poll hang on a bad correlator: poll restarts on the same slot until RCU stalls, and an inactive pool dereferences NULL. Step past the slot, reject inactive pools, count the drop.
5. Use-after-free after a failed probe: the pool kobjects stay in sysfs after the adapter is freed. Put them, as remove() does, and give them a release() that probe and remove() wait for, so CONFIG_DEBUG_KOBJECT_RELEASE cannot free them early either.
6. ethtool -L returns 0 when it cannot allocate the new TX queues. Return the allocation error.
7. Use-after-free of TX buffers: close() frees them without waiting for a running transmit. It is called directly for MTU, offload and buffer pool changes, and through dev_close(), which does not wait either with a noqueue qdisc. Use netif_tx_disable().
8. Use-after-free of the RX queue: napi_disable() returns before the poll has finished, and the rest of the poll re-enables the interrupt and reads the RX queue that close() then frees. Wait with synchronize_net().
The triggers are rare and there are no field reports, so the series targets net-next. All carry Fixes: tags; I am happy to repost against net, or add Cc: stable, if you prefer.
All were found by AI-assisted review of the ibmveth multi-queue RX series [1]. Landing them first also shrinks that series, which then only extends this handling per queue.
Testing: the new and extended KUnit cases in patch 4 fail on the unfixed driver and pass on qemu pseries (ppc64le). On a POWER10
LPAR: netconsole under printk and ping floods, MTU and buffer pool changes under traffic, a forced open() failure followed by down and up, unbind/bind under traffic, a forced register_netdev() failure in probe, ethtool -L with a forced TX buffer allocation failure, and rapid link down/up and MTU cycles under a ping flood for patch 8. The forced failures used test-only module parameters that are not part of this series.
Changes in v2:
From the Sashiko review of v1:
- Patch 5: give the pool kobjects a release() and wait for it before free_netdev() in probe and remove(), which closes the CONFIG_DEBUG_KOBJECT_RELEASE window.
- New patch 8: wait for the poll to return before close() frees the RX queue (raised on patch 1 as a pre-existing bug).
Other small changes:
- Patch 1: also skip RX replenish in netpoll's budget-0 poll.
- Patch 4: count rx_dropped when recycling an invalid buffer fails.
- Commit messages: say how each bug was found and tested, with small clarifications in patches 2 and 7.
Rebased on current net-next.
v1: https://lore.kernel.org/netdev/[email protected]/
[1] https://lore.kernel.org/netdev/[email protected]/
Thanks,
Mingming
Mingming Cao (8):
ibmveth: fix netpoll races with RX replenish
ibmveth: do not close twice after a failed reopen
ibmveth: disable the reset work before unregister in remove
ibmveth: step past bad RX correlators instead of spinning or oopsing
ibmveth: release the pool kobjects when probe fails
ibmveth: return the error when set_channels cannot add TX queues
ibmveth: wait for in-flight transmits in ibmveth_close()
ibmveth: wait for the RX poll to return before freeing the RX queue
drivers/net/ethernet/ibm/ibmveth.c | 294 +++++++++++++++++++++++------ drivers/net/ethernet/ibm/ibmveth.h | 5 + 2 files changed, 237 insertions(+), 62 deletions(-)
Signature · 1 line
2.39.3 (Apple Git-146)
PATCH v2 1/8 ibmveth: fix netpoll races with RX replenish
ibmveth_poll_controller() runs RX replenish outside NAPI and without a lock, racing NAPI's replenish on another CPU. Both can fill the same slot, so an skb and its DMA mapping leak and PHYP can write into an unmapped buffer. netpoll calls it from netconsole and from netpoll-enabled bonds.
ibmveth_open() also enables NAPI before the RX resources exist. ibmveth_change_mtu(), veth_pool_store(), ibmveth_set_csum_offload() and ibmveth_set_tso() call close() and open() directly, so while open() is still setting up, netpoll and the direct ibmveth_interrupt() calls can replenish NULL pools and read freed memory.
Remove the callback, as Eric Dumazet did for many drivers after commit ac3d9dd034e5 ("netpoll: make ndo_poll_controller() optional"), including ibmvnic in commit 0c3b9d1b37df ("ibmvnic: remove ndo_poll_controller"). netpoll then polls NAPI itself with budget 0. napi->poll_owner serializes that with NAPI, but not with a NAPI poll that was already running when netpoll was set up, so skip RX replenish at budget 0, which netpoll uses for TX only. TX completes synchronously, so ibmveth_poll() has nothing else to do for netpoll.
Enable NAPI just before request_irq(), once everything ibmveth_poll() touches exists.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection of poll_one_napi() and the direct close()/open() callers; neither race was reproduced. Tested on a POWER10 LPAR with netconsole over ibmveth: a ping flood (678,470 packets, no loss) during a printk flood, and MTU changes and buffer pool toggles under traffic, with no warnings. No kernel selftests cover ibmveth.
---
Changes in v2:
- skip RX replenish when ibmveth_poll() runs with budget 0:
napi->poll_owner does not serialize netpoll with a NAPI poll that was already running when netpoll was set up
drivers/net/ethernet/ibm/ibmveth.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)
drivers/net/ethernet/ibm/ibmveth.c +13 -15
@@ -623,8 +623,6 @@ static int ibmveth_open(struct net_device *netdev)netdev_dbg (netdev,"open starting\n" ); -napi_enable (&adapter->napi); -for (i =0 ; i <IBMVETH_NUM_BUFF_POOLS ; i++) rxq_entries += adapter->rx_buff_pool[i].size;
@@ -712,10 +710,18 @@ static int ibmveth_open(struct net_device *netdev) } } +/* NAPI can run as soon as it is enabled, from netpoll during the +* direct close()/open() pairs or from a direct ibmveth_interrupt() +* call, so enable it only once everything ibmveth_poll() touches +* exists. +*/ +napi_enable (&adapter->napi); +netdev_dbg (netdev,"registering irq 0x%x\n" , netdev->irq); rc =request_irq (netdev->irq, ibmveth_interrupt,0 , netdev->name, netdev);if (rc !=0 ) { +napi_disable (&adapter->napi);netdev_err (netdev,"unable to request irq 0x%x, rc %d\n" , netdev->irq, rc); do {
@@ -763,7 +769,6 @@ static int ibmveth_open(struct net_device *netdev) out_free_buffer_list:free_page ((unsigned long )adapter->buffer_list_addr); out: -napi_disable (&adapter->napi);return rc; }
@@ -1540,7 +1545,11 @@ static int ibmveth_poll(struct napi_struct *napi, int budget) } } -ibmveth_replenish_task (adapter); +/* netpoll polls with budget 0 for TX only, and is not serialized +* with a NAPI poll that was already running when it was set up +*/ +if (budget) +ibmveth_replenish_task (adapter);if (frames_processed == budget) goto out;
@@ -1680,14 +1689,6 @@ static int ibmveth_change_mtu(struct net_device *dev, int new_mtu)return -EINVAL ; } -#ifdef CONFIG_NET_POLL_CONTROLLER -static void ibmveth_poll_controller (struct net_device *dev) -{ -ibmveth_replenish_task (netdev_priv (dev)); -ibmveth_interrupt (dev->irq, dev); -} -#endif -/** * ibmveth_get_desired_dma - Calculate IO memory desired by the driver *
@@ -1789,9 +1790,6 @@ static const struct net_device_ops ibmveth_netdev_ops = { .ndo_validate_addr = eth_validate_addr, .ndo_set_mac_address = ibmveth_set_mac_addr, .ndo_features_check = ibmveth_features_check, -#ifdef CONFIG_NET_POLL_CONTROLLER - .ndo_poll_controller = ibmveth_poll_controller, -#endif };static int ibmveth_probe (struct vio_dev *dev,const struct vio_device_id *id)
--
2.39.3 (Apple Git-146)
PATCH v2 2/8 ibmveth: do not close twice after a failed reopen
In reply to Mingming Caoibmveth_change_mtu(), veth_pool_store(), ibmveth_set_csum_offload() and ibmveth_set_tso() call close() and open() directly. If open() fails, NAPI is left disabled while IFF_UP stays set, so the next close() (ifdown, unregister or another reconfiguration) calls napi_disable() again and waits forever with RTNL held. Networking and shutdown hang; only a reboot recovers. ethtool -L in that state also wakes queues that have no TX buffer and dereferences NULL in ibmveth_start_xmit().
Any open() failure on those paths triggers it, for example an allocation failure on an MTU change to jumbo frames.
Track a successful open in adapter->opened. close() returns early when it is clear, and set_channels() checks it instead of IFF_UP.
The open() error-path leaks are fixed separately in net by commit af0524bf4ce1 ("ibmveth: h_free logical LAN on open-fail after register") and commit 84bec0bf0352 ("ibmveth: fix TX LTB and filter unwind on open-fail"); this patch does not depend on them. Without them, the TX buffers and the logical LAN registration that a failed open() leaves behind stay in place after the early return, as they did before this patch.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection. Tested on a POWER10 LPAR with ibmveth_open() forced to fail by a test-only module parameter (not part of this patch): 'ip link set dev eth1 mtu 9000' fails, then 'ip link set dev eth1 down' returns at once and 'ip link set dev eth1 up' recovers the interface. No kernel selftests cover ibmveth.
---
Changes in v2:
- commit message: say what a failed open() leaves behind without the two net fixes
drivers/net/ethernet/ibm/ibmveth.c | 24 +++++++++++++++++------- drivers/net/ethernet/ibm/ibmveth.h | 2 ++ 2 files changed, 19 insertions(+), 7 deletions(-)
drivers/net/ethernet/ibm/ibmveth.c +17 -7
@@ -738,6 +738,7 @@ static int ibmveth_open(struct net_device *netdev)netif_tx_start_all_queues (netdev); + adapter->opened = true;netdev_dbg (netdev,"open complete\n" );return 0 ;
@@ -779,6 +780,14 @@ static int ibmveth_close(struct net_device *netdev)long lpar_rc;int i; +/* change_mtu, pool sysfs, set_csum and set_tso call close() and +* open() directly. If that open() fails, IFF_UP stays set and +* NAPI is disabled; a second close() would hang in napi_disable(). +*/ +if (!adapter->opened) +return 0 ; + adapter->opened = false; +netdev_dbg (netdev,"close starting\n" );napi_disable (&adapter->napi);
@@ -830,10 +839,10 @@ static int ibmveth_close(struct net_device *netdev)* * @w: pointer to work_struct embedded in adapter structure * -* Context: This routine acquires rtnl_mutex and disables its NAPI through -* ibmveth_close. It can't be called directly in a context that has -* already acquired rtnl_mutex or disabled its NAPI, or directly from -* a poll routine. +* Context: This routine acquires rtnl_mutex and, if the device is open, +* disables its NAPI through ibmveth_close. It can't be called +* directly in a context that has already acquired rtnl_mutex or +* disabled its NAPI, or directly from a poll routine. * * Return: void */
@@ -1127,10 +1136,11 @@ static int ibmveth_set_channels(struct net_device *netdev, goal = channels->tx_count;int rc, i; -/* If ndo_open has not been called yet then don't allocate, just set -* desired netdev_queue's and return +/* If the device is not open (including a failed close/open with +* IFF_UP still set) then don't allocate, just set desired +* netdev_queue's and return */ -if (!(netdev->flags &IFF_UP )) +if (!adapter->opened)return netif_set_real_num_tx_queues (netdev, goal);/* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
drivers/net/ethernet/ibm/ibmveth.h +2 -0
@@ -172,6 +172,8 @@ struct ibmveth_adapter {int rx_csum;int large_send;bool is_active_trunk; +/* Set by a successful ibmveth_open(), cleared by ibmveth_close(). */ +bool opened;unsigned int rx_buffers_per_hcall;u64 fw_ipv6_csum_support;
--
2.39.3 (Apple Git-146)
PATCH v2 3/8 ibmveth: disable the reset work before unregister in remove
In reply to Mingming Caoibmveth_remove() cancels the reset work before unregister_netdev(), but NAPI keeps running until unregister closes the device and can queue the reset again, on an interrupt enable failure, a bad free_map entry or a bad RX slot. The reset can then run on the adapter after free_netdev(), or reopen the device during unregister.
Use disable_work_sync(), which waits for a running reset and keeps the work from being queued again. A bad RX slot also makes poll spin, which can stall unregister; a later patch in this series fixes that.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection; it was not reproduced. Tested on a POWER10 LPAR with unbind and bind cycles under traffic. No kernel selftests cover ibmveth.
--- drivers/net/ethernet/ibm/ibmveth.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/ibm/ibmveth.c +1 -1
@@ -1954,7 +1954,7 @@ static void ibmveth_remove(struct vio_dev *dev)struct ibmveth_adapter *adapter =netdev_priv (netdev);int i; -cancel_work_sync (&adapter->work); +disable_work_sync (&adapter->work);for (i =0 ; i <IBMVETH_NUM_BUFF_POOLS ; i++)kobject_put (&adapter->rx_buff_pool[i].kobj);
Signature · 1 line
2.39.3 (Apple Git-146)
PATCH v2 4/8 ibmveth: step past bad RX correlators instead of spinning or oopsing
In reply to Mingming Caoibmveth_poll() mishandles a bad RX correlator from PHYP in two ways.
If harvest fails or ibmveth_rxq_get_buffer() returns NULL, poll breaks out without advancing the ring and restarts on the same slot forever. For an out-of-range correlator it also fires a WARN_ON() and schedules a reset each pass, but the reset is queued on that CPU and never runs, and the CPU stalls RCU, so RTNL holders hang too. This dates from the first commit in Fixes:, which turned the BUG_ON()s here into WARN_ON() plus a reset.
The range check also accepts a correlator naming an inactive buffer pool (pools 2 and 3 by default), whose skbuff array is NULL, so the lookup dereferences NULL in softirq. Pools became inactive with the second commit in Fixes:.
Validate the correlator in one helper that also rejects a pool with no skbuff array. On a bad slot, advance the ring, count the drop in rx_dropped and still schedule the reset, which rebuilds the pools. The correlator comes from PHYP, and with the ring advancing a burst of bad slots would WARN once per slot (and panic with panic_on_warn), so use a ratelimited netdev_err() instead. Also free the rx_copybreak skb if harvest fails there.
Add a KUnit case for harvest advancing on errors and extend the existing cases to an inactive pool; on the unfixed driver the first fails and the others oops.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection and the KUnit cases above. Hitting either bug needs PHYP to return a bad correlator, so neither was reproduced on hardware. Tested with KUnit on qemu pseries (ppc64le), and on a POWER10 LPAR with a ping flood and MTU changes under traffic. No kernel selftests cover ibmveth.
---
Changes in v2:
- count rx_dropped when recycling an invalid buffer fails
drivers/net/ethernet/ibm/ibmveth.c | 175 ++++++++++++++++++++++++----- 1 file changed, 146 insertions(+), 29 deletions(-)
drivers/net/ethernet/ibm/ibmveth.c +146 -29
@@ -443,6 +443,37 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter, } } +/* The correlator comes back from PHYP; a bad one schedules a reset. */ +static bool ibmveth_rxq_correlator_valid (struct ibmveth_adapter *adapter, +u64 correlator) +{ +unsigned int index = correlator &0xffffffffUL ; +unsigned int pool = correlator >>32 ; + +/* An inactive pool keeps its size but has no skbuff array. */ +if (pool <IBMVETH_NUM_BUFF_POOLS && + index < adapter->rx_buff_pool[pool].size && + adapter->rx_buff_pool[pool].skbuff) +return true; + +if (net_ratelimit ()) +netdev_err (adapter->netdev, +"invalid RX correlator %llx, resetting\n" , + correlator); +schedule_work (&adapter->work); +return false; +} + +static void ibmveth_rxq_no_skb (struct ibmveth_adapter *adapter, +u64 correlator) +{ +if (net_ratelimit ()) +netdev_err (adapter->netdev, +"no buffer for RX correlator %llx, resetting\n" , + correlator); +schedule_work (&adapter->work); +} +/** * ibmveth_remove_buffer_from_pool - remove a buffer from a pool * @adapter: adapter instance
@@ -451,7 +482,8 @@ static void ibmveth_free_buffer_pool(struct ibmveth_adapter *adapter,* * Return: * * %0 - success -* * %-EINVAL - correlator maps to pool or index out of range +* * %-EINVAL - correlator maps to pool or index out of range, or to an +* inactive pool * * %-EFAULT - pool and index map to null skb */ static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
@@ -462,15 +494,12 @@ static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,unsigned int free_index;struct sk_buff *skb; -if (WARN_ON (pool >=IBMVETH_NUM_BUFF_POOLS ) || -WARN_ON (index >= adapter->rx_buff_pool[pool].size)) { -schedule_work (&adapter->work); +if (!ibmveth_rxq_correlator_valid (adapter, correlator))return -EINVAL ; - } skb = adapter->rx_buff_pool[pool].skbuff[index]; -if (WARN_ON (!skb)) { -schedule_work (&adapter->work); +if (!skb) { +ibmveth_rxq_no_skb (adapter, correlator);return -EFAULT ; }
@@ -510,14 +539,23 @@ static inline struct sk_buff *ibmveth_rxq_get_buffer(struct ibmveth_adapter *adau64 correlator = adapter->rx_queue.queue_addr[adapter->rx_queue.index].correlator;unsigned int pool = correlator >>32 ;unsigned int index = correlator &0xffffffffUL ; +struct sk_buff *skb; -if (WARN_ON (pool >=IBMVETH_NUM_BUFF_POOLS ) || -WARN_ON (index >= adapter->rx_buff_pool[pool].size)) { -schedule_work (&adapter->work); +if (!ibmveth_rxq_correlator_valid (adapter, correlator))return NULL ; - } -return adapter->rx_buff_pool[pool].skbuff[index]; + skb = adapter->rx_buff_pool[pool].skbuff[index]; +if (!skb) +ibmveth_rxq_no_skb (adapter, correlator); +return skb; +} + +static void ibmveth_rxq_advance (struct ibmveth_adapter *adapter) +{ +if (++adapter->rx_queue.index == adapter->rx_queue.num_slots) { + adapter->rx_queue.index =0 ; + adapter->rx_queue.toggle = !adapter->rx_queue.toggle; + } }/**
@@ -528,6 +566,9 @@ static inline struct sk_buff *ibmveth_rxq_get_buffer(struct ibmveth_adapter *ada* * Context: called from ibmveth_poll * +* The ring advances even on error, so poll does not return to a bad +* slot before the scheduled reset can run. +* * Return: * * %0 - success * * other - non-zero return from ibmveth_remove_buffer_from_pool
@@ -540,15 +581,9 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter, cor = adapter->rx_queue.queue_addr[adapter->rx_queue.index].correlator; rc =ibmveth_remove_buffer_from_pool (adapter, cor, reuse); -if (unlikely (rc)) -return rc; +ibmveth_rxq_advance (adapter); -if (++adapter->rx_queue.index == adapter->rx_queue.num_slots) { - adapter->rx_queue.index =0 ; - adapter->rx_queue.toggle = !adapter->rx_queue.toggle; - } - -return 0 ; +return rc; }static void ibmveth_free_tx_ltb (struct ibmveth_adapter *adapter,int idx)
@@ -1468,6 +1503,7 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)int frames_processed =0 ;unsigned long lpar_rc;u16 mss =0 ; +int rc; restart_poll:while (frames_processed < budget) {
@@ -1479,8 +1515,10 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)wmb ();/* suggested by larson1 */ adapter->rx_invalid_buffer++;netdev_dbg (netdev,"recycling invalid buffer\n" ); -if (unlikely (ibmveth_rxq_harvest_buffer (adapter, true))) +if (unlikely (ibmveth_rxq_harvest_buffer (adapter, true))) { + netdev->stats.rx_dropped++;break ; + } }else {struct sk_buff *skb, *new_skb;int length =ibmveth_rxq_frame_length (adapter);
@@ -1490,8 +1528,11 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)__sum16 iph_check =0 ; skb =ibmveth_rxq_get_buffer (adapter); -if (unlikely (!skb)) +if (unlikely (!skb)) { +ibmveth_rxq_advance (adapter); + netdev->stats.rx_dropped++;break ; + }/* if the large packet bit is set in the rx queue * descriptor, the mss will be written by PHYP eight
@@ -1515,12 +1556,19 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)if (rx_flush)ibmveth_flush_buffer (skb->data, length + offset); -if (unlikely (ibmveth_rxq_harvest_buffer (adapter, true))) + rc =ibmveth_rxq_harvest_buffer (adapter, true); +if (unlikely (rc)) { +dev_kfree_skb_any (new_skb); + netdev->stats.rx_dropped++;break ; + } skb = new_skb; }else { -if (unlikely (ibmveth_rxq_harvest_buffer (adapter, false))) + rc =ibmveth_rxq_harvest_buffer (adapter, false); +if (unlikely (rc)) { + netdev->stats.rx_dropped++;break ; + }skb_reserve (skb, offset); }
@@ -2204,8 +2252,7 @@ static void ibmveth_reset_kunit(struct work_struct *w)* @test: pointer to kunit structure * * Tests the error returns from ibmveth_remove_buffer_from_pool. -* ibmveth_remove_buffer_from_pool also calls WARN_ON, so dmesg should be -* checked to see that these warnings happened. +* Each error also logs a ratelimited netdev_err. * * Return: void */
@@ -2214,6 +2261,7 @@ static void ibmveth_remove_buffer_from_pool_test(struct kunit *test)struct ibmveth_adapter *adapter =kunit_kzalloc (test,sizeof (*adapter),GFP_KERNEL );struct ibmveth_buff_pool *pool;u64 correlator; +int ret;KUNIT_ASSERT_NOT_ERR_OR_NULL (test, adapter);
@@ -2237,6 +2285,13 @@ static void ibmveth_remove_buffer_from_pool_test(struct kunit *test)KUNIT_EXPECT_EQ (test, -EINVAL ,ibmveth_remove_buffer_from_pool (adapter, correlator, false));KUNIT_EXPECT_EQ (test, -EINVAL ,ibmveth_remove_buffer_from_pool (adapter, correlator, true)); +/* Pool 2 is in range but has no skbuff array, like an inactive pool. */ + correlator = ((u64 )2 <<32 ) |0 ; + ret =ibmveth_remove_buffer_from_pool (adapter, correlator, false); +KUNIT_EXPECT_EQ (test, -EINVAL , ret); + ret =ibmveth_remove_buffer_from_pool (adapter, correlator, true); +KUNIT_EXPECT_EQ (test, -EINVAL , ret); + correlator = (u64 )0 |0 ; pool->skbuff[0 ] =NULL ;KUNIT_EXPECT_EQ (test, -EFAULT ,ibmveth_remove_buffer_from_pool (adapter, correlator, false));
@@ -2249,9 +2304,8 @@ static void ibmveth_remove_buffer_from_pool_test(struct kunit *test)* ibmveth_rxq_get_buffer_test - unit test for ibmveth_rxq_get_buffer * @test: pointer to kunit structure * -* Tests ibmveth_rxq_get_buffer. ibmveth_rxq_get_buffer also calls WARN_ON for -* the NULL returns, so dmesg should be checked to see that these warnings -* happened. +* Tests ibmveth_rxq_get_buffer. Each NULL return also logs a ratelimited +* netdev_err. * * Return: void */
@@ -2288,6 +2342,10 @@ static void ibmveth_rxq_get_buffer_test(struct kunit *test) adapter->rx_queue.queue_addr[0 ].correlator = (u64 )0 <<32 | adapter->rx_buff_pool[0 ].size;KUNIT_EXPECT_PTR_EQ (test,NULL ,ibmveth_rxq_get_buffer (adapter)); +/* Pool 2 is in range but has no skbuff array, like an inactive pool. */ + adapter->rx_queue.queue_addr[0 ].correlator = (u64 )2 <<32 |0 ; +KUNIT_EXPECT_PTR_EQ (test,NULL ,ibmveth_rxq_get_buffer (adapter)); + pool->skbuff[0 ] = skb; adapter->rx_queue.queue_addr[0 ].correlator = (u64 )0 <<32 |0 ;KUNIT_EXPECT_PTR_EQ (test, skb,ibmveth_rxq_get_buffer (adapter));
@@ -2295,9 +2353,68 @@ static void ibmveth_rxq_get_buffer_test(struct kunit *test)flush_work (&adapter->work); } +/** +* ibmveth_rxq_harvest_buffer_test - unit test for ibmveth_rxq_harvest_buffer +* @test: pointer to kunit structure +* +* A bad correlator must still advance the RX ring, wrapping and flipping +* the toggle at the end. This covers the harvest path; the advance after +* ibmveth_rxq_get_buffer() fails in ibmveth_poll() is not tested here. +* +* Return: void +*/ +static void ibmveth_rxq_harvest_buffer_test (struct kunit *test) +{ +struct ibmveth_adapter *adapter; +struct ibmveth_buff_pool *pool; +int ret; + + adapter =kunit_kzalloc (test,sizeof (*adapter),GFP_KERNEL ); +KUNIT_ASSERT_NOT_ERR_OR_NULL (test, adapter); + +INIT_WORK (&adapter->work, ibmveth_reset_kunit); + + adapter->rx_queue.num_slots =2 ; + adapter->rx_queue.index =0 ; + adapter->rx_queue.toggle =1 ; + adapter->rx_queue.queue_addr = +kunit_kcalloc (test,2 ,sizeof (struct ibmveth_rx_q_entry ), +GFP_KERNEL ); +KUNIT_ASSERT_NOT_ERR_OR_NULL (test, adapter->rx_queue.queue_addr); + +/* Set sane values for buffer pools */ +for (int i =0 ; i <IBMVETH_NUM_BUFF_POOLS ; i++) +ibmveth_init_buffer_pool (&adapter->rx_buff_pool[i], i, + pool_count[i], pool_size[i], + pool_active[i]); + + pool = &adapter->rx_buff_pool[0 ]; + pool->skbuff =kunit_kcalloc (test, pool->size,sizeof (void *), +GFP_KERNEL ); +KUNIT_ASSERT_NOT_ERR_OR_NULL (test, pool->skbuff); + +/* Slot 0: pool out of range. Slot 1: valid, but no skb. */ + adapter->rx_queue.queue_addr[0 ].correlator = + (u64 )IBMVETH_NUM_BUFF_POOLS <<32 |0 ; + adapter->rx_queue.queue_addr[1 ].correlator = (u64 )0 <<32 |0 ; + + ret =ibmveth_rxq_harvest_buffer (adapter, true); +KUNIT_EXPECT_EQ (test, -EINVAL , ret); +KUNIT_EXPECT_EQ (test,1ULL , adapter->rx_queue.index); +KUNIT_EXPECT_EQ (test,1ULL , adapter->rx_queue.toggle); + + ret =ibmveth_rxq_harvest_buffer (adapter, true); +KUNIT_EXPECT_EQ (test, -EFAULT , ret); +KUNIT_EXPECT_EQ (test,0ULL , adapter->rx_queue.index); +KUNIT_EXPECT_EQ (test,0ULL , adapter->rx_queue.toggle); + +flush_work (&adapter->work); +} +static struct kunit_case ibmveth_test_cases[] = {KUNIT_CASE (ibmveth_remove_buffer_from_pool_test),KUNIT_CASE (ibmveth_rxq_get_buffer_test), +KUNIT_CASE (ibmveth_rxq_harvest_buffer_test), {} };
Signature · 1 line
2.39.3 (Apple Git-146)
[email protected]
AI reviewRe: [PATCH net-next v2 4/8] ibmveth: step past bad RX correlators instead of spinning or oopsing
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [High] net: NAPI list corruption and panic on bad RX correlator
Pre-existing issues:
- [Medium] Not introduced by this patch, but ibmveth_poll() takes the frame offset (flags & IBMVETH_RXQ_OFF_MASK, up to 0xFFFF) and length…
--
ibmveth: step past bad RX correlators instead of spinning or oopsing
This patch moves RX correlator validation into one helper. The helper also rejects pools that have no skbuff array, such as inactive pools. On a bad slot, ibmveth_poll() now advances the RX ring, counts the drop in rx_dropped and schedules a reset, and it logs with a ratelimited netdev_err() instead of WARN_ON(). The patch also adds and extends KUnit cases for these paths.
Quoted 4 lines
drivers/net/ethernet/ibm/ibmveth.c +0
-0
[ ... ]
Quoted 12 lines
drivers/net/ethernet/ibm/ibmveth.c +5
-1@@ -1515,12 +1556,19 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)if (rx_flush)ibmveth_flush_buffer (skb->data, length + offset); -if (unlikely (ibmveth_rxq_harvest_buffer (adapter, true))) + rc =ibmveth_rxq_harvest_buffer (adapter, true); +if (unlikely (rc)) { +dev_kfree_skb_any (new_skb); + netdev->stats.rx_dropped++;break ; + } skb = new_skb;
drivers/net/ethernet/ibm/ibmveth.c static int ibmveth_poll(struct napi_struct *napi, int budget) · line 1574
Quoted 10 lines
drivers/net/ethernet/ibm/ibmveth.c +4
-1}else { -if (unlikely (ibmveth_rxq_harvest_buffer (adapter, false))) + rc =ibmveth_rxq_harvest_buffer (adapter, false); +if (unlikely (rc)) { + netdev->stats.rx_dropped++;break ; + }skb_reserve (skb, offset); }
[Severity: High]
Now that the ring is advanced before each of these break statements, can ibmveth_poll() leave the loop with descriptors still pending and go down the completion path before the budget is used up?
After the break, frames_processed is below budget, so poll calls napi_complete_done(), re-enables the interrupt with h_vio_signal() and then finds ibmveth_rxq_pending_buffer() true for the next slot. It calls napi_schedule() and jumps to restart_poll without resetting frames_processed.
If the restarted loop then uses the rest of the budget, ibmveth_poll() returns budget after it has already rescheduled itself. The NAPI core expects a driver that consumes its whole budget to leave the NAPI state alone.
In net_rx_action(), __napi_poll() catches this and only prints the "Budget exhausted after napi rescheduled" warning. However, busy_poll_stop() calls __napi_schedule() whenever poll returns budget. Would that put napi->poll_list on the per-CPU poll list a second time and corrupt it?
A burst of traffic right behind a single bad correlator would be enough to get there.
The !ibmveth_rxq_buffer_valid() recycle path already keeps looping after it harvests a slot. Now that the ring is advanced on every error, is there still a reason to break here rather than continue with the next slot?
The same restart_poll sequence can already return budget today if a burst arrives right after the interrupt is re-enabled. Should the restart also avoid returning the full budget once napi_schedule() has succeeded?
[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The patch hardens ibmveth_poll() against bad correlators from PHYP, so should the frame offset and length from the same RX descriptor be checked as well?
ibmveth_poll() reads both values straight from the queue entry:
int length = ibmveth_rxq_frame_length(adapter);
int offset = ibmveth_rxq_frame_offset(adapter);offset is flags_off & IBMVETH_RXQ_OFF_MASK, so it can be up to 0xFFFF. length is a full be32. Neither is compared against rx_buff_pool[pool].buff_size.
On the rx_copybreak path:
skb_copy_to_linear_data(new_skb,
skb->data + offset,
length);Can this read past the end of the RX buffer? If so, nearby heap memory would be copied into a packet that is passed up the stack.
On the non-copybreak path:
skb_reserve(skb, offset);
...
skb_put(skb, length);If offset + length is too large, would this reach skb_over_panic() and BUG() in softirq context?
As with the bad correlator case, this can only happen if PHYP reports an offset or length larger than the buffer it was given.
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791178212.git.mmc%40linux.ibm.com
Re: [PATCH net-next v2 4/8] ibmveth: step past bad RX correlators instead of spinning or oopsing
Quoted 86 lines · On 10/8/26 2:08 PM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [High] net: NAPI list corruption and panic on bad RX correlator
Pre-existing issues:
- [Medium] Not introduced by this patch, butibmveth_poll()takes the frame offset (flags &IBMVETH_RXQ_OFF_MASK, up to0xFFFF) and length…--
ibmveth: step past bad RX correlators instead of spinning or oopsing
This patch moves RX correlator validation into one helper. The helper also rejects pools that have no skbuff array, such as inactive pools. On a bad slot,
ibmveth_poll()now advances the RX ring, counts the drop inrx_droppedand schedules a reset, and it logs with a ratelimitednetdev_err()instead ofWARN_ON(). The patch also adds and extends KUnit cases for these paths.Quoted 4 lines
drivers/net/ethernet/ibm/ibmveth.c +0
-0[ ... ]
Quoted 12 lines
drivers/net/ethernet/ibm/ibmveth.c +5
-1@@ -1515,12 +1556,19 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)if (rx_flush)ibmveth_flush_buffer (skb->data, length + offset); -if (unlikely (ibmveth_rxq_harvest_buffer (adapter, true))) + rc =ibmveth_rxq_harvest_buffer (adapter, true); +if (unlikely (rc)) { +dev_kfree_skb_any (new_skb); + netdev->stats.rx_dropped++;break ; + } skb = new_skb;drivers/net/ethernet/ibm/ibmveth.c static int ibmveth_poll(struct napi_struct *napi, int budget) · line 1574
Quoted 10 lines
drivers/net/ethernet/ibm/ibmveth.c +4
-1}else { -if (unlikely (ibmveth_rxq_harvest_buffer (adapter, false))) + rc =ibmveth_rxq_harvest_buffer (adapter, false); +if (unlikely (rc)) { + netdev->stats.rx_dropped++;break ; + }skb_reserve (skb, offset); }[Severity: High]
Now that the ring is advanced before each of these break statements, canibmveth_poll()leave the loop with descriptors still pending and go down the completion path before the budget is used up?After the break,
frames_processedis below budget, so poll callsnapi_complete_done(), re-enables the interrupt withh_vio_signal()and then findsibmveth_rxq_pending_buffer()true for the next slot. It callsnapi_schedule()and jumps torestart_pollwithout resettingframes_processed.If the restarted loop then uses the rest of the budget,
ibmveth_poll()returns budget after it has already rescheduled itself. The NAPI core expects a driver that consumes its whole budget to leave the NAPI state alone.In
net_rx_action(),__napi_poll()catches this and only prints the "Budget exhausted after napi rescheduled" warning. However,busy_poll_stop()calls__napi_schedule()whenever poll returns budget. Would that putnapi->poll_liston the per-CPU poll list a second time and corrupt it?A burst of traffic right behind a single bad correlator would be enough to get there.
The !ibmveth_rxq_buffer_valid() recycle path already keeps looping after it harvests a slot. Now that the ring is advanced on every error, is there still a reason to break here rather than continue with the next slot?
Quoted 4 lines
The same
restart_pollsequence can already return budget today if a burst arrives right after the interrupt is re-enabled. Should the restart also avoid returning the full budget oncenapi_schedule()has succeeded?
*
Yes. Those paths already advance the ring, so the break is wrong.
The respin continues and counts the slot against the budget.
The re-arm can do the same thing on a normal burst. Once
napi_schedule() has succeeded, the poll will not return budget.
*
Quoted 29 lines
[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The patch hardensibmveth_poll()against bad correlators from PHYP, so should the frame offset and length from the same RX descriptor be checked as well?
ibmveth_poll()reads both values straight from the queue entry:int length = ibmveth_rxq_frame_length(adapter); int offset = ibmveth_rxq_frame_offset(adapter);offset is
flags_off&IBMVETH_RXQ_OFF_MASK, so it can be up to0xFFFF. length is a full be32. Neither is compared against rx_buff_pool[pool].buff_size.On the
rx_copybreakpath:skb_copy_to_linear_data(new_skb, skb->data + offset, length);Can this read past the end of the RX buffer? If so, nearby heap memory would be copied into a packet that is passed up the stack.
On the non-copybreak path:
skb_reserve(skb, offset); ... skb_put(skb, length);
Quoted 5 lines
If offset + length is too large, would this reach
skb_over_panic()andBUG()in softirq context?As with the bad correlator case, this can only happen if PHYP reports an offset or length larger than the buffer it was given.
*
Offset and length should be checked too. The respin drops a frame
that does not fit the pool. That has not shown up in the field, so
it has no additional Fixes: tag.
pw-bot: cr
Thanks,
Mingming
*
PATCH v2 5/8 ibmveth: release the pool kobjects when probe fails
In reply to Mingming CaoIf register_netdev() fails in ibmveth_probe(), for example with -EINTR when the binding task is killed, probe frees the netdev but leaves the pool%d kobjects embedded in it registered in sysfs. Reading /sys/devices/vio/<unit>/pool0/num is then a use-after-free, and the next probe cannot add pool0.
Put the kobjects before freeing the netdev, as ibmveth_remove() does, on this path and on the netif_set_real_num_tx_queues() one.
The kobjects also have no release(). With CONFIG_DEBUG_KOBJECT_RELEASE, kobject_put() defers their cleanup, including removing the sysfs files, to a work item, so free_netdev() can free them first, here and in remove(). Add a release() that signals a per-pool completion, and wait for it in both places before free_netdev(). Without that config, release() runs from kobject_put() and the wait returns at once.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection; the release() part was raised by the Sashiko AI review of the first version. Tested on a POWER10 LPAR with register_netdev() forced to fail with -EINTR by a test-only module parameter (not part of this patch): no pool%d directories remain and the device binds again. No kernel selftests cover ibmveth.
---
Changes in v2:
- give the pool kobjects a release() that signals a per-pool completion, and wait for it before free_netdev() in probe and remove(); with CONFIG_DEBUG_KOBJECT_RELEASE the deferred cleanup could run after the free (Sashiko review of v1)
- put the kobjects through one helper, ibmveth_put_pool_kobjs(), and an err_put_pools label for both probe failure paths
drivers/net/ethernet/ibm/ibmveth.c | 52 +++++++++++++++++++++++++----- drivers/net/ethernet/ibm/ibmveth.h | 3 ++ 2 files changed, 47 insertions(+), 8 deletions(-)
drivers/net/ethernet/ibm/ibmveth.c +44 -8
@@ -1850,6 +1850,40 @@ static const struct net_device_ops ibmveth_netdev_ops = { .ndo_features_check = ibmveth_features_check, }; +/** +* ibmveth_pool_kobj_release - Mark a pool kobject finished +* @kobj: kobject embedded in the pool +* +* The pool kobjects live in netdev_priv(), so the last put must wait +* for this before free_netdev(). +*/ +static void ibmveth_pool_kobj_release (struct kobject *kobj) +{ +struct ibmveth_buff_pool *pool =container_of (kobj, + struct ibmveth_buff_pool, + kobj); + +complete (&pool->released); +} + +/** +* ibmveth_put_pool_kobjs - Drop the pool kobjects and wait for release +* @adapter: ibmveth adapter +* +* With CONFIG_DEBUG_KOBJECT_RELEASE the cleanup, including removing the +* sysfs files, runs later from a work item in the kobject; wait for it +* so free_netdev() cannot free the pools first. +*/ +static void ibmveth_put_pool_kobjs (struct ibmveth_adapter *adapter) +{ +int i; + +for (i =0 ; i <IBMVETH_NUM_BUFF_POOLS ; i++) +kobject_put (&adapter->rx_buff_pool[i].kobj); +for (i =0 ; i <IBMVETH_NUM_BUFF_POOLS ; i++) +wait_for_completion (&adapter->rx_buff_pool[i].released); +} +static int ibmveth_probe (struct vio_dev *dev,const struct vio_device_id *id) {int rc, i, mac_len;
@@ -1960,6 +1994,7 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)ibmveth_init_buffer_pool (&adapter->rx_buff_pool[i], i, pool_count[i], pool_size[i], pool_active[i]); +init_completion (&adapter->rx_buff_pool[i].released); error =kobject_init_and_add (kobj, &ktype_veth_pool, &dev->dev.kobj,"pool%d" , i);if (!error)
@@ -1971,8 +2006,7 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)if (rc) {netdev_dbg (netdev,"failed to set number of tx queues rc=%d\n" , rc); -free_netdev (netdev); -return rc; + goto err_put_pools; } adapter->tx_ltb_size =PAGE_ALIGN (IBMVETH_MAX_TX_BUF_SIZE );for (i =0 ; i <IBMVETH_MAX_QUEUES ; i++)
@@ -1987,25 +2021,27 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)if (rc) {netdev_dbg (netdev,"failed to register netdev rc=%d\n" , rc); -free_netdev (netdev); -return rc; + goto err_put_pools; }netdev_dbg (netdev,"registered\n" );return 0 ; + +err_put_pools: +ibmveth_put_pool_kobjs (adapter); +free_netdev (netdev); +return rc; }static void ibmveth_remove (struct vio_dev *dev) {struct net_device *netdev =dev_get_drvdata (&dev->dev);struct ibmveth_adapter *adapter =netdev_priv (netdev); -int i;disable_work_sync (&adapter->work); -for (i =0 ; i <IBMVETH_NUM_BUFF_POOLS ; i++) -kobject_put (&adapter->rx_buff_pool[i].kobj); +ibmveth_put_pool_kobjs (adapter);unregister_netdev (netdev);
@@ -2181,7 +2217,7 @@ static const struct sysfs_ops veth_pool_ops = { };static struct kobj_type ktype_veth_pool = { - .release =NULL , + .release = ibmveth_pool_kobj_release, .sysfs_ops = &veth_pool_ops, .default_groups = veth_pool_groups, };
drivers/net/ethernet/ibm/ibmveth.h +3 -0
@@ -14,6 +14,8 @@#ifndef _IBMVETH_H#define _IBMVETH_H +#include <linux/completion.h> +/* constants for H_MULTICAST_CTRL */ #define IbmVethMcastReceptionModifyBit 0x80000UL#define IbmVethMcastReceptionEnableBit 0x20000UL
@@ -143,6 +145,7 @@ struct ibmveth_buff_pool {struct sk_buff **skbuff;int active;struct kobject kobj; +struct completion released; };struct ibmveth_rx_q {
Signature · 1 line
2.39.3 (Apple Git-146)
PATCH v2 6/8 ibmveth: return the error when set_channels cannot add TX queues
In reply to Mingming CaoWhen ibmveth_set_channels() cannot allocate a TX buffer for a new queue, it falls back to the old queue count, and the successful netif_set_real_num_tx_queues() call then overwrites rc. ethtool -L reports success while the queue count is unchanged.
Return the allocation error when the fallback succeeds.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection. Tested on a POWER10 LPAR with the TX buffer allocation forced to fail by a test-only module parameter (not part of this patch): ethtool -L tx 8 now fails with -ENOMEM and the device keeps its four queues and passes traffic. No kernel selftests cover ibmveth.
--- drivers/net/ethernet/ibm/ibmveth.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/ethernet/ibm/ibmveth.c +4 -1
@@ -1169,7 +1169,7 @@ static int ibmveth_set_channels(struct net_device *netdev,struct ibmveth_adapter *adapter =netdev_priv (netdev);unsigned int old = netdev->real_num_tx_queues, goal = channels->tx_count; -int rc, i; +int rc, i, alloc_rc =0 ;/* If the device is not open (including a failed close/open with * IFF_UP still set) then don't allocate, just set desired
@@ -1195,6 +1195,7 @@ static int ibmveth_set_channels(struct net_device *netdev,/* if something goes wrong, free everything we just allocated */ netdev_err (netdev,"Failed to allocate more tx queues, returning to %d queues\n" , old); + alloc_rc = rc; goal = old; old = i;break ;
@@ -1205,6 +1206,8 @@ static int ibmveth_set_channels(struct net_device *netdev, old); goal = old; old = i; + }else if (alloc_rc) { + rc = alloc_rc; }/* Free any that are no longer needed */ for (i = old; i > goal; i--) {
Signature · 1 line
2.39.3 (Apple Git-146)
PATCH v2 7/8 ibmveth: wait for in-flight transmits in ibmveth_close()
In reply to Mingming Caoibmveth_close() frees the TX buffers after netif_tx_stop_all_queues(), which does not wait for an ibmveth_start_xmit() already running on another CPU. That transmit can copy into a freed buffer and hand PHYP a stale DMA address.
MTU, csum/TSO and buffer pool changes call ibmveth_close() directly while traffic flows. ifdown and the reset work go through dev_close(), which waits for running transmits only when the qdisc has an enqueue function, so with noqueue they race the same way.
Use netif_tx_disable(), which waits for running transmits.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection; the race was not reproduced. Tested on a POWER10 LPAR with MTU changes during a ping flood, with no warnings. No kernel selftests cover ibmveth.
---
Changes in v2:
- commit message: dev_close() waits for running transmits only when the qdisc has an enqueue function, so ifdown and the reset work race the same way with noqueue
drivers/net/ethernet/ibm/ibmveth.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/ibm/ibmveth.c +1 -1
@@ -827,7 +827,7 @@ static int ibmveth_close(struct net_device *netdev)napi_disable (&adapter->napi); -netif_tx_stop_all_queues (netdev); +netif_tx_disable (netdev);h_vio_signal (adapter->vdev->unit_address,VIO_IRQ_DISABLE );
Signature · 1 line
2.39.3 (Apple Git-146)
PATCH v2 8/8 ibmveth: wait for the RX poll to return before freeing the RX queue
In reply to Mingming Caonapi_disable() returns once ibmveth_poll() has called napi_complete_done(), but the poll is not finished: it then re-enables the interrupt and calls ibmveth_rxq_pending_buffer(), which reads the RX queue. ibmveth_close() can free that queue in the meantime, and the late enable can leave the interrupt unmasked after close() masked it. The request_irq() failure path in ibmveth_open() frees the same memory after napi_disable() too.
Call synchronize_net() after napi_disable() on both paths. Every caller of ibmveth_poll() runs it with bottom halves or interrupts disabled, so this waits for the poll to return.
Found by AI-assisted review of the ibmveth multi-queue RX series and confirmed by code inspection; also raised by the Sashiko AI review of the first version of this series. The race was not reproduced. Tested on a POWER10 LPAR under an incoming ping flood with 30 rapid link down/up cycles and 30 rapid MTU cycles (1500 <-> 9000); ran cleanly with no warnings or faults. No kernel selftests cover ibmveth.
---
Changes in v2:
- new patch; raised by the Sashiko review of v1 patch 1 as a pre-existing bug
drivers/net/ethernet/ibm/ibmveth.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/net/ethernet/ibm/ibmveth.c +6 -0
@@ -757,6 +757,7 @@ static int ibmveth_open(struct net_device *netdev) netdev);if (rc !=0 ) {napi_disable (&adapter->napi); +synchronize_net ();netdev_err (netdev,"unable to request irq 0x%x, rc %d\n" , netdev->irq, rc); do {
@@ -826,6 +827,11 @@ static int ibmveth_close(struct net_device *netdev)netdev_dbg (netdev,"close starting\n" );napi_disable (&adapter->napi); +/* napi_disable() returns once ibmveth_poll() has called +* napi_complete_done(), but the poll still re-enables the +* interrupt and reads the RX queue after that. +*/ +synchronize_net ();netif_tx_disable (netdev);
--
2.39.3 (Apple Git-146)