Re: [PATCH 2/4] mm/memory-tiers: introduce package-aware topology management for NUMA nodes

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] CPU nodes without local memory are completely excluded from the package topology.
- [High] Upgrading a provisional SLIT entry permanently unbinds and orphans the node.
- [High] TOCTOU race in `bind_node_to_package` binds a newly upgraded node to the wrong package.
- [High] `unbind_node_to_package` clears the node from the wrong nodemask if the node's state changed, permanently leaking the package.
- [High] `memory_package_lock` is acquired in a reclaim path while also being held during a GFP_KERNEL allocation, causing a deadlock.
- [Medium] Initialization loop in memory_package_init() permanently leaks kobjects and sysfs devices on failure.
- [Medium] Mixing goto-based error handling with scope-based guard() violates strict cleanup guidelines.
- [Medium] Global topology symmetry state is not recalculated when a provisional node is dropped.
- [Medium] The package notifier chain is exported but never invoked during node creation.
- [Medium] Exported API `mp_add_package_node_by_initiator` lacks bounds checking on `nid`, causing an out-of-bounds array access.
--

--- Patch [2]: [PATCH 2/4] mm/memory-tiers: introduce package-aware topology management for NUMA nodes ---
commit 0e269cf99bac40f9c00f53199f76f3781bc439c6
Author: Rakie Kim <[email protected]>
Date:   Thu Aug 6 08:09:33 2026 +0000

    mm/memory-tiers: introduce package-aware topology management for NUMA nodes
    
    The NUMA distance model provides only relative latency values between nodes
    and has no notion of structural grouping. Memory policies based on distance
    alone therefore cannot tell which nodes are local to the same physical
    package and which belong to a different one, which limits how well they can
    keep placement and migration within a package.
    
    Introduce a package-aware topology layer that groups NUMA nodes into a
    "memory package": a set of CPU nodes and their local memory-only nodes
    (such as CXL or HBM). A subsystem that owns a node registers a resolver to
    supply its package, and a policy can query, for any node, the other nodes
    that belong to the same package.

> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
> index 54851d8a195b0..5932df315604a 100644
> --- a/mm/memory-tiers.c
> +++ b/mm/memory-tiers.c

[ ... ]

