Re: [PATCH v4 11/18] sgi-xp: Use higher-level allocator API

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] xpc_init_mq_node() incorrectly passes a NUMA node ID (nid) in place of a CPU ID to xpc_create_gru_mq_uv().
- [High] xpc_init_mq_node() fails to catch empty CPU masks, leaving global MQ pointers NULL and reporting false success.
--

commit 4df0c24c50fd7156f7d0c39d45d908827e5063f5
Author: Brendan Jackman <jackmanb@xxxxxxxxxx>

sgi-xp: Use higher-level allocator API

This patch updates the memory allocation in xpc_create_gru_mq_uv() by
replacing __alloc_pages_node() with alloc_pages_node(). It aligns with
the deprecation of the former while preserving the existing NUMA-aware
memory allocation behavior.

> diff --git a/drivers/misc/sgi-xp/xpc_uv.c b/drivers/misc/sgi-xp/xpc_uv.c
> index 772c787268932..79c2f00ed4d70 100644
> --- a/drivers/misc/sgi-xp/xpc_uv.c
> +++ b/drivers/misc/sgi-xp/xpc_uv.c
[ ... ]
> @@ -170,9 +170,8 @@ xpc_create_gru_mq_uv(unsigned int mq_size, int cpu, char *irq_name,
>  	mq->mmr_blade = uv_cpu_to_blade_id(cpu);
>  
>  	nid = cpu_to_node(cpu);
> -	page = __alloc_pages_node(nid,
> -				      GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE,
> -				      pg_order);
> +	page = alloc_pages_node(nid, GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE,
> +				pg_order);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how this function
is called from xpc_init_mq_node(), is the correct CPU being passed in?

drivers/misc/sgi-xp/xpc_uv.c:xpc_init_mq_node() {
	for_each_cpu(cpu, cpumask_of_node(nid)) {
		xpc_activate_mq_uv =
			xpc_create_gru_mq_uv(XPC_ACTIVATE_MQ_SIZE_UV, nid,
					     XPC_ACTIVATE_IRQ_NAME,
					     xpc_handle_activate_IRQ_uv);
        ...

It appears the loop invariant nid is passed as the cpu argument instead of
the cpu variable. Could this cause the message queue to be initialized
using the wrong physical topology resources, scrambling routing and IRQ
configuration since xpc_create_gru_mq_uv() uses the provided cpu argument for
uv_cpu_to_blade_id(), cpu_to_node(), and xpc_get_gru_mq_irq_uv()?

[Severity: High]
This is also a pre-existing issue, but does xpc_init_mq_node() properly
handle online NUMA nodes with no CPUs?

If the cpumask for the node is empty, the for_each_cpu() loop will not
execute:

drivers/misc/sgi-xp/xpc_uv.c:xpc_init_mq_node() {
	for_each_cpu(cpu, cpumask_of_node(nid)) {
		...
		if (!IS_ERR(xpc_activate_mq_uv))
			break;
	}
	if (IS_ERR(xpc_activate_mq_uv)) {
		cpus_read_unlock();
		return PTR_ERR(xpc_activate_mq_uv);
	}
    ...

Since xpc_activate_mq_uv is initially NULL, IS_ERR(NULL) evaluates to false.
Will this bypass the error check, report false success, and leave the
pointer NULL, leading to a panic when it is dereferenced later in the driver
lifecycle?

>  	if (page == NULL) {
>  		dev_err(xpc_part, "xpc_create_gru_mq_uv() failed to alloc %d "

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-alloc-trylock-v4-0-0af8ff387e80@xxxxxxxxxx?part=11