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
