Re: [RFC PATCH v8 03/14] irqchip/gic-v3-its: Resolve the default NUMA node explicitly

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] irqchip/gic-v3-its: incomplete NUMA_NO_NODE resolution exposes allocations to task mempolicy
> --
>
> commit 6d762ab83972132164ae57c1422f1916decb2fe3
> Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>
>     irqchip/gic-v3-its: Resolve the default NUMA node explicitly
>     
>     alloc_pages_node() resolves NUMA_NO_NODE to numa_mem_id() internally. In
>     preparation for switching ITS allocations to the CoCo shared memory
>     allocator, resolve the default node explicitly in its_alloc_pages().
>     
>     The CoCo shared memory allocator interprets NUMA_NO_NODE as a request to
>     apply the current task's memory policy. Passing numa_mem_id() instead
>     preserves the existing nearest-memory-node placement across the
>     allocator switch. This patch does not change the current allocation
>     behavior.
>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -236,7 +237,7 @@ static struct page *its_alloc_pages_node(int node, gfp_t gfp,
>>  
>>  static struct page *its_alloc_pages(gfp_t gfp, unsigned int order)
>>  {
>> -	return its_alloc_pages_node(NUMA_NO_NODE, gfp, order);
>> +	return its_alloc_pages_node(numa_mem_id(), gfp, order);
>>  }
>
> [Severity: Medium]
> Will this leave other callers of its_alloc_pages_node() exposed to the
> task memory policy change?
>
> When a device probe or ITT allocation occurs with its->numa_node ==
> NUMA_NO_NODE (which is common when firmware lacks NUMA info), callers pass
> NUMA_NO_NODE directly to its_alloc_pages_node():
>
> drivers/irqchip/irq-gic-v3-its.c:its_alloc_pages_node() {
> 	...
> 	page = alloc_pages_node(node, gfp | gfp_flags_quirk, order);
> 	...
> }
>
> If node is NUMA_NO_NODE, it passes it directly to the underlying allocator.
> When the underlying allocator interprets NUMA_NO_NODE as a request to apply
> task mempolicy, these direct callers will be subjected to the current task's
> memory policy instead of bypassing it and using the local node. 
>
> If a user binds the driver via sysfs under a restrictive task mempolicy,
> the allocations could land on suboptimal distant nodes or fail entirely.
>
> Should the NUMA_NO_NODE resolution logic be moved inside its_alloc_pages_node()
> to ensure all callers are protected?

Yes, updated the patch to perform the mapping in its_alloc_pages_node(),
so all callers are handled.

@@ -216,6 +217,9 @@ static struct page *its_alloc_pages_node(int node, gfp_t gfp,
 	struct page *page;
 	int ret = 0;
 
+	if (node == NUMA_NO_NODE)
+		node = numa_mem_id();
+
 	page = alloc_pages_node(node, gfp | gfp_flags_quirk, order);
 
 	if (!page)