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

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

On 11/12/26 3:57 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 3 · Low: 0 (2)
>
> 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).
>
> - [Medium] The patch is titled 'Refactor', but it also carries the only
>    fix for a NULL-pointer dereference that already exists in the base…
> - [Medium] ibmveth_open() now allocates the TX LTBs (line 1432) after
>    h_register_logical_lan, ibmveth_alloc_buffer_pools(),…
> - [Medium] The patch makes a non-NULL tx_ltb_ptr[idx] mean 'this LTB is
>    usable'.
>
> Pre-existing issues:
> - [Low] Not a real bug for the scenario described (baseline decision:
>    false).
> - [Low] Resolved later in the series (baseline decision: false).
>
> --
>
> ibmveth: Refactor TX resource allocation in open/close paths
>
> This patch moves TX LTB allocation and freeing in open() and close() into
> the new ibmveth_alloc_tx_resources() and ibmveth_free_tx_resources()
> helpers. It also moves the TX allocation to the end of open(), after RX
> is live. It adds netif_tx_disable() at the start of open(), and adds a
> check in ibmveth_start_xmit() that drops packets when tx_ltb_ptr[] is
> NULL.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 62e6c9a9f5e91..cab895cc970c5 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> +/* Caller must ensure tx_ltb_ptr[idx] is NULL. open() runs on
>> + * probe-zeroed slots; set_channels() skips populated indices.
>> + */
>>   static int ibmveth_allocate_tx_ltb(struct ibmveth_adapter *adapter, int idx)
>>   {
>>   	adapter->tx_ltb_ptr[idx] = kzalloc(adapter->tx_ltb_size,
>> @@ -1277,12 +1287,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;
>>   	}
> [Severity: Medium]
> ibmveth_free_tx_ltb() now takes a non-NULL tx_ltb_ptr[idx] to mean the
> LTB is usable, and the new check in ibmveth_start_xmit() depends on that.
> Does ibmveth_allocate_tx_ltb() follow the same rule?
>
> It stores the kzalloc() result in tx_ltb_ptr[idx] before tx_ltb_dma[idx]
> is mapped. On the map failure path it calls kfree() before it clears the
> slot.
>
> The stale sender that the new guard is meant to catch could hit these
> windows on a later grow. After a shrink, netif_tx_wake_all_queues()
> leaves the freed index j without DRV_XOFF. If ethtool -L then grows the
> queues, ibmveth_set_channels() does this:
>
> 	netif_tx_stop_all_queues(netdev);
>
> 	/* Allocate any queue that we need */
> 	for (i = old; i < goal; i++) {
> 		...
> 		rc = ibmveth_allocate_tx_ltb(adapter, i);
>
> The grow path has no synchronize_net(). A stale sender that already
> passed the DRV_XOFF check could see a non-NULL tx_ltb_ptr[j] and
> memcpy() into it. It could then put tx_ltb_dma[j] == 0 into the
> H_SEND_LOGICAL_LAN descriptor, or write into memory that the map failure
> path is about to kfree().
>
> The grow-failure free loop in ibmveth_set_channels() also frees the slots
> it just allocated without waiting for a grace period.
>
> Would it be safer to allocate and map into local variables, and publish
> tx_ltb_ptr[idx] last with WRITE_ONCE() or a release store? On failure,
> the slot could be cleared before kfree(), and ibmveth_start_xmit() could
> read the pointer and the DMA address once each.
>
> [ ... ]
That publish-before-map ordering was already there. This patch doesn't
touch the allocation order or the ethtool -L TX grow loop; it only
clears tx_ltb_dma[] on the map-failure path and in the free helper. So
in net-next today, a stale sender after a shrink can hit a NULL
tx_ltb_ptr[], and one during a grow can hit the window you describe.
The new check turns the first case into a drop, but it doesn't try to
fix the grow window.

Your suggestion is the right fix: allocate and map into locals, publish
the pointer only once the DMA address is valid, clear it before freeing
on failure, and read the pointer and DMA address once in xmit. I'd
rather do that as a follow-up to the TX resize code than grow this
series.
>> @@ -1373,26 +1424,36 @@ static int ibmveth_open(struct net_device *netdev)
>>   	 */
>>   	rc = ibmveth_setup_rx_interrupts(adapter);
>>   	if (rc)
>> -		goto out_free_buffer_pools;
>> +		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;
> [Severity: Medium]
> Can a TX LTB allocation failure here now free RX buffers that PHYP still
> owns?
>
> By this point ibmveth_schedule_rx_queue(adapter, 0) has posted RX skb DMA
> addresses to the hypervisor. If kzalloc() or dma_map_single() fails in
> ibmveth_allocate_tx_ltb(), open() jumps to out_cleanup_rx_interrupts and
> falls through into:
>
> 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_queue_mem:
> 	ibmveth_cleanup_rx_resources(adapter);
>
> Any H_FREE_LOGICAL_LAN result other than busy is ignored. The posted RX
> skbs are then unmapped and freed, and so are the RX ring and the buffer
> list pages. If the H_FREE failed, could PHYP still DMA inbound frames, or
> the buffer list drop counter, into pages already returned to the
> allocator?
>
> Before this patch, TX allocation ran before LAN registration, so only
> close() had this window.
>
> The commit message says a later patch keeps this memory mapped on a hard
> failure. At the end of the series, though, ibmveth_free_all_queues() only
> returns -EIO in multi-queue mode:
>
> 	if (lpar_rc != H_PARAMETER && adapter->multi_queue)
> 		return -EIO;
>
> so single-queue mode would still free the posted buffers on this path.
>
> Does TX LTB allocation need to move after the replenish kick? Since
> ibmveth_alloc_tx_resources() unwinds its own partial failure, it could
> probably stay before h_register_logical_lan(), or at least before
> ibmveth_schedule_rx_queue().
>
> [ ... ]
Only if two things go wrong in the same open(): the TX LTB allocation
fails after the RX buffers are posted, and then H_FREE_LOGICAL_LAN
returns a hard failure. In multi-queue mode (patch 10 onward)
ibmveth_free_all_queues() returns -EIO and open() leaves the LAN memory
mapped, so the next open() or remove() can retry the free. Single-queue
mode still does what close() has always done: log the H_FREE failure
and unmap.

Moving the TX allocation ahead of h_register_logical_lan() would close
this particular window. I didn't want to reshuffle open() again for a
case that needs both a TX allocation failure and a hard H_FREE failure.
The single-queue hard-H_FREE policy is something I plan to fix
separately.
>> @@ -1864,6 +1923,14 @@ static netdev_tx_t ibmveth_start_xmit(struct sk_buff *skb,
>>   	int i, queue_num = skb_get_queue_mapping(skb);
>>   	unsigned long mss = 0;
>>   
>> +	/* TX LTBs are freed on shrink; a sender that picked its queue
>> +	 * before the shrink can get here after the free.
>> +	 */
>> +	if (unlikely(!adapter->tx_ltb_ptr[queue_num])) {
> [Severity: Medium]
> Does this check fix a NULL pointer dereference that is already in the
> base tree?
>
> On a TX shrink, ibmveth_set_channels() frees the dropped LTBs and then
> wakes every queue, including the freed indices:
>
> 	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);
>
> Some senders can still have a queue_mapping that points at a freed
> index. One is AF_PACKET with PACKET_QDISC_BYPASS, preempted between
> packet_pick_tx_queue() and dev_direct_xmit(). Another is pktgen reusing
> a cloned skb. In the old ibmveth_start_xmit(), such a sender reaches the
> memcpy() into adapter->tx_ltb_ptr[queue_num] with a NULL pointer.
>
> If so, could this check be sent as a separate minimal patch for net,
> with a tag like the following?
>
> Fixes: 10c2aba89cc0 ("ibmveth: Ethtool set queue support")
>
> Right now it sits inside a refactor in the middle of a net-next series,
> along with the open() reorder and the RX helper dependencies, so it
> can't be backported on its own.
Yes, though the trigger is narrow. Normal stack TX is drained by the
synchronize_net() in the shrink path. What's left is senders holding a
stale queue mapping that bypass that path, e.g. PACKET_QDISC_BYPASS or
pktgen reusing an skb after ethtool -L removed the queue.

I kept the check in this patch because it's where start_xmit() learns
to cope with a missing LTB, and the later MQ patches build on that. If
the maintainers want it for stable, I can peel it out as a minimal net
fix with the fix tag.
> Also, the commit message says:
>
>    a sender already in xmit during the free is not covered, as before
>    this series.
>
> Isn't that sender already drained on the shrink path? When queues are
> being disabled, netif_set_real_num_tx_queues() calls synchronize_net(),
> and ibmveth_set_channels() calls it before the free loop. What remains is
> a stale sender that arrives after netif_tx_wake_all_queues(), and that
> is the case this check catches.
Yes. netif_set_real_num_tx_queues() runs before the LTB free loop and
synchronizes when queues are removed. What's left is a stale sender
that shows up after the later wake. The commit message sentence about
an already-in-xmit sender not being covered is too broad; I'll fix it
in the follow-up.

Thanks,
Mingming