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?
