Re: [PATCH v5 04/36] numa: introduce N_MEMORY_PRIVATE

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing bounds validation on `nid` in `node_private_register()` and `node_private_unregister()` leads to out-of-bounds reads.
- [Medium] Lockless read of an `__rcu` pointer in `pgdat_is_private()` without using `rcu_access_pointer()`.
--

--- Patch [4]: [PATCH v5 04/36] numa: introduce N_MEMORY_PRIVATE ---
commit 69b3cbf2395b52c525233c59376e585ae83c3f31
Author: Gregory Price <[email protected]>

numa: introduce N_MEMORY_PRIVATE

Some devices want to hotplug their memory onto a node, but not have it
exposed as "general purpose". This commit adds basic infrastructure for
N_MEMORY_PRIVATE nodes, intended for memory isolated from the page
allocator's normal and fallback zonelists.

> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 3da91929ad4e3..94cd51f51b7e8 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c

[ ... ]

> @@ -868,6 +869,116 @@ void register_memory_blocks_under_node_hotplug(int nid, unsigned long start_pfn,
>  			   (void *)&nid, register_mem_block_under_node_hotplug);
>  	return;
>  }
> +
> +static DEFINE_MUTEX(node_private_lock);
> +
> +/**
> + * node_private_register - Register a private node
> + * @nid: Node identifier

[ ... ]

> + */
> +int node_private_register(int nid, struct node_private *np)
> +{
> +	struct node_private *existing;
> +	pg_data_t *pgdat;
> +	int ret = 0;
> +
> +	if (!np || !node_possible(nid))
> +		return -EINVAL;

[Severity: Medium]
Could this lead to an out-of-bounds read if a driver passes an invalid nid
like NUMA_NO_NODE (-1)? 

node_possible() and NODE_DATA() do not perform bounds checking internally.
Should we verify nid >= 0 && nid < MAX_NUMNODES before using it?

> +
> +	mutex_lock(&node_private_lock);
> +	mem_hotplug_begin();
> +
> +	/* N_MEMORY_PRIVATE and N_MEMORY are mutually exclusive */
> +	if (node_state(nid, N_MEMORY)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	pgdat = NODE_DATA(nid);

[ ... ]

> +/**
> + * node_private_unregister - Unregister a private node
> + * @nid: Node identifier

[ ... ]

> + */
> +int node_private_unregister(int nid)
> +{
> +	struct node_private *np;
> +	pg_data_t *pgdat;
> +
> +	if (!node_possible(nid))
> +		return 0;

[Severity: Medium]
Is it possible for a driver to pass NUMA_NO_NODE here as well, leading to
the same out-of-bounds read issue when node_possible() and NODE_DATA()
are evaluated?

> +
> +	mutex_lock(&node_private_lock);
> +	mem_hotplug_begin();
> +
> +	pgdat = NODE_DATA(nid);

[ ... ]

> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 0507193b3ae34..9815e48c03b97 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h

[ ... ]

> @@ -1596,12 +1598,25 @@ typedef struct pglist_data {
>  	atomic_long_t		vm_stat[NR_VM_NODE_STAT_ITEMS];
>  #ifdef CONFIG_NUMA
>  	struct memory_tier __rcu *memtier;
> +	struct node_private __rcu *node_private;
>  #endif
>  #ifdef CONFIG_MEMORY_FAILURE
>  	struct memory_failure_stats mf_stats;
>  #endif
>  } pg_data_t;
>  
> +#ifdef CONFIG_NUMA
> +static inline bool pgdat_is_private(pg_data_t *pgdat)
> +{
> +	return !!pgdat->node_private;

[Severity: Medium]
Does this lockless read of the __rcu pointer require rcu_access_pointer()?

Without it, sparse will likely complain about an __rcu address space
violation, and we might be missing the READ_ONCE() barrier that prevents
compiler load tearing.

> +}
> +#else
> +static inline bool pgdat_is_private(pg_data_t *pgdat)
> +{
> +	return false;
> +}
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4