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

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

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

*