Re: [PATCH v5 09/36] mm/migrate: disallow userland driven migration for private nodes

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

New issues:
- [High] Incomplete replacement of `zone_device` checks allows uncoordinated background migrations of private node memory.
- [High] node_private_unregister() permanently returns -EBUSY for private nodes because offline_pages() never clears N_MEMORY_PRIVATE.

Pre-existing issues:
- [High] User-space addresses are not stripped of memory tags (e.g., ARM64 MTE) before VMA lookup in do_pages_stat_array(), causing valid tagged addresses to fail with -EFAULT.
--

--- Patch [9]: [PATCH v5 09/36] mm/migrate: disallow userland driven migration for private nodes ---
commit 7ed55bccdffe7ca2bf917693b091b54ebe294b02
Author: Gregory Price <[email protected]>

mm/migrate: disallow userland driven migration for private nodes

Use the same filter locations as zone_device folios to disallow
userland driven migration requests for private node memory.

> diff --git a/mm/migrate.c b/mm/migrate.c
> index a8a86141dbb54..d20674c07b947 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2268,7 +2268,7 @@ static int __add_folio_for_migration(struct folio *folio, int node,
>  	if (is_zero_folio(folio) || is_huge_zero_folio(folio))
>  		return -EFAULT;
>  
> -	if (folio_is_zone_device(folio))
> +	if (folio_is_private_managed(folio))

[Severity: High]
By updating these filters, do we also need to update similar checks in
uncoordinated background migration paths?

If background tasks encounter a private node folio, they will evaluate
folio_is_zone_device() or is_zone_device_page() as false. Could this allow
NUMA balancing, KSM, or khugepaged to silently migrate private node memory
to general-purpose nodes?

For example, in NUMA balancing:

mm/memory.c:do_numa_page() {
    ...
    if (!folio || folio_is_zone_device(folio))
        goto out_map;
    ...
}

In THP collapse:

mm/khugepaged.c:khugepaged_scan_pmd() {
    ...
    if (WARN_ON_ONCE(page && is_zone_device_page(page)))
        page = NULL;
    ...
}

And in KSM:

mm/ksm.c:get_mergeable_page() {
    ...
    if (!folio_is_zone_device(folio) &&
        folio_test_anon(folio)) {
    ...
}

>  		return -ENOENT;
>  
>  	if (folio_nid(folio) == node)
> @@ -2477,7 +2477,7 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
>  		if (folio) {
>  			if (is_zero_folio(folio) || is_huge_zero_folio(folio))
>  				err = -EFAULT;
> -			else if (folio_is_zone_device(folio))
> +			else if (folio_is_private_managed(folio))

[Severity: High]
This isn't a bug introduced by this patch, but does do_pages_stat_array() fail
to handle tagged pointers?

When a user-space program passes an array of tagged pointers to move_pages
with nodes == NULL, the pointers are cast directly to unsigned long without
being sanitized by untagged_addr_remote():

mm/migrate.c:do_pages_stat_array() {
    ...
    unsigned long addr = (unsigned long)(*pages);
    struct vm_area_struct *vma;
    // ...
    vma = vma_lookup(mm, addr);
    ...
}

Could this cause vma_lookup() to treat the tag bits as part of the virtual
address, causing valid tagged addresses (like those used with ARM64 MTE) to
fail with -EFAULT?

>  				err = -ENOENT;
>  			else
>  				err = folio_nid(folio);

[Severity: High]
On a related note regarding private node teardown, does offline_pages() need
to clear N_MEMORY_PRIVATE?

If a driver attempts to teardown a device and offline its memory,
offline_pages() clears N_MEMORY but omits N_MEMORY_PRIVATE:

mm/memory_hotplug.c:offline_pages() {
    ...
    if (node_arg.nid >= 0)
        node_clear_state(node, N_MEMORY);
    ...
}

Will this cause node_private_unregister() to evaluate node_is_private(nid)
as true and permanently return -EBUSY?

drivers/base/node.c:node_private_unregister() {
    ...
    if (node_is_private(nid)) {
        mem_hotplug_done();
        mutex_unlock(&node_private_lock);
        return -EBUSY;
    }
    ...
}

Could this permanent -EBUSY prevent drivers using the N_MEMORY_PRIVATE
feature from cleanly unloading?

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