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

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

Hi, Thanks for your review..

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (2)
>
> - [Medium] The patch silently repairs a live mainline bug while
>    presenting itself as a TX refactor.
>
> Pre-existing issues:
> - [Low] Concern (assessed as a FALSE POSITIVE): the new `if
>    (unlikely(!adapter->tx_ltb_ptr[queue_num]))` guard in…
> - [Low] Concern (assessed as a FALSE POSITIVE for the final series
>    state): ibmveth_alloc_tx_resources() calls ibmveth_allocate_tx_ltb()…
>
> --
>
> ibmveth: Refactor TX resource allocation in open/close paths
>
> This moves the per-queue TX LTB allocation and free out of ibmveth_open()
> and ibmveth_close() into ibmveth_alloc_tx_resources() and
> ibmveth_free_tx_resources(), moves the TX allocation to the end of open()
> after RX interrupt setup, and adds a tx_ltb_ptr[] check to
> ibmveth_start_xmit(). It also reworks the open() failure unwind labels.
>
> A few questions below.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 335712faaa42a..7a420e1a41d52 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -1201,12 +1201,27 @@ 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);
>> -	kfree(adapter->tx_ltb_ptr[idx]);
>> +	void *ltb = adapter->tx_ltb_ptr[idx];
>> +	dma_addr_t dma = adapter->tx_ltb_dma[idx];
>> +
>> +	if (!ltb)
>> +		return;
>> +
>> +	/*
>> +	 * Clear the slot before releasing it. start_xmit() tests
>> +	 * tx_ltb_ptr[idx] to decide whether the LTB is usable.
>> +	 */
>>   	adapter->tx_ltb_ptr[idx] = NULL;
>> +	adapter->tx_ltb_dma[idx] = 0;
>> +
>> +	dma_unmap_single(&adapter->vdev->dev, dma, adapter->tx_ltb_size,
>> +			 DMA_TO_DEVICE);
>> +	kfree(ltb);
>>   }
>>   
>> +/* 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,
> [Severity: Low]
> This is a pre-existing style point rather than something this patch breaks,
> but the new kdoc on ibmveth_allocate_tx_ltb() states an invariant that the
> helper itself never checks:
>
>    /* Caller must ensure tx_ltb_ptr[idx] is NULL. ...
>
> and ibmveth_alloc_tx_resources() below calls it unconditionally for every
> index in [0, real_num_tx_queues). If a slot were ever populated on entry,
> the kzalloc() would overwrite the old pointer and leak both the 64 KiB
> buffer and its streaming DMA mapping.
>
> Tracing the callers, the only way to reach open() with a populated slot was
> ibmveth_set_channels() gating on IFF_UP instead of adapter->opened, and a
> later patch in this series makes set_channels() return early on !opened
> before it touches the LTBs, so no reachable caller can present a populated
> index. Would a WARN_ON_ONCE(adapter->tx_ltb_ptr[idx]) (or just an early
> return) be worth adding so the documented precondition is enforced rather
> than only described?
A comment, not a WARN_ON or an early return, and
v6 added that comment. I am keeping it that way: a
skip would hide a caller that left a live mapping,
and still strand the old TCE.
>
> [ ... ]
>
>> @@ -1308,24 +1359,24 @@ 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;
>>   
>>   	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);
>>   
>>   	adapter->opened = true;
>> @@ -1333,11 +1384,14 @@ static int ibmveth_open(struct net_device *netdev)
>>   
>>   	return 0;
>>   
>> -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]
> Should the h_free_logical_lan() part of this hunk be split out as its own
> patch with a Fixes: tag and Cc: stable?
>
> Rerouting the ibmveth_alloc_buffer_pools() failure to out_unregister_lan
> looks like a real bug fix, not just a label rename. At the series merge base
> (1b78070aaef6) that failure path in ibmveth_open() runs:
>
>    rc = ibmveth_alloc_buffer_pools(adapter);
>    if (rc)
>            goto out_free_buffer_pools;
>    ...
>    out_free_buffer_pools:
>            while (--i >= 0) { ... }
>    out_unmap_filter_list:
>            dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>                             DMA_BIDIRECTIONAL);
>
> so the filter list, the buffer list and the RX completion ring are unmapped
> and freed while the logical LAN registered by ibmveth_register_logical_lan()
> is still live. Only the request_irq() failure path issued the hcall. This
> patch is the first place that path deregisters, which matches the commit
> message note:
>
>    "After LAN registration, open-fail teardown issues h_free_logical_lan()
>    before RX pool DMA teardown on the pool-fail path that previously never
>    issued that hcall (missing deregistration, not a preference reorder)."
>
> The concern is packaging rather than correctness of the code here. As
> written, the fix for an ifup-under-memory-pressure path that leaves PHYP
> registered against freed DMA memory is folded into a TX helper refactor with
> no Fixes: tag, no Cc: stable and no statement of the failure mode.
>
> The next patch in the series ("ibmveth: Add RX queue register helpers for
> MQ") then moves pool allocation ahead of registration, so this arrangement
> is transient inside the series and no single commit is backportable for the
> mainline/stable bug. Could the deregistration fix be lifted to a standalone
> patch at the front of the series?
That path never issued the hcall, and the tag
would be Fixes: d43732ce021f — the same defect as
Patch 3. I am leaving h_free on this unwind rather
than peeling a Fixes: patch out here, so this
posting stays at 15, and sending one standalone
[PATCH net] after the series. Happy to send that
unwind ahead of this 15 instead if you would
prefer it that way.

The commit message already names the missing
deregistration.
> [ ... ]
>
>> @@ -1807,6 +1864,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])) {
>> +		netdev->stats.tx_dropped++;
>> +		goto out;
>> +	}
>> +
>>   	if (ibmveth_is_packet_unsupported(skb, netdev))
>>   		goto out;
>>   	/* veth can't checksum offload UDP */
> [Severity: Low]
> This isn't a bug introduced by the patch, and after checking it appears to
> be safe, but the shape of the new guard invites the question. The check
> reads adapter->tx_ltb_ptr[queue_num] once, then the body re-reads the same
> slots later:
>
>    memcpy(adapter->tx_ltb_ptr[queue_num], skb->data, skb_headlen(skb));
>    ...
>    desc.fields.address = adapter->tx_ltb_dma[queue_num];
>
> Since ibmveth_free_tx_ltb() now NULLs the slot before the unmap, is the
> guard intended as a race barrier, or purely as a check for a slot that is
> already known to be quiesced?
>
> Every free site does look quiesced independently of this check:
> ibmveth_close() calls netif_tx_disable() plus synchronize_net() before
> ibmveth_free_tx_resources(); the ibmveth_set_channels() shrink frees only
> after netif_set_real_num_tx_queues(), which does synchronize_net() on the
> disabling path; and the open() unwind runs with all TX queues still XOFF.
>
> The commit message already says as much:
>
>    "NULL-first in free_tx_ltb() only closes the check-then-use window, it is
>    not itself a UAF barrier."
>
> Would it be clearer to state next to the guard that TX is quiesced at all
> free sites, so the single read is not load-bearing?


Not a race barrier. close() is safe:
netif_tx_disable()
waits out xmit. Shrink frees only after
netif_set_real_num_tx_queues(), which
synchronize_net()s on the disabling path. The
guard is for a slot that is already gone (close
/ failed reopen with IFF_UP still set).
NULL-first only closes the check-then-use
window; the v6 commit message already says it
is not a UAF barrier.

I am leaving out "all free sites are quiesced"
next to the guard; that is not load-bearing.

Thanks,
Mingming