> +static void mp_snapshot_rebuild(void)
> +{
> +	struct mp_snapshot *new, *old;
> +	struct memory_package *mp;
> +	int nr = 0, i = 0, nid;
> +
> +	lockdep_assert_held(&memory_package_lock);
> +
> +	list_for_each_entry(mp, &memory_packages, list)
> +		nr++;
> +
> +	new = kvzalloc(struct_size(new, pkg, nr), GFP_KERNEL);

[Severity: High]
Could this allocation trigger a deadlock?

The memory_package_lock is held here during a GFP_KERNEL allocation, which
can trigger direct reclaim. However, the same lock is acquired in
__mp_get_preferred_nodemask() during memory demotion, which runs in reclaim
context. 

If direct reclaim attempts to acquire the already held lock, will it
cause a self-deadlock?

> +	if (!new)
> +		return;

[ ... ]

> +static int bind_node_to_package(int nid)
> +{
> +	int package_id, pkg_id;
> +	struct memory_package *mp;
> +	nodemask_t nodes, cpu, mem;
> +
> +	mutex_lock(&memory_package_lock);
> +	if (!mpns[nid]) {
> +		mutex_unlock(&memory_package_lock);
> +		return -EINVAL;
> +	}
> +	package_id = mpns[nid]->package_id;
> +	mutex_unlock(&memory_package_lock);
> +
> +	mp = find_create_memory_package(package_id);
> +	if (IS_ERR(mp))
> +		return PTR_ERR(mp);
> +
> +	mutex_lock(&memory_package_lock);
> +	if (!mpns[nid]) {
> +		mutex_unlock(&memory_package_lock);
> +		return -ENOENT;
> +	}
> +	mpns[nid]->package = mp;

[Severity: High]
Is there a race condition when the lock is dropped and reacquired?

If a concurrent driver probe calls mp_add_package_node_by_initiator() while
the lock is dropped, it might destroy the old mpns[nid] and create a new one
with a different package_id.

When the lock is reacquired, mpns[nid] is not NULL, but could we end up
binding the new node (which wants the new package ID) to the old package 
struct that we just fetched?

> +	node_set(mpns[nid]->nid, mp->nodes);

[ ... ]

> +static void unbind_node_to_package(struct memory_package *mp, int nid)
> +{
> +	lockdep_assert_held(&memory_package_lock);
> +
> +	node_clear(nid, mp->nodes);
> +	if (node_state(nid, N_CPU))
> +		node_clear(nid, mp->cpu_nodes);
> +	else
> +		node_clear(nid, mp->memory_only_nodes);

[Severity: High]
Does this clear the node from the correct nodemask if its state changed?

If a memory-only node was initially added to mp->memory_only_nodes, but CPUs
were later hot-added to it, node_state(nid, N_CPU) will now evaluate to true.

During a subsequent hot-remove, this will clear the bit from mp->cpu_nodes
instead of mp->memory_only_nodes, leaving the bit permanently set and
potentially leaking the kobject.

> +
> +	if (mpns[nid])
> +		list_del(&mpns[nid]->package_entry);
> +
> +	update_package_preferred(mp);
> +}
> +
> +static struct memory_package_node *create_package_node(int nid, int initiator_nid)
> +{
> +	int cpu_nid, package_id;
> +	int source_flags;
> +	struct memory_package_node *mpn;
> +
> +	if (node_state(nid, N_CPU)) {
> +		cpu_nid = nid;
> +		source_flags = MPN_SRC_CPU;
> +	} else {
> +		if (initiator_nid >= 0) {
> +			cpu_nid = initiator_nid;
> +			source_flags = MPN_SRC_INITIATOR;
> +		} else {
> +			/*
> +			 * No driver-supplied initiator: fall back to the
> +			 * nearest CPU node (via SLIT/numa_distance).
> +			 */
> +			cpu_nid = numa_nearest_node(nid, N_CPU);
> +			source_flags = MPN_SRC_SLIT;
> +		}
> +	}

[Severity: Medium]
Are we missing a call to the exported package notifier chain here?

The register_mp_package_notifier() and mp_probe_package_id() APIs are
provided so subsystems can supply package hints, but mp_probe_package_id()
is never called in the node creation flow.

If a node is created without an explicit initiator, shouldn't we invoke the
notifier before falling back directly to numa_nearest_node()?

> +
> +	package_id = mp_node_to_package_id(cpu_nid);

[ ... ]

> +static int find_package_node(int nid, int initiator_nid)
> +{
> +	struct memory_package *unreg_mp = NULL;
> +	int ret = nid;
> +
> +	mutex_lock(&memory_package_lock);
> +	if (!mpns[nid]) {

[Severity: Medium]
Is it safe to index the mpns array with nid here?

The nid is passed directly from mp_add_package_node_by_initiator(), which
is an exported API. If a driver processes malformed or malicious firmware
tables, could an unvalidated nid cause an out-of-bounds array access?

> +		ret = NUMA_NO_NODE;
> +	} else if (mpns[nid]->source_flags == MPN_SRC_SLIT && initiator_nid >= 0) {
> +		/*
> +		 * SLIT-derived entries are provisional; if a driver later
> +		 * provides an explicit initiator, drop the provisional
> +		 * entry and rebuild with the stronger hint.
> +		 */
> +		unreg_mp = __destroy_package_node(nid);
> +		ret = NUMA_NO_NODE;
> +	}
> +	mutex_unlock(&memory_package_lock);
> +
> +	if (unreg_mp)
> +		device_unregister(&unreg_mp->dev);
> +
> +	return ret;
> +}

[Severity: Medium]
Should we recalculate the global topology symmetry state here?

When an existing provisional SLIT-derived node is dropped to make way for an
explicit driver initiator, __destroy_package_node() modifies the package node
counts. However, validate_topology_symmetry() is not called before returning.

Will the system's symmetry verdict remain stale until another completely
unrelated hotplug event triggers a recalculation?

[ ... ]

> +int mp_add_package_node_by_initiator(int nid, int initiator_nid)
> +{
> +	int ret;
> +
> +	ret = find_create_package_node(nid, initiator_nid);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mp_add_package_node_by_initiator);

[Severity: High]
Does upgrading a provisional SLIT entry permanently drop it from the topology?

If a driver calls this function to supply an initiator for a node that was
already initialized at boot, find_create_package_node() destroys the old entry 
and creates a new mpns[nid].

However, we simply return 0 here without calling bind_node_to_package().
Since the node is already online, no hotplug event will fire to bind it, 
potentially leaving mpn->package as NULL and orphaning the node.

[ ... ]

> +static int __mp_get_preferred_nodemask(int nid, enum mp_nodes_type node_type,
> +				    nodemask_t *out)
> +{
> +	int ret = 0;
> +
> +	/* No hot-path callers: the mutex is fine here. */
> +	guard(mutex)(&memory_package_lock);
> +
> +	if (!out) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

[Severity: Medium]
Does this mix goto-based error handling with scope-based guard() blocks?

The kernel's cleanup subsystem guidelines in include/linux/cleanup.h
explicitly forbid mixing goto jumps with guard() cleanup declarations in
the same function, as it can create unsafe ownership semantics.

[ ... ]

> +static int __init memory_package_init(void)
> +{
> +	int ret = 0, nid;
> +
> +	for_each_online_node(nid) {
> +		if (!node_state(nid, N_MEMORY))
> +			continue;

[Severity: High]
Are CPU nodes without local memory completely excluded from the topology?

If a CPU node evaluates to false for N_MEMORY, it will be skipped entirely.
This would cause update_package_preferred() to find an empty CPU mask for
memory-only nodes in that package, potentially breaking memory promotion by
leaving a memory-only node to set itself as its preferred promotion target.

> +
> +		ret = create_node_with_package(nid);
> +		if (ret)
> +			goto out;

[Severity: Medium]
Does this leak prior successfully initialized nodes on failure?

If an allocation or registration fails inside create_node_with_package(),
the code jumps directly to out.

Without a centralized rollback loop to individually clean up the
successfully established memory_package devices and kobjects, will this
result in a permanent unrecoverable memory leak?

> +	}
> +
> +	hotplug_node_notifier(mp_hotplug_callback, MEMTIER_HOTPLUG_PRI);
> +
> +	validate_topology_symmetry();
> +
> +out:
> +	return ret;
> +}
> +late_initcall(memory_package_init);

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