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

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.

[ ... ]

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

[ ... ]

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

[ ... ]

[ ... ]

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