On 11/12/26 3:57 PM, [email protected] 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
