Re: [PATCH net-next v5 01/15] ibmveth: Add MQ RX hypercall wrappers and call definitions

[Jakub Kicinski <kuba@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.
---
ibmveth: Add MQ RX hypercall wrappers and call definitions

This adds the PHYP call IDs for H_REG_LOGICAL_LAN_QUEUE,
H_ADD_LOGICAL_LAN_BUFFERS_QUEUE and H_FREE_LOGICAL_LAN_QUEUE, raising
MAX_HCALL_OPCODE accordingly, and adds matching static inline wrappers in
ibmveth.h.  It is plumbing only, with no runtime behaviour change until a
later commit enables multi_queue.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
> index d87713668ed30..c0ef03cad9b9a 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.h
> +++ b/drivers/net/ethernet/ibm/ibmveth.h
> @@ -66,6 +66,142 @@ static inline long h_add_logical_lan_buffers(unsigned long unit_address,

[ ... ]

> +/**
> + * h_add_logical_lan_buffers_queue - Add buffers to subordinate queue
> + * @unit_address: Device unit address
> + * @queue_handle: Queue handle from h_register_logical_lan_queue()

[Severity: Low]
This isn't a bug, but do these two kernel-doc blocks in the same header
contradict each other about which handles are valid here?

h_add_logical_lan_buffers_queue() is documented as "Add buffers to
subordinate queue" with "@queue_handle: Queue handle from
h_register_logical_lan_queue()", while h_register_logical_lan_with_handle()
below says its queue 0 handle:

 * Registers the primary receive queue (queue 0) with the hypervisor and
 * returns the queue handle. This is needed in multi-queue mode to use
 * h_add_logical_lan_buffers_queue() for all queues including queue 0.

By the end of the series the second wording is the one that matches the
code: ibmveth_add_logical_lan_buffers() in
drivers/net/ethernet/ibm/ibmveth.c passes
adapter->queue_handle[queue_index] for every index, queue_index == 0
included, and that handle comes from h_register_logical_lan_with_handle().

Would it be clearer to widen the add-buffers kdoc to say the handle may
come from either h_register_logical_lan_queue() or
h_register_logical_lan_with_handle(), so a future caller doesn't conclude
queue 0 must use the legacy buffer hcalls?

> + * @buffersznum: Buffer size (upper 32 bits) | count (lower 32 bits)
> + * @ioba12: Buffer addresses 1 and 2 packed ((addr1 << 32) | addr2)
> + * @ioba34: Buffer addresses 3 and 4 packed
> + * @ioba56: Buffer addresses 5 and 6 packed
> + * @ioba78: Buffer addresses 7 and 8 packed
> + * @ioba910: Buffer addresses 9 and 10 packed
> + * @ioba1112: Buffer addresses 11 and 12 packed
> + *
> + * Return:
> + *   H_SUCCESS - All buffers added successfully
> + *   H_PARAMETER - Invalid parameters
> + *   H_HARDWARE - Hardware error
> + */
> +static inline long h_add_logical_lan_buffers_queue(unsigned long unit_address,
> +						   unsigned long queue_handle,

[ ... ]

> +/**
> + * h_register_logical_lan_with_handle - Register primary queue and get handle
> + * @unit_address: Device unit address
> + * @buffer_list: DMA address of buffer list
> + * @rec_queue: Buffer descriptor of receive queue
> + * @filter_list: DMA address of filter list
> + * @mac_address: MAC address
> + * @queue_handle: Output parameter for queue handle (may be NULL)
> + *
> + * Registers the primary receive queue (queue 0) with the hypervisor and
> + * returns the queue handle. This is needed in multi-queue mode to use
> + * h_add_logical_lan_buffers_queue() for all queues including queue 0.
> + *
> + * Return: H_SUCCESS (0) on success, error code otherwise
> + */

[ ... ]

Cross-instance finding from sashiko-gemini (34d1289184ae332db8c101e251d3dec96e9969f2e8dee8d98c3efff05039c9d0):
[Severity: High]
Calling plpar_hcall9() with missing arguments causes an out-of-bounds stack read, leaking uninitialized kernel stack memory to the hypervisor.
-- 
pw-bot: cr