Hi, Thanks for your review..
On 9/3/26 11:10 AM, [email protected] 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
