Re: [net-next,v6,03/15] ibmveth: Refactor RX resource allocation for MQ RX bring-up

[mingming cao <mmc@linux.ibm.com>]

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 (2) · Medium: 0 (3) · Low: 2
>
> - [High] ibmveth_cleanup_rx_resources() and ibmveth_alloc_rx_queues()'s
>    rollback newly store NULL into adapter->buffer_list_addr[i] (and…
> - [Low] The patch is presented as a mechanical refactor ("Runtime
>    behavior stays single-queue") but bundles a claimed memory-corruption…
> - [Low] The comment added above the err_cleanup: rollback loop in
>    ibmveth_alloc_rx_queues() states an invariant that the function's own…
>
> Pre-existing issues:
> - [High] ibmveth_open() calls napi_enable(&adapter->napi[0]) at entry and
>    napi_disable() on every failure at the out: label (ibmveth.c:907),…
> - [High] On the ibmveth_open() error path taken after
>    h_register_logical_lan() has already succeeded (buffer-pool allocation…
> - [Medium] TX long-term buffers are leaked when ibmveth_open() fails in
>    the buffer-pool allocation loop or in request_irq(): the loop counter…
> - [Medium] The TX long-term buffers keep the stale-DMA-handle pattern
>    that this patch fixes for the RX/filter lists: ibmveth_free_tx_ltb()…
> - [Medium] ibmveth_poll_controller() (ibmveth.c:1815-1819) calls
>    ibmveth_replenish_task() directly from netpoll context, bypassing…
>
> --
>
> ibmveth: Refactor RX resource allocation for MQ RX bring-up
>
> This moves the multicast filter list and the per-queue RX buffer lists and
> RX rings out of ibmveth_open() into ibmveth_alloc_filter_list(),
> ibmveth_alloc_rx_queues() and ibmveth_cleanup_rx_resources(), and wires
> those helpers into open()/close().  Runtime behaviour stays single-queue,
> and the helpers now NULL each pointer slot and zero each DMA handle as
> they free.
>
> (no Link: tags present in the commit header)
>
> Before the diff, a question about the changelog itself.
>
> [Severity: Low]
> The commit message says:
>
>    "The pointer and DMA-handle guards also close a baseline double
>     free_page() and stale dma_unmap after a failed reopen followed by
>     ndo_stop"
>
> Is that baseline sequence actually reachable?  ibmveth_open() calls
> napi_enable(&adapter->napi[0]) on entry and napi_disable() at its out:
> label, so after a failed reopen NAPI is already disabled while
> netif_running() is still true.  The following ndo_stop enters
> ibmveth_close(), whose first action is another napi_disable():
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_close() {
> 	napi_disable(&adapter->napi[0]);
>
> 	netif_tx_stop_all_queues(netdev);
> 	...
> }
>
> napi_disable_locked() leaves NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC set on
> exit, and only napi_enable()/napi_complete() clear SCHED, so the second
> call sits in:
>
> net/core/dev.c:napi_disable_locked() {
> 	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
> 		usleep_range(20, 200);
> 		val = READ_ONCE(n->state);
> 	}
> 	...
> }
>
> That is ahead of every free path in close(), so the double free_page()
> described in the changelog does not appear to be reachable as written.
>
> Separately, if these guards do fix memory corruption on stable kernels,
> would it be better to split them into their own patch with a Fixes: tag
> ahead of the refactor, rather than bundling them into a 200 line
> restructuring?
You are right. v7 drops that sentence. Failed reopen then
ndo_stop never reaches the second free_page() here: close()
hits napi_disable() first.

The pointer and DMA-handle checks stay in this helper extract
rather than being peeled into a Fixes: patch in this 15. On
current kernels that ndo_stop path never reaches the frees, so
the checks alone are not a stable hotfix. After this series a
standalone unwind for net can take
Fixes: d43732ce021f ("ibmveth: properly unwind on init errors").

>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 7cb828b476c1b..d2dad9e6068e0 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -151,6 +151,193 @@ static unsigned int ibmveth_real_max_tx_queues(void)
>>   	return min(n_cpu, IBMVETH_MAX_QUEUES);
>>   }
>>   
> [ ... ]
>
>> +		adapter->buffer_list_dma[i] =
>> +			dma_map_single(dev, adapter->buffer_list_addr[i],
>> +				       4096, DMA_BIDIRECTIONAL);
>> +		if (dma_mapping_error(dev, adapter->buffer_list_dma[i])) {
>> +			netdev_err(netdev,
>> +				   "unable to map buffer list for queue %d\n",
>> +				   i);
>> +			free_page((unsigned long)adapter->buffer_list_addr[i]);
>> +			adapter->buffer_list_addr[i] = NULL;
>> +			adapter->buffer_list_dma[i] = 0;
>> +			goto err_cleanup;
>> +		}
> [ ... ]
>
>> +err_cleanup:
>> +	/*
>> +	 * Every failure path above releases what it had already allocated
>> +	 * for queue i, so each index here is either fully constructed or
>> +	 * fully empty. Do not unmap buffer_list_dma[] without the matching
>> +	 * buffer_list_addr[] check: the two are only ever set together.
>> +	 */
> [Severity: Low]
> Does this comment match what the dma_mapping_error() branch just above
> leaves behind?  On that path buffer_list_addr[i] is freed and NULLed and
> buffer_list_dma[i] is zeroed, but rx_queue[i].queue_addr from
> dma_alloc_coherent() is still live, so index i arrives here partially
> constructed rather than "fully constructed or fully empty".
>
> The loop below is written with independent checks so it still frees the
> ring, but a later reader who trusts "the two are only ever set together"
> could collapse the two tests into one buffer_list_addr[] guard and leak
> the coherent RX ring on exactly that failure path.  Could the comment be
> reworded to describe the mixed state?
Yes — thanks. v7 rewords the err_cleanup comment: cleanup
frees by pointer presence. A dma_mapping_error on queue i
can leave queue_addr live after buffer_list_addr is already
NULL. The two checks stay independent.

>> +	for (; i >= 0; i--) {
>> +		if (adapter->buffer_list_addr[i]) {
>> +			dma_unmap_single(dev, adapter->buffer_list_dma[i],
>> +					 4096, DMA_BIDIRECTIONAL);
>> +			adapter->buffer_list_dma[i] = 0;
>> +		}
>> +		if (adapter->rx_queue[i].queue_addr) {
>> +			dma_free_coherent(dev, adapter->rx_queue[i].queue_len,
>> +					  adapter->rx_queue[i].queue_addr,
>> +					  adapter->rx_queue[i].queue_dma);
>> +			adapter->rx_queue[i].queue_addr = NULL;
>> +		}
>> +		if (adapter->buffer_list_addr[i]) {
>> +			free_page((unsigned long)adapter->buffer_list_addr[i]);
>> +			adapter->buffer_list_addr[i] = NULL;
>> +		}
>> +	}
>> +
>> +	return -ENOMEM;
>> +}
> [ ... ]
>
>> +static void
>> +ibmveth_cleanup_rx_resources(struct ibmveth_adapter *adapter)
>> +{
>> +	struct device *dev = &adapter->vdev->dev;
>> +	int i;
>> +
>> +	netdev_dbg(adapter->netdev, "cleaning up %u RX queue(s)\n",
>> +		   adapter->num_rx_queues);
>> +
>> +	for (i = 0; i < adapter->num_rx_queues; i++) {
>> +		if (adapter->buffer_list_addr[i]) {
>> +			dma_unmap_single(dev, adapter->buffer_list_dma[i],
>> +					 4096, DMA_BIDIRECTIONAL);
>> +			adapter->buffer_list_dma[i] = 0;
>> +		}
>> +
>> +		if (adapter->rx_queue[i].queue_addr) {
>> +			dma_free_coherent(dev, adapter->rx_queue[i].queue_len,
>> +					  adapter->rx_queue[i].queue_addr,
>> +					  adapter->rx_queue[i].queue_dma);
>> +			adapter->rx_queue[i].queue_addr = NULL;
>> +		}
>> +
>> +		if (adapter->buffer_list_addr[i]) {
>> +			free_page((unsigned long)adapter->buffer_list_addr[i]);
>> +			adapter->buffer_list_addr[i] = NULL;
>> +		}
>> +	}
>> +}
> [Severity: High]
> This helper now stores NULL into buffer_list_addr[i], but
> ibmveth_update_rx_no_buffer() still dereferences that slot with no check:
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_update_rx_no_buffer() {
> 	__be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
>
> 	adapter->rx_no_buffer = be64_to_cpup(p);
> }
>
> Can that read fault after a failed reopen?  ibmveth_set_csum_offload(),
> ibmveth_set_tso(), __ibmveth_change_mtu() and veth_pool_store() all do
> ibmveth_close() followed by ibmveth_open() and only return the error if
> the reopen fails, so netif_running() stays true while
> ibmveth_open()'s unwind has already run ibmveth_cleanup_rx_resources()
> and left buffer_list_addr[0] NULL.
>
> The next netpoll poll then reaches the reader without going through NAPI:
>
> ibmveth_poll_controller()
>    ibmveth_replenish_task()
>      ibmveth_update_rx_no_buffer()   /* NULL + 4088 */
>
> ibmveth_free_buffer_pool() does not reset pool->available, so after a
> close the "available < threshold" test in ibmveth_replenish_task() is
> false for the fully posted pools and the loop falls through to
> ibmveth_update_rx_no_buffer().
>
> ibmveth_close() has the same ordering problem: it calls
> ibmveth_update_rx_no_buffer() before the new cleanup helpers, so the
> "failed reopen followed by ndo_stop" sequence in the changelog reaches
> the unguarded reader.
>
> In the baseline the same pointer was stale but still mapped, so this
> turns a silent read into a NULL dereference.  A matching
> "if (!adapter->buffer_list_addr[queue_index]) return;" guard, together
> with an "if (!adapter->opened) return;" gate in
> ibmveth_poll_controller(), only shows up in a later patch of this series
> - could that guard come in this patch instead, so the NULL state is never
> introduced without it?
Good catch — yes, at this commit. After a failed reopen,
netpoll can still reach the reader (IFF_UP is set, the
pointer is already NULL). The later ndo_stop hits
napi_disable() in close() first.

