Re: [PATCH v2 08/13] sgi-xp: Use higher-level allocator

[sashiko-bot]

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

New issues:
- [Medium] Commit message fundamentally misunderstands and inverts the documented differences between __alloc_pages_node() and alloc_pages_node().

Pre-existing issues:
- [High] Logic Error / Variable Mismatch in xpc_init_mq_node() breaks CPU fallback and NUMA affinity.
- [High] Hardware IRQ enabled before GRU message queue descriptors are fully initialized leads to NULL pointer dereference on spurious interrupts.
--

commit 2717112c630ddd5adcf0324575c2741c6d3bf969
Author: Brendan Jackman <jackmanb@xxxxxxxxxx>

sgi-xp: Use higher-level allocator

This commit switches xpc_create_gru_mq_uv() to use alloc_pages_node() instead
of __alloc_pages_node(). The change is intended to replace an allocator
function that is being removed with one that handles similar requirements.

> sgi-xp: Use higher-level allocator
> 
> The difference between __alloc_pages_node() and alloc_pages_node() is
> that the former allows you to pass NUMA_NO_NODE.
> 
> The former is going away and the latter works fine here so switch over.

[Severity: Medium]
Is this description accurate?

__alloc_pages_node() strictly prohibits NUMA_NO_NODE, while
alloc_pages_node() safely handles it by falling back to numa_mem_id().

While the code change itself appears safe here since cpu_to_node() returns
a valid node, could the commit message be updated to reflect the correct
behavior of these memory allocation APIs?

> diff --git a/drivers/misc/sgi-xp/xpc_uv.c b/drivers/misc/sgi-xp/xpc_uv.c
> index 772c787268932..aacff70204241 100644
> --- a/drivers/misc/sgi-xp/xpc_uv.c
> +++ b/drivers/misc/sgi-xp/xpc_uv.c
> @@ -170,7 +170,7 @@ 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,
> +	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 the interrupt setup
that occurs further down in xpc_create_gru_mq_uv():

drivers/misc/sgi-xp/xpc_uv.c:xpc_create_gru_mq_uv() {
    ...
    ret = request_irq(mq->irq, irq_handler, 0, irq_name, NULL);
    ...
}

If a spurious interrupt occurs immediately after request_irq() is called,
could xpc_handle_activate_IRQ_uv() dereference the global xpc_activate_mq_uv
pointer before it is initialized?

drivers/misc/sgi-xp/xpc_uv.c:xpc_handle_activate_IRQ_uv() {
    ...
    msg_hdr = gru_get_next_message(xpc_activate_mq_uv->gru_mq_desc);
    ...
}

The global pointer doesn't seem to be assigned until xpc_create_gru_mq_uv()
returns to its caller.

[Severity: High]
This is a pre-existing issue, but looking at xpc_init_mq_node() which calls
xpc_create_gru_mq_uv():

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,
    ...
}

Does this loop inadvertently pass the nid variable instead of the cpu variable
to xpc_create_gru_mq_uv()?

This appears to bypass the CPU fallback iteration and could result in
querying uv_cpu_to_blade_id() and cpu_to_node() with a node index rather
than a CPU index.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-alloc-trylock-v2-0-31f31367d420@xxxxxxxxxx?part=8