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

[Simon Horman <horms@kernel.org>]

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?

[ ... ]

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

[ ... ]

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

[ ... ]

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?