v7 adds the missing NULL return in
ibmveth_update_rx_no_buffer() here, so this helper extract
does not introduce the NULL state without a guard. The
opened gate on close() and poll_controller still lands in
the later patches — opened is not available yet.

>> +
>>   /* setup the initial settings for a buffer pool */
>>   static void ibmveth_init_buffer_pool(struct ibmveth_buff_pool *pool,
>>   				     u32 pool_index, u32 pool_size,
> [ ... ]
>
>> @@ -708,7 +855,7 @@ static int ibmveth_open(struct net_device *netdev)
>>   				     rxq_desc.desc,
>>   				     mac_address);
>>   		rc = -ENONET;
>> -		goto out_unmap_filter_list;
>> +		goto out_free_tx_ltb;
>>   	}
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the TX long term buffers
> keep the stale DMA handle pattern that the RX and filter helpers here
> now avoid:
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_free_tx_ltb() {
> 	dma_unmap_single(&adapter->vdev->dev, adapter->tx_ltb_dma[idx],
> 			 adapter->tx_ltb_size, DMA_TO_DEVICE);
> 	kfree(adapter->tx_ltb_ptr[idx]);
> 	adapter->tx_ltb_ptr[idx] = NULL;
> }
>
> tx_ltb_dma[idx] is never zeroed and the unmap is not gated on
> tx_ltb_ptr[idx].  When the h_register_logical_lan() failure above takes
> out_free_tx_ltb with the TX LTBs already allocated, a later
> ibmveth_close() loops over all real_num_tx_queues and calls
> ibmveth_free_tx_ltb() again, repeating dma_unmap_single() on a handle the
> driver no longer owns.  Can that tear down a recycled TCE?
>
> The changelog notes this is deferred, and the later "ibmveth: Refactor TX
> resource allocation in open/close paths" patch adds the pointer check and
> zeroes tx_ltb_dma[idx].
Yes. This patch only does the RX and filter side. The TX
long-term buffers get the same pointer check and a cleared
dma handle in the later TX helper patch ("Refactor TX
resource allocation in open/close paths"). That is already
in this series — deferred from here, not dropped.

>>   
>>   	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> @@ -735,8 +882,6 @@ static int ibmveth_open(struct net_device *netdev)
>>   		goto out_free_buffer_pools;
>>   	}
>>   
>> -	rc = -ENOMEM;
>> -
>>   	netdev_dbg(netdev, "initial replenish cycle\n");
>>   	ibmveth_interrupt(netdev->irq, netdev);
>>   
>> @@ -752,26 +897,12 @@ static int ibmveth_open(struct net_device *netdev)
>>   			ibmveth_free_buffer_pool(adapter,
>>   						 &adapter->rx_buff_pool[0][i]);
>>   	}
> [Severity: High]
> This is a pre-existing ordering issue, but the fall-through from
> out_free_buffer_pools now releases more state than before.  The buffer
> pool loop runs after h_register_logical_lan() has succeeded, and on
> ibmveth_alloc_buffer_pool() failure this label falls through to
> out_free_tx_ltb and out_free_filter_list, which now call
> ibmveth_cleanup_rx_resources() and ibmveth_free_filter_list() with no
> h_free_logical_lan() anywhere on the path.
>
> The adjacent request_irq() failure path does perform the
> deregistration first:
>
> 		do {
> 			lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
> 		} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>
> Can PHYP keep DMA-writing the no-buffer counter in the last eight bytes
> of the buffer list page after those pages have been unmapped and returned
> to the page allocator?
>
> The changelog acknowledges this predates the patch, and at the end of the
> series ibmveth_open() allocates the pools before registering and every
> post-registration failure goes through ibmveth_free_all_queues(), which
> issues h_free_logical_lan().
Yes, pre-existing at this commit. The TX helper patch issues
h_free on that pool-fail path. The register-helper patch then
allocates the pools before register, and every later failure
goes through ibmveth_free_all_queues().
>> -out_unmap_filter_list:
>> -	dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -
>>   out_free_tx_ltb:
>> -	while (--i >= 0) {
>> +	while (--i >= 0)
>>   		ibmveth_free_tx_ltb(adapter, i);
>> -	}
> [Severity: Medium]
> This is a pre-existing problem rather than something this patch adds, but
> does this loop ever run when the failure came from the buffer pool loop
> or from request_irq()?  Both out_free_buffer_pools and out_free_tx_ltb
> consume the same counter i, and i has been reused as the buffer pool
> index by then:
>
> out_free_buffer_pools:
> 	while (--i >= 0) {		/* drains i to -1 */
> 		...
> 	}
> out_free_tx_ltb:
> 	while (--i >= 0)		/* starts at -2, never runs */
> 		ibmveth_free_tx_ltb(adapter, i);
> So every tx_ltb_ptr[]/tx_ltb_dma[] allocated in this open() stays
> allocated and mapped.  The changelog mentions the shared i leak, and the
> later TX refactor patch gives the rollback its own counter.
No — not on those two paths. out_free_buffer_pools already
walks i down to -1, so the fall-through while (--i >= 0)
never frees a TX LTB. Pre-existing. The TX helper patch
gives that rollback its own counter, and it allocates TX
after the pools, so a pool failure has no LTBs to leak.

