Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing page table teardown on vmap_pages_range() failure in vm_module_tags_populate() creates dangling PTEs pointing to freed physical memory. - [High] Failure to revert `module_tags.size` after `vm_module_tags_populate()` fails creates a race condition that can return unmapped memory to the module loader, leading to a kernel crash. - [High] Lockless race between alloc_tag_sub() and clean_unused_counters() causes use-after-free on per-CPU counters. -- commit d4d7fb3ee0094651ba53e0f489d27eaa1b6cc2af Author: Hao Ge <[email protected]> Subject: alloc_tag: release the reservation when populate fails This patch updates reserve_module_tags() to call release_module_tags() when vm_module_tags_populate() fails. This ensures the module tags reservation is correctly released on error paths. > diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c > index 112a014d4b890..c11813cd9e153 100644 > --- a/mm/alloc_tag.c > +++ b/mm/alloc_tag.c > @@ -963,6 +963,7 @@ static void *reserve_module_tags(struct module *mod, > unsigned long size, > shutdown_mem_profiling(true); > pr_err("Failed to allocate memory for allocation tags > in the module %s. Memory allocation profiling is disabled!\n", > mod->name); > + release_module_tags(mod, false); > return ERR_PTR(grow_res); > } > } [Severity: High] This isn't a bug introduced by this patch, but is it possible for module_tags.size to remain inflated here on error? Earlier in reserve_module_tags(), module_tags.size is optimistically updated: if (module_tags.size < offset + size) { int grow_res; module_tags.size = offset + size; [...] grow_res = vm_module_tags_populate(); If vm_module_tags_populate() fails, the error path calls shutdown_mem_profiling() and releases the module tags, but it does not revert module_tags.size. Can a second thread loading a different module concurrently evaluate codetag_needs_module_section() as true just before shutdown_mem_profiling() disables profiling? If this second thread enters reserve_module_tags(), it might find the newly freed space at the same offset in the maple tree. Because module_tags.size was left inflated, would the check "if (module_tags.size < offset + size)" evaluate to false, causing the second thread to skip vm_module_tags_populate()? This could result in returning a pointer to unmapped virtual memory to the module loader, leading to a kernel crash when the loader subsequently calls memcpy() on this pointer. [Severity: Critical] This is a pre-existing issue, but does vm_module_tags_populate() clean up page tables correctly when it fails? Looking at vm_module_tags_populate(), physical pages are mapped using vmap_pages_range(): if (nr < more_pages || vmap_pages_range(phys_end, phys_end + (nr << PAGE_SHIFT), PAGE_KERNEL, next_page, PAGE_SHIFT) < 0) { release_pages_arg arg = { .pages = next_page }; /* Clean up and error out */ release_pages(arg, nr); return -ENOMEM; } If vmap_pages_range() fails midway through, it returns a negative error code but does not unmap the pages it already successfully mapped. The error handler unconditionally calls release_pages(arg, nr) to return the physical pages to the allocator, but it fails to call vunmap_range() to clear the kernel page table entries. Does this leave valid, writable kernel PTEs pointing to freed physical memory? If these physical pages are reallocated to other subsystems, could a subsequent write to the module_tags virtual memory region silently corrupt unrelated kernel memory? [Severity: High] This is also a pre-existing issue, but is there a lockless race between alloc_tag_sub() and clean_unused_counters() that could cause a use-after-free? When an object allocated by a module is freed, alloc_tag_sub() locklessly decrements the profiling counters: this_cpu_sub(tag->counters->bytes, bytes); this_cpu_dec(tag->counters->calls); Concurrently, another module load or unload may trigger clean_unused_counters(), which checks if the counters are completely unused based solely on the bytes counter: counter = alloc_tag_read(tag); if (!counter.bytes) { free_percpu(tag->counters); tag->counters = NULL; If a thread executing alloc_tag_sub() is preempted or interrupted after this_cpu_sub() but before this_cpu_dec(), the global sum of bytes could reach 0 while the calls counter decrement is still pending. Seeing bytes == 0, clean_unused_counters() could immediately call free_percpu(tag->counters). When the preempted thread resumes, would it attempt to execute this_cpu_dec(tag->counters->calls) on the freed memory, causing a use-after-free that corrupts the per-CPU allocator state? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
