Re: [PATCH net-next v3 10/15] ibmveth: Enable multi-queue RX receive path

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.

quoted

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

[ ... ]

quoted

@@ -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.

*

quoted

@@ -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, *

quoted

@@ -2268,34 +2299,40 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
  
  static int ibmveth_poll(struct napi_struct *napi, int budget)
  {

[ ... ]

quoted

  		} 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

*