On 7/14/26 5:55 AM, Simon Horman 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.
Full review at: https://sashiko.dev
---
ibmveth: Enable multi-queue RX receive path
This is the first patch that sets multi_queue from H_ILLAN_ATTRIBUTES
and switches registration, buffer posting, and receive to the MQ
hcall path. It also raises num_rx_queues and enables per-queue NAPI.
diff --git a/drivers/net/ethernet/ibm/ibmveth.c
b/drivers/net/ethernet/ibm/ibmveth.c
index b3b3886c3eedb..863e5c68b42c7 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
@@ -826,15 +841,23 @@ static void ibmveth_replenish_buffer_pool(struct
ibmveth_adapter *adapter,
*/
static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
{
- __be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
+ int i;
+
+ for (i = 0; i < adapter->num_rx_queues; i++) {
+ __be64 *p = adapter->buffer_list_addr[i] + 4096 - 8;
+ u64 drops = be64_to_cpup(p);
- adapter->rx_no_buffer = be64_to_cpup(p);
+ if (i == 0)
+ adapter->rx_no_buffer = drops;
+ }
}
[Severity: Medium]
Will this logic silently discard the hardware drop counts for all queues
other than queue 0?
Since ethtool -S reports the global adapter->rx_no_buffer statistic, shouldn't
this loop accumulate the drops across all queues instead of only recording
them when evaluating the first queue?
[ ... ]
*
Good catch. Per-queue drops are already in rx_qstats
once that patch lands; the global rx_no_buffer still mirrors queue 0
only. I'll sum across queues into rx_no_buffer in v4.
*
@@ -842,6 +865,8 @@ static void ibmveth_replenish_task(struct ibmveth_adapter
*adapter,
int queue_index)
{
+ struct ibmveth_rx_q *rxq = &adapter->rx_queue[queue_index];
+ unsigned long flags;
int i;
if (queue_index >= adapter->num_rx_queues)
@@ -842,6 +865,8 @@ static void ibmveth_replenish_task(struct ibmveth_adapter
*adapter,
adapter->replenish_task_cycles++;
+ spin_lock_irqsave(&rxq->replenish_lock, flags);
+
[Severity: Medium]
With multi-queue RX enabled, can multiple NAPI threads execute this task
concurrently for different queues?
If so, does this non-atomic increment of the global replenish_task_cycles
stat risk data corruption, torn reads on 32-bit platforms, or lost updates?
Similarly, in ibmveth_replenish_buffer_pool():
drivers/net/ethernet/ibm/ibmveth.c:ibmveth_replenish_buffer_pool() {
...
buffers_added += filled;
adapter->replenish_add_buff_success += filled;
remaining -= filled;
...
}
Even though the caller acquires the per-queue rxq->replenish_lock,
replenish_add_buff_success is a global adapter statistic. Does this update
also race with other CPUs holding the locks for their respective queues?
[ ... ]
*Same as on the buffer-submit patch: these are informational counters only.*
*replenish_lock serializes replenishment for a single RX queue, but it
does not *
*protect the adapter-wide counters across multiple queues.
*
*However, these counters are used only for diagnostics and observability, *
*not for correctness, synchronization, or resource management.While
stricter *
*accounting could be achieved with atomic operations, these counters are *
*updated in a hot path and the additional synchronization overhead is
difficult *
*to justify for diagnostic-only statistics, particularly on larger
systems with *
*many RX queues. If more accurate accounting is desired, a per-queue *
*statistics model would likely be a better follow-up approach, *
@@ -2268,34 +2299,40 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
static int ibmveth_poll(struct napi_struct *napi, int budget)
{
[ ... ]
} else {
struct sk_buff *skb, *new_skb;
- int length = ibmveth_rxq_frame_length(adapter);
- int offset = ibmveth_rxq_frame_offset(adapter);
- int csum_good = ibmveth_rxq_csum_good(adapter);
- int lrg_pkt = ibmveth_rxq_large_packet(adapter);
+ int length = ibmveth_rxq_frame_length(adapter,
queue_index);
+ int offset = ibmveth_rxq_frame_offset(adapter,
queue_index);
+ int csum_good = ibmveth_rxq_csum_good(adapter,
queue_index);
+ int lrg_pkt = ibmveth_rxq_large_packet(adapter,
queue_index);
[Severity: High]
This isn't a bug introduced by this patch, but does this code validate the
length and offset provided by the hardware descriptor before modifying the
SKB bounds?
Later in ibmveth_poll(), these values are used directly:
drivers/net/ethernet/ibm/ibmveth.c:ibmveth_poll() {
...
skb_reserve(skb, offset);
skb_put(skb, length);
...
}
If the hypervisor provides an untrusted or artificially large length, could
the call to skb_put() trigger an skb_over_panic() and crash the guest
kernel?
[ ... ]
*Agreed, this needs guarding.I was already working on a follow-up
poll-hardening
**patch that adds correlator validation and verifies sufficient
skb_tailroom() before
calling skb_reserve() and skb_put(). I will keep those validations in v4.
If preferred for bisect safety, I can also move the checks earlier in
the series rather
than introducing them only in the final hardening patch.*
Also, I noticed a potential memory leak in the error unwinding path of
ibmveth_open():
drivers/net/ethernet/ibm/ibmveth.c:ibmveth_open() {
...
if (rc) {
netdev_err(netdev, "failed to set number of rx queues\n");
goto out_unregister_queues;
}
...
out_free_buffer_pools:
ibmveth_free_buffer_pools(adapter);
out_unregister_queues:
ibmveth_dispose_subordinate_irq_mappings(adapter);
ibmveth_free_all_queues(adapter);
}
[Severity: Medium]
This is a pre-existing issue, but if netif_set_real_num_rx_queues() fails,
the code jumps to the out_unregister_queues label.
Does this bypass out_free_buffer_pools, leaving the RX buffer pools unfreed
and causing a memory leak when cleaning up after a failure?
*
Yes — same open() unwind issue called out on the open/close
refactor patch. Will fix the label order in v4 so buffer pools
are always freed after unregister.
**
Thanks,
Mingming
*
