Re: [PATCH 3/4] mm/memory-tiers: register CXL nodes to memory packages via initiator

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

New issues:
- [High] `cxl_region_find_nearest_node()` incorrectly uses `p->nr_targets` as an upper bound and assumes a dense array, causing a NULL pointer dereference when accessing sparse targets.
- [High] The package notifier callback `cxl_region_add_package_node` accesses region parameters and targets without holding the required `cxl_rwsem.region` lock, allowing race conditions with concurrent region teardown.

Pre-existing issues:
- [High] Preexisting loops iterating over `p->targets` up to `p->nr_targets` fail to check for `NULL` pointers, leading to potential crashes when the array is sparse.
--

--- Patch [3]: [PATCH 3/4] mm/memory-tiers: register CXL nodes to memory packages via initiator ---
commit dff9dccb7111817b944d1f03a5ff003c16d488a5
Author: Rakie Kim <[email protected]>

mm/memory-tiers: register CXL nodes to memory packages via initiator

A CXL memory node comes online without an explicit package association, and
plain NUMA distance does not convey which physical package it belongs to.
Without that association a CXL node cannot be grouped with the CPUs that
front it.

Register a package notifier per CXL region. When the region's memory node
comes online, the notifier resolves an initiator CPU node - the NUMA node
of the first memdev backing the region - and binds the memory node to that
initiator's package. This gives the topology layer the CPU-side association
that plain NUMA distance does not carry.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e50dc716d4e82..af66e2e06c624 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2673,6 +2673,55 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
>  	return NOTIFY_STOP;
>  }
>  
> +/*
> + * Find a NUMA node to act as the initiator for this region: scan the
> + * region's endpoint targets and return the first one that resolves to a
> + * valid NUMA node.
> + */
> +static int cxl_region_find_nearest_node(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_endpoint_decoder *cxled = NULL;
> +	struct cxl_memdev *cxlmd = NULL;
> +	int i, numa_node;
> +
> +	for (i = 0; i < p->nr_targets; i++) {
> +		cxled = p->targets[i];
> +		cxlmd = cxled_to_memdev(cxled);

[Severity: High]
Does this code handle sparse target arrays correctly?

If __cxl_decoder_detach() clears elements in p->targets[] and decrements
p->nr_targets when a target is detached, the array can become sparse.
When iterating using p->nr_targets as an upper bound, could p->targets[i]
be NULL for detached targets?

Passing a NULL cxled to cxled_to_memdev() will immediately dereference
cxled->cxld.dev.parent, potentially causing a kernel panic.

[Severity: High]
This isn't a bug introduced by this patch, but the same missing NULL check
appears to be a pre-existing issue in other functions iterating p->targets.

For example, in cxl_dpa_to_hpa():

drivers/cxl/core/region.c:cxl_dpa_to_hpa() {
    ...
    for (int i = 0; i < p->nr_targets; i++) {
        if (cxlmd == cxled_to_memdev(p->targets[i])) {
    ...
}

And in region_offset_to_dpa_result():

drivers/cxl/core/region.c:region_offset_to_dpa_result() {
    ...
    for (int i = 0; i < p->nr_targets; i++) {
        cxled = p->targets[i];
        if (cxled->pos != pos)
    ...
}

Should these loops also check if p->targets[i] is NULL before dereferencing it?

> +		numa_node = dev_to_node(&cxlmd->dev);
> +		if (numa_node != NUMA_NO_NODE)
> +			return numa_node;
> +	}
> +	return NUMA_NO_NODE;
> +}
> +
> +/*
> + * Package notifier callback: when a new memory node is onlined via dax
> + * kmem, bind the node this CXL region backs to its memory package, using
> + * the nearest region target as the initiator. Notifications for other
> + * nodes are ignored.
> + */
> +static int cxl_region_add_package_node(struct notifier_block *nb,
> +				       unsigned long dax_nid, void *data)
> +{
> +	int region_nid, nearest_nid, ret;
> +	struct cxl_region *cxlr = container_of(nb, struct cxl_region, package_notifier);
> +
> +	region_nid = phys_to_target_node(cxlr->params.res->start);
> +	if (region_nid != dax_nid)
> +		return NOTIFY_DONE;
> +
> +	nearest_nid = cxl_region_find_nearest_node(cxlr);

[Severity: High]
Does this callback need to hold the cxl_rwsem.region lock when accessing
shared region parameters?

This notifier can be called asynchronously when a DAX kmem node is probed.
It accesses cxlr->params.res->start and p->targets without holding the
topology lock.

Concurrently, cxl_decoder_detach() could modify p->targets, or teardown
logic could free p->res under the cxl_rwsem.region write lock. Could this
race condition result in a use-after-free or NULL pointer dereference?

> +	if (nearest_nid == NUMA_NO_NODE)
> +		return NOTIFY_DONE;
> +
> +	ret = mp_add_package_node_by_initiator(dax_nid, nearest_nid);
> +	if (ret)
> +		return NOTIFY_DONE;
> +
> +	return NOTIFY_OK;
> +}

[ ... ]

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