>> -
>> -out_unmap_buffer_list:
>> -	dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -out_free_queue_mem:
>> -	dma_free_coherent(dev, adapter->rx_queue[0].queue_len,
>> -			  adapter->rx_queue[0].queue_addr,
>> -			  adapter->rx_queue[0].queue_dma);
>> +	ibmveth_cleanup_rx_resources(adapter);
>>   out_free_filter_list:
>> -	free_page((unsigned long)adapter->filter_list_addr);
>> -out_free_buffer_list:
>> -	free_page((unsigned long)adapter->buffer_list_addr[0]);
>> +	ibmveth_free_filter_list(adapter);
>>   out:
>>   	napi_disable(&adapter->napi[0]);
>>   	return rc;
> [Severity: High]
> This is a pre-existing imbalance and not introduced here, but it is the
> path the changelog leans on, so it seems worth raising.
>
> Every open failure leaves NAPI disabled while the device is still
> administratively up, because ibmveth_set_csum_offload(), ibmveth_set_tso(),
> __ibmveth_change_mtu() and veth_pool_store() just return the reopen error.
> ibmveth_close() then calls napi_disable() a second time with no
> intervening napi_enable(), and napi_disable_locked() spins:
>
> net/core/dev.c:napi_disable_locked() {
> 	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
> 		usleep_range(20, 200);
> 		val = READ_ONCE(n->state);
> 	}
> 	...
> }
>
> Nothing clears SCHED at that point, and the loop runs inside dev_close()
> with rtnl_lock() and the netdev lock held.  Can an ifdown after a failed
> reopen wedge every later RTNL operation?
>
> The series later drops this by returning early from close() when
> adapter->opened is false and handling NAPI in
> ibmveth_cleanup_rx_interrupts().
Yes — at this commit that ifdown can wedge. open() enables
NAPI on entry and disables it on every failure, then close()
calls napi_disable() again. Pre-existing. Patch 5 ("Refactor
RX interrupt control for MQ RX queues") fixes it: close()
returns early when opened is false, and NAPI teardown moves
into ibmveth_cleanup_rx_interrupts().
>> @@ -780,7 +911,6 @@ static int ibmveth_open(struct net_device *netdev)
>>   static int ibmveth_close(struct net_device *netdev)
>>   {
>>   	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> -	struct device *dev = &adapter->vdev->dev;
>>   	long lpar_rc;
>>   	int i;
>>   
>> @@ -805,17 +935,8 @@ static int ibmveth_close(struct net_device *netdev)
>>   
>>   	ibmveth_update_rx_no_buffer(adapter);
>>   
>> -	dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -	free_page((unsigned long)adapter->buffer_list_addr[0]);
>> -
>> -	dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -	free_page((unsigned long)adapter->filter_list_addr);
>> -
>> -	dma_free_coherent(dev, adapter->rx_queue[0].queue_len,
>> -			  adapter->rx_queue[0].queue_addr,
>> -			  adapter->rx_queue[0].queue_dma);
>> +	ibmveth_cleanup_rx_resources(adapter);
>> +	ibmveth_free_filter_list(adapter);
> [Severity: Medium]
> This isn't a bug introduced by the patch either, but the failure mode
> changes here.  What serializes these frees against netpoll?
>
> ibmveth_poll_controller() calls ibmveth_replenish_task() directly, so it
> never goes through napi_schedule()/poll and the napi_disable() at the top
> of ibmveth_close() does not exclude it:
>
> static void ibmveth_poll_controller(struct net_device *dev)
> {
> 	ibmveth_replenish_task(netdev_priv(dev));
> 	ibmveth_interrupt(dev->irq, dev);
> }
>
> The teardown path holds only rtnl and the netdev lock, which netpoll does
> not take, and the internal close()/open() callers (ethtool csum and tso,
> change_mtu, veth_pool_store) never call netpoll_poll_disable(), which only
> dev_close()/dev_open() do.  With the slots now NULLed, a racing netpoll
> poll dereferences NULL rather than reading a stale but mapped page.
>
> The series later adds the adapter->opened gate in
> ibmveth_poll_controller(), the per-queue replenish_lock, and the NULL
> check in ibmveth_update_rx_no_buffer().
>>   
>>   	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
>>   		if (adapter->rx_buff_pool[0][i].active)

At this commit, nothing does — netpoll can still call
replenish after cleanup has NULLed the buffer list. v7 adds
the NULL return in update_rx_no_buffer() in this patch, so
that read no longer faults. Later patches add replenish_lock
(patch 8) and poll_controller returns if !opened (patch 15).
What is still leftover is netpoll vs free_buffer_pool /
irqsave, for a follow-up after this series.



Thanks,
Mingming