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

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

On 8/17/26 6:47 PM, 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()/ibmveth_close() into ibmveth_alloc_tx_resources() and
> ibmveth_free_tx_resources(), moves the TX allocation to the end of open()
> after RX is live, and adds a tx_ltb_ptr[] presence check to
> ibmveth_start_xmit().  No Link: tags are present in the commit header.
>
> A few questions on the LTB lifetime rules the patch establishes.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 99eeb6ef51bf3..b39e8c53cbfd1 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -1183,8 +1183,12 @@ static int ibmveth_rxq_harvest_buffer(struct ibmveth_adapter *adapter,
>>   
>>   static void ibmveth_free_tx_ltb(struct ibmveth_adapter *adapter, int idx)
>>   {
>> +	if (!adapter->tx_ltb_ptr[idx])
>> +		return;
>> +
>>   	dma_unmap_single(&adapter->vdev->dev, adapter->tx_ltb_dma[idx],
>>   			 adapter->tx_ltb_size, DMA_TO_DEVICE);
>> +	adapter->tx_ltb_dma[idx] = 0;
>>   	kfree(adapter->tx_ltb_ptr[idx]);
>>   	adapter->tx_ltb_ptr[idx] = NULL;
>>   }
> [Severity: Medium]
> tx_ltb_ptr[] now doubles as the guard for tx_ltb_dma[], but the retire
> order here clears the guarded value first:
>
> 	dma_unmap_single(...);
> 	adapter->tx_ltb_dma[idx] = 0;
> 	kfree(adapter->tx_ltb_ptr[idx]);
> 	adapter->tx_ltb_ptr[idx] = NULL;
>
> Can an ibmveth_start_xmit() that already passed the new check
>
> 	if (unlikely(!adapter->tx_ltb_ptr[queue_num])) {
>
> then reach
>
> 	desc.fields.address = adapter->tx_ltb_dma[queue_num];
>
> and hand a zeroed DMA address to H_SEND_LOGICAL_LAN?
>
> The publish side has the mirror-image order: ibmveth_allocate_tx_ltb()
> stores tx_ltb_ptr[idx] from kzalloc() before tx_ltb_dma[idx] is filled in
> by dma_map_single(), with no release/acquire pairing.
>
> Both the check in ibmveth_start_xmit() and its later uses (the memcpy into
> tx_ltb_ptr[queue_num] and the descriptor address read) are plain loads with
> no READ_ONCE and no common lock, so nothing prevents a reload of
> tx_ltb_ptr[queue_num] after it was set to NULL.
>
> Against ibmveth_close() this appears harmless, since ibmveth does not set
> lltx and netif_tx_disable() therefore takes every __netif_tx_lock.  Against
> ibmveth_set_channels() (more below) there is no such serialization.  Would a
> real lock, or gating on adapter->opened, express this better than a pointer
> used as a flag?
close() is safe: no lltx, so netif_tx_disable() waits out xmit. The
-L shrink window is real; answered below. v6 clears both slots before
unmap/free so a later start_xmit() fails the pointer check.

A lock or opened gate in start_xmit() would not replace that. opened
is false after a failed reopen, when the LTBs are already gone; the
-L race runs while opened is true. The right fix there is
netif_tx_disable() on the shrink path, not another flag on xmit.

>> @@ -1207,12 +1211,54 @@ static int ibmveth_allocate_tx_ltb(struct ibmveth_adapter *adapter, int idx)
>>   			   "unable to DMA map tx long term buffer\n");
>>   		kfree(adapter->tx_ltb_ptr[idx]);
>>   		adapter->tx_ltb_ptr[idx] = NULL;
>> +		adapter->tx_ltb_dma[idx] = 0;
>>   		return -ENOMEM;
>>   	}
>>   
>>   	return 0;
>>   }
>>   
>> +/**
>> + * ibmveth_alloc_tx_resources - Allocate TX resources for all queues
>> + * @adapter: ibmveth adapter structure
>> + *
>> + * Allocates TX Long Term Buffers (LTBs) for all TX queues.
>> + *
>> + * Return: 0 on success, -ENOMEM on failure
>> + */
>> +static int ibmveth_alloc_tx_resources(struct ibmveth_adapter *adapter)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +	int i;
>> +
>> +	for (i = 0; i < netdev->real_num_tx_queues; i++) {
>> +		if (ibmveth_allocate_tx_ltb(adapter, i))
>> +			goto err_free_ltbs;
>> +	}
> [Severity: Low]
> This isn't a bug introduced by this patch, and it looks unreachable by the
> end of the series, but noting it for traceability: ibmveth_allocate_tx_ltb()
> assigns the kzalloc() and dma_map_single() results straight into
> tx_ltb_ptr[i] / tx_ltb_dma[i] with no check for an already-populated slot,
> unlike ibmveth_set_channels() which does
>
> 	if (adapter->tx_ltb_ptr[i])
> 		continue;
>
> so a caller reaching this helper with a live mapping in the slot would
> strand the old TCE mapping.  With the later set_channels rewrite in this
> series the !adapter->opened branch allocates nothing and the opened branch
> frees everything it allocated on failure, so no path leaves tx_ltb_ptr[]
> populated while adapter->opened is false and open() cannot overwrite a live
> mapping.  Would a defensive check (or a comment stating the invariant) be
> worth it here?
A comment. A defensive skip would hide a caller that left a live
mapping.
>> +
>> +	return 0;
>> +
>> +err_free_ltbs:
>> +	while (--i >= 0)
>> +		ibmveth_free_tx_ltb(adapter, i);
>> +	return -ENOMEM;
>> +}
>> +
>> +/**
>> + * ibmveth_free_tx_resources - Free TX resources for all queues
>> + * @adapter: ibmveth adapter structure
>> + *
>> + * Frees TX Long Term Buffers (LTBs) for all TX queues.
>> + */
>> +static void ibmveth_free_tx_resources(struct ibmveth_adapter *adapter)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +	int i;
>> +
>> +	for (i = 0; i < netdev->real_num_tx_queues; i++)
>> +		ibmveth_free_tx_ltb(adapter, i);
>> +}
> [Severity: Low]
> Small wording mismatch: the kernel-doc says "Frees TX Long Term Buffers
> (LTBs) for all TX queues", but the loop is bounded by real_num_tx_queues
> while tx_ltb_ptr[] is IBMVETH_MAX_QUEUES wide.  Since ibmveth_free_tx_ltb()
> is now NULL-safe, either iterating IBMVETH_MAX_QUEUES or saying
> "real_num_tx_queues" in the comment would make the two agree.
The kdoc now says real_num_tx_queues.
>> @@ -1263,12 +1309,6 @@ static int ibmveth_open(struct net_device *netdev)
> [ ... ]
>
>>   	rc = ibmveth_alloc_buffer_pools(adapter);
>>   	if (rc)
>> -		goto out_free_tx_ltb;
>> +		goto out_unregister_lan;
>>   
>>   	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;
> [ ... ]
>
>> -out_free_buffer_pools:
>> +out_cleanup_rx_interrupts:
>> +	ibmveth_cleanup_rx_interrupts(adapter);
>> +out_unregister_lan:
>> +	do {
>> +		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>> +	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>>   	ibmveth_free_buffer_pools(adapter);
>> -out_free_tx_ltb:
>> -	while (--i >= 0)
>> -		ibmveth_free_tx_ltb(adapter, i);
>> +out_free_queue_mem:
>>   	ibmveth_cleanup_rx_resources(adapter);
> [Severity: Medium]
> This isn't a bug introduced by this patch, but is the changelog wording
> accurate about what changed here?  The commit message says:
>
>    "After LAN registration, open-fail teardown frees the logical LAN before
>    tearing down RX pool DMA (intentional safer order than leaving the LAN
>    registered while unmapping RX memory)."
>
> At the parent commit, an ibmveth_alloc_buffer_pools() failure after a
> successful ibmveth_register_logical_lan() went
>
> 	rc = ibmveth_alloc_buffer_pools(adapter);
> 	if (rc)
> 		goto out_free_tx_ltb;
>
> and out_free_tx_ltb fell through to ibmveth_cleanup_rx_resources() and
> ibmveth_free_filter_list() with no h_free_logical_lan() anywhere on that
> path, so the RX queue and buffer/filter lists were unmapped and freed while
> PHYP still had the LAN registered.  Post-patch that failure goes to
> out_unregister_lan, which issues the deregistration hcall first.
>
> So this looks less like a reordering and more like adding a hcall that was
> never issued on that path.  Should this be split out as its own patch with a
> Fixes: tag so it can be picked up for stable, rather than described as an
> ordering preference inside a TX-only refactor?

Yes. That path never issued the hcall; v6 says so.

The tag would be:

   Fixes: d43732ce021f ("ibmveth: properly unwind on init errors")

Same d43732ce021f origin as patch 3 — one standalone after this
series covers both. The posting stays 15.

>> @@ -1343,7 +1385,10 @@ static int ibmveth_close(struct net_device *netdev)
> [ ... ]
>
>> -	netif_tx_stop_all_queues(netdev);
>> +	/* Disable and wait for in-flight ndo_start_xmit (stop_all_queues
>> +	 * alone does not). Direct close() callers bypass synchronize_net().
>> +	 */
>> +	netif_tx_disable(netdev);
> [Severity: High]
> This isn't a bug introduced by this patch, but the reasoning in this new
> comment seems to apply equally to ibmveth_set_channels(), which still frees
> the same LTBs after only netif_tx_stop_all_queues():
>
> ibmveth_set_channels() {
> 	...
> 	netif_tx_stop_all_queues(netdev);
> 	...
> 	/* Free any that are no longer needed */
> 	for (i = old; i > goal; i--) {
> 		if (adapter->tx_ltb_ptr[i - 1])
> 			ibmveth_free_tx_ltb(adapter, i - 1);
> 	}
>
> 	netif_tx_wake_all_queues(netdev);
> }
>
> netif_tx_stop_all_queues() only sets __QUEUE_STATE_DRV_XOFF; it takes no
> __netif_tx_lock, and netif_set_real_num_tx_queues() only touches the qdisc
> root lock.  Since ibmveth does not declare lltx, an ndo_start_xmit already
> running on another CPU holds the txq lock and keeps going:
>
> CPU0 ibmveth_start_xmit()
> 	/* already passed !adapter->tx_ltb_ptr[queue_num] */
> 	memcpy(adapter->tx_ltb_ptr[queue_num], skb->data, skb_headlen(skb));
> 	...
> 	desc.fields.address = adapter->tx_ltb_dma[queue_num];
>
> CPU1 ibmveth_set_channels()
> 	ibmveth_free_tx_ltb(adapter, i - 1);
> 	  dma_unmap_single(...);
> 	  adapter->tx_ltb_dma[idx] = 0;
> 	  kfree(adapter->tx_ltb_ptr[idx]);
>
> Can an ethtool -L shrink therefore memcpy into kfree()d memory and pass a
> stale or zeroed DMA address to H_SEND_LOGICAL_LAN?  The new one-shot
> tx_ltb_ptr[] check in ibmveth_start_xmit() does not close this window, and
> this stop-then-free sequence is unchanged at the end of the series.  Should
> ibmveth_set_channels() use netif_tx_disable() too?

Yes. close() already uses netif_tx_disable(). The shrink path stays
stop-then-free here and at series tip. Patches 14 and 15 switch the
IFF_UP vs opened gate; they do not add disable.

>> @@ -1359,13 +1404,14 @@ static int ibmveth_close(struct net_device *netdev)
> [ ... ]
>
>> @@ -1789,6 +1835,12 @@ static netdev_tx_t ibmveth_start_xmit(struct sk_buff *skb,
>>   	int i, queue_num = skb_get_queue_mapping(skb);
>>   	unsigned long mss = 0;
>>   
>> +	/* Close / failed reopen can free LTBs while IFF_UP is still set. */
>> +	if (unlikely(!adapter->tx_ltb_ptr[queue_num])) {
>> +		dev_kfree_skb_any(skb);
>> +		return NETDEV_TX_OK;
>> +	}
>> +
> [Severity: Medium]
> Should this drop bump netdev->stats.tx_dropped like every other drop in this
> function does?
>
> 	if (unlikely(skb->len > adapter->tx_ltb_size)) {
> 		...
> 		netdev->stats.tx_dropped++;
> 		goto out;
> 	}
>
> The other drop sites (ibmveth_is_packet_unsupported(), the checksum-help
> failure, the oversize check, the total_bytes mismatch and the
> ibmveth_send() failure) all increment tx_dropped, and returning
> NETDEV_TX_OK tells the core the skb was consumed so the core cannot account
> for it either.  In exactly the abnormal states this guard targets, traffic
> is then discarded with nothing visible in ip -s link or in
> /sys/class/net/<if>/statistics/tx_dropped.  Could this reuse the existing
> out: label after the tx_dropped++ instead?
Yes. v6 increments tx_dropped and falls into out:.
> [Severity: Low]
> This isn't a bug introduced by this patch, and it looks resolved later in
> the series, but noting it for traceability against the claim in the commit
> message that "close/failed-reopen with IFF_UP set cannot UAF".
>
> The guard keys off tx_ltb_ptr[] rather than adapter->opened, while
> ibmveth_set_channels() at this point in the series still selects its live
> path from IFF_UP alone:
>
> 	if (!(netdev->flags & IFF_UP))
> 		return netif_set_real_num_tx_queues(netdev, goal);
>
> In the state the driver documents in ibmveth_close() (IFF_UP set,
> adapter->opened false after a failed reopen), ethtool -L would allocate LTBs
> and finish with netif_tx_wake_all_queues() on an adapter whose logical LAN
> was already released, so packets pass this pointer-only check and reach
> ibmveth_send() with no registered LAN, and those LTBs are not freed by a
> later close() because it early-returns on !adapter->opened.  The later patch
> "ibmveth: Wire ethtool set_channels to MQ RX queue resize" replaces the
> IFF_UP gating with an adapter->opened test whose !opened branch allocates
> nothing and wakes no queues, which removes this window.
The v5 claim was too broad. NULL-first only closes the check-then-use
window; close-path safety is netif_tx_disable(). The IFF_UP vs opened
window closes in patches 14 and 15.


Thanks,

Mingming