Re: [PATCH net-next v4 06/14] ibmveth: Refactor TX resource allocation in open/close paths

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

On 8/6/26 11:37 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ibmveth: Refactor TX resource allocation in open/close paths
>
> This moves the per-queue TX long term buffer alloc/free loops out of
> ibmveth_open() and ibmveth_close() into ibmveth_alloc_tx_resources() and
> ibmveth_free_tx_resources(), relocates the TX allocation to the end of
> open(), and reworks the open() error unwind labels.
>
> A few questions on the changelog and on the new ordering below.
Hi Jakub,

Thanks for the review.

> [Severity: Medium]
> The changelog says:
>
>      That also retires a mid-open TX leak where the old index-based
>      while (--i) free could skip successfully allocated LTBs.
>
> Is that leak real?  In the parent commit the TX loop re-initializes the
> shared index before allocating anything:
>
> 	rc = -ENOMEM;
> 	for (i = 0; i < netdev->real_num_tx_queues; i++) {
> 		if (ibmveth_allocate_tx_ltb(adapter, i))
> 			goto out_free_tx_ltb;
> 	}
>
> and nothing between that loop and the later gotos (mac_address/rxq_desc
> setup, ibmveth_register_logical_lan(), ibmveth_alloc_buffer_pools(),
> ibmveth_setup_rx_interrupts()) reassigns i.  On every path that reaches
> out_free_tx_ltb, i is either the failing index or real_num_tx_queues, so
> while (--i >= 0) frees exactly what was allocated.  Paths taken before the
> TX loop jump to out_free_filter_list, which is below the TX label.
>
> The new helper also uses the same index-based unwind that the changelog
> calls broken:
>
> err_free_ltbs:
> 	while (--i >= 0)
> 		ibmveth_free_tx_ltb(adapter, i);
>
> Could this paragraph be dropped or reworded to describe the actual
> motivation (removing the dependence on a shared loop index and matching
> the RX helpers)?  As written it reads like a leak fix that a stable
> maintainer might want to backport, and there is no Fixes: tag.
Agreed for this patch. I will drop that paragraph and reword the
motivation in terms of helper extraction, avoiding dependence on shared
open-path loop state, and matching the RX helper structure, rather than
presenting it as a leak fix here.
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 664169c4d27a4..131c1a61d9e7e 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -1077,8 +1077,14 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
>>   
>>   static void ibmveth_free_tx_ltb(struct ibmveth_adapter *adapter, int idx)
>>   {
>> -	dma_unmap_single(&adapter->vdev->dev, adapter->tx_ltb_dma[idx],
>> -			 adapter->tx_ltb_size, DMA_TO_DEVICE);
>> +	if (!adapter->tx_ltb_ptr[idx])
>> +		return;
>> +
>> +	if (adapter->tx_ltb_dma[idx]) {
>> +		dma_unmap_single(&adapter->vdev->dev, adapter->tx_ltb_dma[idx],
>> +				 adapter->tx_ltb_size, DMA_TO_DEVICE);
>> +		adapter->tx_ltb_dma[idx] = 0;
>> +	}
> [Severity: Low]
> Is the inner test on tx_ltb_dma[idx] wanted here?  It treats DMA address 0
> as a "not mapped" sentinel, and the only architecturally valid check for a
> bad handle is dma_mapping_error() at map time.  If a mapping ever
> legitimately returned IOVA 0, the TCE entry would leak while kfree() below
> still released the buffer.
>
> On pseries this cannot happen today, since
> arch/powerpc/kernel/iommu.c:iommu_table_reserve_pages() reserves entry 0:
>
> 	if (tbl->it_offset == 0)
> 		set_bit(0, tbl->it_map);
>
> The test also looks redundant: ibmveth_allocate_tx_ltb() already kfree()s
> the buffer and clears tx_ltb_ptr[idx] on dma_mapping_error(), so
> tx_ltb_ptr[idx] != NULL already implies a valid mapping and the new early
> return covers the stated "safe on an unallocated slot" goal.  Would
> dropping the zero test and relying on the pointer check be simpler?
Agreed. In v5 I will just key the free path off allocation state instead
of treating DMA address 0 as a sentinel.
>>   	kfree(adapter->tx_ltb_ptr[idx]);
>>   	adapter->tx_ltb_ptr[idx] = NULL;
>>   }
> [ ... ]
>
>> @@ -1184,35 +1226,38 @@ static int ibmveth_open(struct net_device *netdev)
>>   				     rxq_desc.desc,
>>   				     mac_address);
>>   		rc = -ENONET;
>> -		goto out_free_tx_ltb;
>> +		goto out_free_queue_mem;
>>   	}
>>   
>>   	rc = ibmveth_alloc_buffer_pools(adapter);
>>   	if (rc)
>> -		goto out_free_tx_ltb;
>> +		goto out_unregister_lan;
> [Severity: Low]
> This changes hypervisor-visible behaviour on an RX error path, which the
> changelog does not mention while stating "there is no MQ RX behaviour
> change here".
>
> In the parent commit an ibmveth_alloc_buffer_pools() failure went to
> out_free_tx_ltb and then to ibmveth_cleanup_rx_resources(), leaving the
> logical LAN registered with PHYP while the RX ring was freed and the
> buffer list unmapped.  With the new label the h_free_logical_lan() loop
> runs first, which is the safer order.
>
> Is this an intentional fix folded into a TX-only refactor?  If so, could
> it be split out with its own changelog text (the next patch in the series
> reorders open() so this path disappears entirely, so the fix becomes hard
> to find later).
Yes, that change was intentional, and I agree it needs to be called out
more clearly. Freeing the LAN before tearing down the RX resources is
the safer order on that failure path, so in v5 I will say that
explicitly in the changelog instead of letting it read like a pure TX
refactor.
>>   
>>   	rc = ibmveth_setup_rx_interrupts(adapter);
>> -	if (rc) {
>> -		do {
>> -			lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>> -		} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>> -		goto out_free_buffer_pools;
>> -	}
>> +	if (rc)
>> +		goto out_unregister_lan;
>>   
>>   	netdev_dbg(netdev, "initial replenish cycle\n");
>>   	ibmveth_schedule_rx_queue(adapter, 0);
>>   
>> +	rc = ibmveth_alloc_tx_resources(adapter);
>> +	if (rc)
>> +		goto out_cleanup_rx_interrupts;
>> +
>>   	netif_tx_start_all_queues(netdev);
> [ ... ]
>
>> @@ -1224,13 +1269,13 @@ static int ibmveth_close(struct net_device *netdev)
>>   {
>>   	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>>   	long lpar_rc;
>> -	int i;
>>   
>>   	netdev_dbg(netdev, "close starting\n");
>>   
>>   	netif_tx_stop_all_queues(netdev);
>>   
>>   	/* PHYP mask + napi_disable + free_irq live in cleanup_rx_interrupts */
>> +	ibmveth_free_tx_resources(adapter);
>>   	ibmveth_cleanup_rx_interrupts(adapter);
> [Severity: High]
> Can a transmit already in flight write into the TX LTB after it has been
> unmapped and freed here?
>
> netif_tx_stop_all_queues() only sets __QUEUE_STATE_DRV_XOFF.  It does not
> take __netif_tx_lock and does not wait for an ndo_start_xmit that is
> already running on another CPU (netif_tx_disable() would).  ibmveth_close()
> is also called directly, bypassing dev_close() -> dev_deactivate_many()
> and its synchronize_net() / some_qdisc_is_busy() wait, from:
>
> 	ibmveth_set_csum_offload()
> 	ibmveth_set_tso()
> 	ibmveth_change_mtu()
> 	veth_pool_store()
>
> all of which do:
>
> 	if (netif_running(dev)) {
> 		restart = 1;
> 		ibmveth_close(dev);
> 	}
>
> So a transmitter that already passed netif_xmit_stopped() can be inside
> ibmveth_start_xmit(), which copies into the LTB with no NULL check and no
> lock:
>
> 	memcpy(adapter->tx_ltb_ptr[queue_num], skb->data, skb_headlen(skb));
> 	...
> 		memcpy(adapter->tx_ltb_ptr[queue_num] + total_bytes,
> 		       skb_frag_address_safe(frag), skb_frag_size(frag));
> 	...
> 	desc.fields.address = adapter->tx_ltb_dma[queue_num];
>
> while ibmveth_free_tx_ltb() concurrently does:
>
> 	dma_unmap_single(...);
> 	adapter->tx_ltb_dma[idx] = 0;
> 	kfree(adapter->tx_ltb_ptr[idx]);
> 	adapter->tx_ltb_ptr[idx] = NULL;
>
> That looks like either a write of up to tx_ltb_size bytes into freed heap
> memory, or a NULL dereference on the cleared pointer.
>
> A second question on the same move: the DMA unmap now happens before the
> h_free_logical_lan() loop below, so a racing ibmveth_send() can hand PHYP
> a descriptor whose address is 0 or an already unmapped TCE while the
> logical LAN is still registered.  Previously the TX LTBs were freed at the
> very end of close(), after h_free_logical_lan(), free_irq()/napi_disable()
> and the buffer pool teardown, which incidentally left a large gap between
> the XOFF and the free.  Was the inversion intended?
>
> Would keeping ibmveth_free_tx_resources() after the h_free_logical_lan()
> loop, switching to netif_tx_disable(), and/or adding an
>
> 	if (unlikely(!adapter->tx_ltb_ptr[queue_num]))
>
> bail-out in ibmveth_start_xmit() be preferable?

Agreed. This is the main correctness issue in the patch. In v5 I will
quiesce TX before freeing the LTB state and move the TX free back after
`h_free_logical_lan()`. I will also add the `!tx_ltb_ptr[q]` guard in
`ibmveth_start_xmit()`, but I think it reads better as the answer to
your open-side mirror-window point below.


>   The last one would also
> cover the mirror window that the new open() ordering creates, where the
> LAN is registered and RX interrupts/NAPI are live while tx_ltb_ptr[] is
> still NULL during the sleeping GFP_KERNEL allocation in
> ibmveth_alloc_tx_resources():
>
> 	ibmveth_schedule_rx_queue(adapter, 0);
>
> 	rc = ibmveth_alloc_tx_resources(adapter);

Yes, and that is how I plan to cover the open-side window for this
series. `netif_tx_start_all_queues()` still runs only after
`ibmveth_alloc_tx_resources()` succeeds, and the `!tx_ltb_ptr[q]` guard
in `ibmveth_start_xmit()` closes the remaining mirror case without
needing to reorder open in this patch.

>
> [Severity: Low]
> The comment about PHYP mask, napi_disable and free_irq now sits above
> ibmveth_free_tx_resources() instead of above the
> ibmveth_cleanup_rx_interrupts() call it describes, so it reads as
> documentation of the TX free.  Could it move below the TX free?
Agreed. I will clean up that misplaced comment so the code and

commentary match.

Thanks,
Mingming

>
>>   
>>   	do {
> [ ... ]
>
>> -	for (i = 0; i < netdev->real_num_tx_queues; i++)
>> -		ibmveth_free_tx_ltb(adapter, i);
>> -
>>   	netdev_dbg(netdev, "close complete\n");
>>   
>>   	return 0;