alloc_tag and module codetag section fixes
Subsystems (9)
Hao Ge
reviewerv11 cover alloc_tag and module codetag section fixes
With profiling toggled off, the overflow check in reserve_module_tags() did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted /proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns -EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.
Review of the earlier series by Sashiko turned up more problems.
One is a race. layout_sections() and move_module() both asked codetag_needs_module_section() where a codetag section goes, and mem_profiling_support can change between the two calls, for instance when another module load overflows the tag index and shuts profiling down. move_module() then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.
v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before layout_sections() and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on -EAGAIN the section is laid out as regular module data right in the same load.
[1] https://lore.kernel.org/all/[email protected]/
Patch 1 moves release_module_tags() above reserve_module_tags(), since the failure paths now have to call it.
Patch 2 is new. __vmap_pages_range_noflush() and friends leave the PTEs they installed when they fail, and the callers do not agree on who cleans them up. Per the discussion with Suren and Ulad [2] each mapping function now undoes its own partial work, the __GFP_NOFAIL retry loop in __vmalloc_area_node() and pci_remap_iospace() stop tripping over the leftovers of a previous attempt, and vm_module_tags_populate() no longer needs its own undo, patch 3 drops it.
[2] https://lore.kernel.org/all/[email protected]/
Patch 3 cleans up the populate failure path: the reservation is released and module_tags.size rolled back, so a concurrent load which already passed needs_section_mem() does not skip populate for the freed gap. The PTEs a failed vmap_pages_range() left behind are undone by patch 2 now. It carries both Fixes tags so it backports wherever patch 5 goes, which uses its prev_size.
Patch 4 introduces SH_ENTSIZE_STANDALONE to mark sections with a separate allocation. The percpu section was previously excluded from the layout by clearing its SHF_ALLOC, which per the ELF spec says the section occupies memory during execution, and percpu does, only outside the regular module layout. The mark lives in sh_entsize now, find_sec(".data..percpu") gives stable results again and apply_relocations() goes back to testing only SHF_ALLOC. The section would show up under /sys/module/*/sections/, but it has one instance per CPU and no single address, and the entry never existed, so add_sect_attrs() and add_notes_attrs() skip it.
Patch 5 moves the codetag allocation out of move_module() in front of layout_sections(), so the placement is decided in one step and the race is gone. On overflow profiling is shut down, the reservation released, module_tags.size rolled back and -EAGAIN returned, the section is laid out as regular module data and the module loads without profiling instead of failing. Any other error fails the load. The release and the fallback belong together, without the release rmmod hits the stale entry and panics.
Patch 6 skips the percpu counter allocation when profiling is off. After the shutdown modules load their codetag section as regular data, load_module() still allocated counters for every tag and release_module_tags() cannot find them on unload, so they leaked (Suggested by Suren).
Patch 7 fixes the /proc/allocinfo lifecycle. shutdown_mem_profiling() runs under mod_lock, and the synchronous remove_proc_entry() deadlocks with a reader taking mod_lock for read in allocinfo_start(), so the removal goes to a workqueue. The file is created at the end of alloc_tag_init(), a leftover file after a failed init would panic its readers (Found by Sashiko). When proc_create() itself fails the codetag type and the module tags memory are released with the new codetag_unregister_type(), and the lockless alloc_tag_cttype reader in alloc_tag_top_users() is guarded with RCU.
Tested on an x86_64 virtual machine:
Booted without sysctl.vm.mem_profiling=1,compressed:
# cat /proc/allocinfo is fine
Booted with sysctl.vm.mem_profiling=1,compressed:
# cat /proc/allocinfo is fine # insmod overflow_tag.ko # dmesg With module overflow_tag there are too many tags to fit in 13 page flag bits. Memory allocation profiling is disabled! # rmmod overflow_tag The module loads without profiling and unloads cleanly.
Also ran continuous LTP stress for a few days, nothing abnormal so far.
Changes in v11:
- new patch 2 makes the vmap functions undo their partial mappings on failure, per the discussion with Suren and Ulad [2]; patch 3 drops its own vmap undo accordingly
- patch 7 is renamed to "fix the /proc/allocinfo lifecycle", it now also unwinds the codetag type with the new codetag_unregister_type() when proc_create() fails and guards the lockless alloc_tag_cttype reader in alloc_tag_top_users() with RCU
- collected Reviewed-by tags from Suren Baghdasaryan (patches 4, 5)
Changes in v10:
- fold in the two alloc_tag fixes from
https://lore.kernel.org/all/[email protected]/
as patches 5 and 6, they replace the versions in the mm tree and fix the percpu leak Sashiko keeps flagging
https://lore.kernel.org/all/[email protected]/
- create /proc/allocinfo at the end of alloc_tag_init(), a leftover file after a failed init would panic its readers (Found by Sashiko)
- count note sections with sect_visible() in add_notes_attrs() too, the count has to match the fill loop
Changes in v9:
- do not export .data..percpu under /sys/module/*/sections/ (Petr Pavlu).
- move the populate failure cleanup in front of the rework, v8 patch 4 is patch 2 now. It declares prev_size itself, which the overflow path of the rework also uses, so it carries both Fixes tags and the two patches backport together
Changes in v8:
- roll back module_tags.size when the reservation is released, so a concurrent load which already passed needs_section_mem() does not skip populate for the freed gap (Sashiko)
- unmap the PTEs a failed vmap_pages_range() installed, a retry to populate the same range would BUG on them
- zero the separately allocated codetag memory for an SHT_NOBITS section, the tag area pages are not zeroed on allocation (Sashiko)
- .data..percpu is exported under /sys/module/*/sections/ with the boot CPU instance of the per-cpu area, and the interface is documented in the ABI docs (Petr Pavlu)
- keep a comment in apply_relocations() on how .data..percpu is relocated (Petr Pavlu)
- replace the "Based-on-a-patch-by:" tag with an in-body attribution and a numbered Link: (Andrew Morton)
Changes in v7:
- split the rework following review feedback (Petr Pavlu, Suren Baghdasaryan)
- new patch 2 marks separately allocated sections with SH_ENTSIZE_STANDALONE instead of clearing SHF_ALLOC (suggested by Petr Pavlu)
- split the populate failure release into its own patch (suggested by Suren Baghdasaryan)
Changes in v6:
- rework on Petr's prototype and allocate codetag sections before layout_sections(), the retry and its state resets are gone
- fix the layout_sections()/move_module() race (Found by Sashiko)
- release the reservation on populate failure as well (Found by Sashiko)
- only -EAGAIN keeps the fallback, other errors fail the load
Changes in v5:
- add Fixes: and Cc: stable to patch 1/2 as well, since 2/2 does not compile without it (Andrew Morton)
- restore frob-adjusted mem[type].size on retry instead of zeroing, as s390 and parisc add GOT/PLT space there in module_frob_arch_sections() (Reported by Sashiko)
- drop the load_module() mem_profiling_support check; the percpu counter leak is pre-existing and orthogonal to this fix
Changes in v4:
- add a new patch (1/2) to move release_module_tags() above reserve_module_tags(); the overflow fix is 2/2
- release the reservation on the -EAGAIN path
- return -EAGAIN instead of -ENOMEM so the module can still load without profiling (Suren)
- reset sh_addr, mem[type].size and sym/str SHF_ALLOC before retry
- skip percpu counters in load_module() when profiling is off
Changes in v3:
- use pr_warn_once() instead of pr_warn()
- return -ENOMEM instead of -ENOSPC (Suren)
- expand the commit message to describe the /proc/allocinfo impact (Andrew)
Changes in v2:
- return an error after shutdown_mem_profiling() to skip vm_module_tags_populate()
v1: https://lore.kernel.org/all/[email protected]/
v2: https://lore.kernel.org/all/[email protected]/
v3: https://lore.kernel.org/all/[email protected]/
v4: https://lore.kernel.org/all/[email protected]/
v5: https://lore.kernel.org/all/[email protected]/
v6: https://lore.kernel.org/all/[email protected]/
v7: https://lore.kernel.org/all/[email protected]/
v8: https://lore.kernel.org/all/[email protected]/
v9: https://lore.kernel.org/all/[email protected]/
v10: https://lore.kernel.org/all/[email protected]/
Hao Ge (7):
alloc_tag: move release_module_tags() above reserve_module_tags() mm/vmalloc: undo partial mappings inside the mapping functions
alloc_tag: clean up the populate failure path
module: introduce SH_ENTSIZE_STANDALONE for separately allocated
sections
module: allocate codetag sections before the regular module layout
alloc_tag: skip percpu counter allocation when profiling is disabled
alloc_tag: fix the /proc/allocinfo lifecycle
include/linux/alloc_tag.h | 2 +- include/linux/codetag.h | 2 + include/linux/module.h | 2 + kernel/module/internal.h | 8 ++ kernel/module/kallsyms.c | 13 +--- kernel/module/main.c | 133 ++++++++++++++++--------------- kernel/module/sysfs.c | 17 +++- lib/codetag.c | 36 ++++++++- mm/alloc_tag.c | 160 ++++++++++++++++++++++---------------- mm/kmsan/shadow.c | 4 + mm/show_mem.c | 2 +- mm/vmalloc.c | 31 +++++++- 12 files changed, 263 insertions(+), 147 deletions(-)
Signature · 1 line
2.25.1
Hao Ge
reviewerPATCH v11 1/7 alloc_tag: move release_module_tags() above reserve_module_tags()
release_module_tags() is a cleanup helper. reserve_module_tags() can also fail after storing the reservation in the maple tree, in which case it should call release_module_tags() to undo it. Move the helper above reserve_module_tags() so no forward declaration is needed.
No functional change.
--- mm/alloc_tag.c | 92 +++++++++++++++++++++++++------------------------- 1 file changed, 46 insertions(+), 46 deletions(-)
mm/alloc_tag.c +46 -46
@@ -835,6 +835,52 @@ static int vm_module_tags_populate(void)return 0 ; } +static void release_module_tags (struct module *mod,bool used) +{ +MA_STATE (mas, &mod_area_mt, module_tags.size, module_tags.size); +struct alloc_tag *start_tag; +struct alloc_tag *end_tag; +struct module *val; + +mas_lock (&mas); +mas_for_each_rev (&mas, val,0 ) +if (val == mod) +break ; + +if (!val)/* module not found */ + goto out; + +if (!used) + goto release_area; + + start_tag = (struct alloc_tag *)(module_tags.start_addr + mas.index); + end_tag = (struct alloc_tag *)(module_tags.start_addr + mas.last); +if (!clean_unused_counters (start_tag, end_tag)) { +struct alloc_tag *tag; + +for (tag = start_tag; tag <= end_tag; tag++) { +struct alloc_tag_counters counter; + +if (!tag->counters) +continue ; + + counter =alloc_tag_read (tag); +pr_info ("%s:%u module %s func:%s has %llu allocated at module unload\n" , + tag->ct.filename, tag->ct.lineno, tag->ct.modname, + tag->ct.function, counter.bytes); + } + }else { + used = false; + } +release_area: +mas_store (&mas, used ? &unloaded_mod :NULL ); + val =mas_prev_range (&mas,0 ); +if (val == &prepend_mod) +mas_store (&mas,NULL ); +out: +mas_unlock (&mas); +} +static void *reserve_module_tags (struct module *mod,unsigned long size,unsigned int prepend,unsigned long align) {
@@ -922,52 +968,6 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,return (struct alloc_tag *)(module_tags.start_addr + offset); } -static void release_module_tags (struct module *mod,bool used) -{ -MA_STATE (mas, &mod_area_mt, module_tags.size, module_tags.size); -struct alloc_tag *start_tag; -struct alloc_tag *end_tag; -struct module *val; - -mas_lock (&mas); -mas_for_each_rev (&mas, val,0 ) -if (val == mod) -break ; - -if (!val)/* module not found */ - goto out; - -if (!used) - goto release_area; - - start_tag = (struct alloc_tag *)(module_tags.start_addr + mas.index); - end_tag = (struct alloc_tag *)(module_tags.start_addr + mas.last); -if (!clean_unused_counters (start_tag, end_tag)) { -struct alloc_tag *tag; - -for (tag = start_tag; tag <= end_tag; tag++) { -struct alloc_tag_counters counter; - -if (!tag->counters) -continue ; - - counter =alloc_tag_read (tag); -pr_info ("%s:%u module %s func:%s has %llu allocated at module unload\n" , - tag->ct.filename, tag->ct.lineno, tag->ct.modname, - tag->ct.function, counter.bytes); - } - }else { - used = false; - } -release_area: -mas_store (&mas, used ? &unloaded_mod :NULL ); - val =mas_prev_range (&mas,0 ); -if (val == &prepend_mod) -mas_store (&mas,NULL ); -out: -mas_unlock (&mas); -} -static int load_module (struct module *mod,struct codetag *start,struct codetag *stop) {/* Allocate module alloc_tag percpu counters */
Signature · 1 line
2.25.1
Hao Ge
reviewerPATCH v11 2/7 mm/vmalloc: undo partial mappings inside the mapping functions
In reply to Hao Ge__vmap_pages_range_noflush() and friends can install some PTEs before failing and leave them mapped, and the kernel callers do not agree on who cleans them up. For example, pcpu_map_pages() and kmsan_ioremap_page_range() unmap the leftovers themselves, while vm_module_tags_populate() and the __GFP_NOFAIL retry loop in __vmalloc_area_node() relied on the mapping functions cleaning up and did not call anything like vunmap_range() themselves. When the same range is mapped again, the attempt hits the leftovers and fails, with BUG() in vmap_pte_range() for huge mappings.
After discussing with Suren and Ulad, we decided the cleanup belongs to __vmap_pages_range_noflush() and friends, so the callers no longer need to unmap the partial mappings themselves. Each function now undoes the PTEs it installed itself.
The rollback calls the low-level __vunmap_range_noflush(), it just clears the PTEs of the range it is given, which is all a rollback needs. It cannot use vunmap_range_noflush() because these mapping functions also map the KMSAN shadow and origin, and for a metadata range its hook would look up the metadata of the metadata, get 0 and BUG() on addr >= end. The failed mappings were never accessed, no TLB flush needed.
--- mm/kmsan/shadow.c | 4 ++++ mm/vmalloc.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)
mm/kmsan/shadow.c +4 -0
@@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, o_pages, page_shift);kmsan_leave_runtime ();if (mapped) { +/* Undo the shadow mapping set up above. */ +kmsan_enter_runtime (); +__vunmap_range_noflush (shadow_start, shadow_end); +kmsan_leave_runtime (); err = mapped; goto ret; }
mm/vmalloc.c +29 -2
@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }
@@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,if (!err) err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) +__vunmap_range_noflush (addr, end); +return err; }
@@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }
@@ -683,6 +694,7 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr;WARN_ON (page_shift <PAGE_SHIFT );
@@ -696,8 +708,14 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, err =vmap_range_noflush (addr, addr + (1UL << page_shift),page_to_phys (pages[i]), prot, page_shift); -if (err) +if (err) { +/* +* vmap_range_noflush() undoes its own partial work, +* discard the chunks that already mapped. +*/ +__vunmap_range_noflush (start, addr);return err; + } addr +=1UL << page_shift; }
@@ -714,7 +732,16 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +/* +* The page tables undo themselves on failure. Tear down the +* metadata that was fully set up before the mapping failed. +*/ +if (ret) +kmsan_vunmap_range_noflush (addr, end); + +return ret; } static int __vmap_pages_range(unsigned long addr, unsigned long end,
Signature · 1 line
2.25.1
[email protected]
AI reviewRe: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] mm: BUG_ON() triggered in __vunmap_range_noflush() on first chunk failure - [High] mm: stale TLB…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: BUG_ON() triggered in __vunmap_range_noflush() on first chunk failure
- [High] mm: stale TLB entries left on mapping rollback cause UAF
--
commit 8d2eaa273026d4be467acd8256954d343a66fc4c
Author: Hao Ge <[email protected]>
mm/vmalloc: undo partial mappings inside the mapping functions
This patch modifies __vmap_pages_range_noflush() and friends to undo the PTEs they installed themselves on failure. Callers no longer need to clean up partial mappings.
Quoted 5 lines
mm/vmalloc.c +0
-0
mm/vmalloc.c static int vmap_range_noflush(unsigned long addr, unsigned long end, · line 357
Quoted 9 lines
mm/vmalloc.c +4
-0@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }
[Severity: High]
Is it safe to omit a TLB flush on rollback here?
By clearing the PTEs using __vunmap_range_noflush() without flushing the TLB, the CPU might retain stale entries.
If a caller like vm_module_tags_populate() in mm/alloc_tag.c frees the physical pages immediately after the mapping attempt fails:
if (vmap_pages_range(...) < 0) {
...
release_pages(arg, nr);
}could speculative or adjacent accesses hit the freed memory, leading to a use-after-free and state corruption?
[ ... ]
Quoted 11 lines
mm/vmalloc.c +1
-0@@ -683,6 +694,7 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr;WARN_ON (page_shift <PAGE_SHIFT );@@ -696,8 +708,14 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, err = vmap_range_noflush(addr, addr + (1UL << page_shift), page_to_phys(pages[i]), prot,
mm/vmalloc.c int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, · line 718
Quoted 10 lines
mm/vmalloc.c +7
-1page_shift); -if (err) +if (err) { +/* +* vmap_range_noflush() undoes its own partial work, +* discard the chunks that already mapped. +*/ +__vunmap_range_noflush (start, addr);return err; + }
[Severity: High]
What happens if vmap_range_noflush() fails on the very first iteration of this loop?
In that case, the variable addr is still equal to start, so this calls __vunmap_range_noflush() with a zero-length range.
Because __vunmap_range_noflush() contains:
BUG_ON(addr >= end);
will this immediately panic the kernel on an early allocation failure?
Quoted 6 lines
mm/vmalloc.c +0
-0addr +=1UL << page_shift; }@@ -714,7 +732,16 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)
mm/vmalloc.c int vmap_pages_range_noflush(unsigned long addr, unsigned long end, · line 742
Quoted 10 lines
mm/vmalloc.c +8
-1return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +/* +* The page tables undo themselves on failure. Tear down the +* metadata that was fully set up before the mapping failed. +*/ +if (ret) +kmsan_vunmap_range_noflush (addr, end);
[Severity: High]
Are we also leaving behind stale TLB entries for the KMSAN metadata here?
kmsan_vmap_pages_range_noflush() successfully maps the metadata and explicitly flushes the TLB for shadow/origin mappings.
If the subsequent main mapping fails, this rollback clears the KMSAN PTEs but skips the TLB flush, which might leave stale TLB entries pointing to freed KMSAN metadata.
Quoted 3 lines
mm/vmalloc.c +2
-0+ +return ret; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Uladzislau Rezki
maintainerRe: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions
In reply to Hao GeQuoted 39 lines · Hao Ge
__vmap_pages_range_noflush()and friends can install some PTEs before failing and leave them mapped, and the kernel callers do not agree on who cleans them up. For example,pcpu_map_pages()andkmsan_ioremap_page_range()unmap the leftovers themselves, whilevm_module_tags_populate()and the__GFP_NOFAILretry loop in__vmalloc_area_node()relied on the mapping functions cleaning up and did not call anything likevunmap_range()themselves. When the same range is mapped again, the attempt hits the leftovers and fails, withBUG()invmap_pte_range()for huge mappings.After discussing with Suren and Ulad, we decided the cleanup belongs to
__vmap_pages_range_noflush()and friends, so the callers no longer need to unmap the partial mappings themselves. Each function now undoes the PTEs it installed itself.The rollback calls the low-level
__vunmap_range_noflush(), it just clears the PTEs of the range it is given, which is all a rollback needs. It cannot usevunmap_range_noflush()because these mapping functions also map the KMSAN shadow and origin, and for a metadata range its hook would look up the metadata of the metadata, get 0 andBUG()on addr >= end. The failed mappings were never accessed, no TLB flush needed.Fixes 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL")Fixes 0f9b685626da ("alloc_tag: populate memory for module tags as needed")Reported-by Sashiko <[email protected]>Signed-off-by Hao Ge <[email protected]>--- mm/kmsan/shadow.c | 4 ++++ mm/vmalloc.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)mm/kmsan/shadow.c +0
-0
mm/kmsan/shadow.c int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, · line 267
Quoted 10 lines
mm/kmsan/shadow.c +4
-0@@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, o_pages, page_shift);kmsan_leave_runtime ();if (mapped) { +/* Undo the shadow mapping set up above. */ +kmsan_enter_runtime (); +__vunmap_range_noflush (shadow_start, shadow_end); +kmsan_leave_runtime (); err = mapped; goto ret; }
Can we just do that inside the vmalloc? For example in the __vmap_pages_range_noflush() as entry function for all(?) helpers? So we do not need do it manually?
Quoted 26 lines
mm/vmalloc.c +7
-0@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }@@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,if (!err) err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) +__vunmap_range_noflush (addr, end); +return err; }
mm/vmalloc.c static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end, · line 683
Quoted 10 lines
mm/vmalloc.c +4
-0@@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }
The only one user of that function is __vmap_pages_range_noflush() so we can cover both cases there small and huge path.
Is that enough just to do unroll in the below:
__vmap_pages_range_noflush()vmap_page_range()
functions? It will also fix NOFAIL case:
<snip>
mm/vmalloc.c +39 -15
@@ -359,10 +359,19 @@ int vmap_page_range(unsigned long addr, unsigned long end, err =vmap_range_noflush (addr, end, phys_addr,pgprot_nx (prot), ioremap_max_page_shift); +if (err) + goto error_cleanup_range; +flush_cache_vmap (addr, end); -if (!err) - err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, + err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) + goto error_cleanup_range; + +return 0 ; + +error_cleanup_range: +__vunmap_range_noflush (addr, end);return err; }
@@ -683,26 +692,35 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr; +int err; -WARN_ON (page_shift <PAGE_SHIFT ); - -if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || - page_shift ==PAGE_SHIFT ) -return vmap_small_pages_range_noflush (addr, end, prot, pages); +if (WARN_ON (addr >= end)) +return -EINVAL ; -for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { -int err; +if (WARN_ON (page_shift <PAGE_SHIFT )) +return -EINVAL ; - err =vmap_range_noflush (addr, addr + (1UL << page_shift), +if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || + page_shift ==PAGE_SHIFT ) { + err =vmap_small_pages_range_noflush (addr, end, prot, pages); + }else { +for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { + err =vmap_range_noflush (addr, addr + (1UL << page_shift),page_to_phys (pages[i]), prot, page_shift); -if (err) -return err; +if (err) +break ; - addr +=1UL << page_shift; + addr +=1UL << page_shift; + } } -return 0 ; +if (err) +__vunmap_range_noflush (start, end); + +/* 0 on success. */ +return err; } int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
@@ -714,7 +732,13 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +if (ret) +/* Cleanup KMSAN metadata. */ +kmsan_vunmap_range_noflush (addr, end); + +return ret; } static int __vmap_pages_range(unsigned long addr, unsigned long end,
<snip>
--
Uladzislau Rezki
Suren Baghdasaryan
maintainerRe: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions
Quoted 25 lines · Uladzislau Rezki
Quoted 23 lines · Hao Ge
__vmap_pages_range_noflush()and friends can install some PTEs before failing and leave them mapped, and the kernel callers do not agree on who cleans them up. For example,pcpu_map_pages()andkmsan_ioremap_page_range()unmap the leftovers themselves, whilevm_module_tags_populate()and the__GFP_NOFAILretry loop in__vmalloc_area_node()relied on the mapping functions cleaning up and did not call anything likevunmap_range()themselves. When the same range is mapped again, the attempt hits the leftovers and fails, withBUG()invmap_pte_range()for huge mappings.After discussing with Suren and Ulad, we decided the cleanup belongs to
__vmap_pages_range_noflush()and friends, so the callers no longer need to unmap the partial mappings themselves. Each function now undoes the PTEs it installed itself.The rollback calls the low-level
__vunmap_range_noflush(), it just clears the PTEs of the range it is given, which is all a rollback needs. It cannot usevunmap_range_noflush()because these mapping functions also map the KMSAN shadow and origin, and for a metadata range its hook would look up the metadata of the metadata, get 0 andBUG()on addr >= end. The failed mappings were never accessed, no TLB flush needed.
Quoted 2 lines
Quoted 2 lines
Fixes 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL")Fixes 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
I don't think this fixes 0f9b685626da. I mean it fixes it but indirectly. That patch expects vmap_pages_range() to undo the mappings when it fails, which is a reasonable assumption. By fixing vmap_pages_xxx we fix all its users, including vm_module_tags_populate(). So, it indirectly fixes several callers but when adding a Fixes tag we do not list all patches we indirectly fix.
Quoted 19 lines
Quoted 19 lines
--- mm/kmsan/shadow.c | 4 ++++ mm/vmalloc.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)mm/kmsan/shadow.c +3
-0@@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, o_pages, page_shift);kmsan_leave_runtime ();if (mapped) { +/* Undo the shadow mapping set up above. */ +kmsan_enter_runtime (); +__vunmap_range_noflush (shadow_start, shadow_end);
Quoted 8 lines
mm/kmsan/shadow.c int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, · line 267
Quoted 5 lines
mm/kmsan/shadow.c +1
-0+kmsan_leave_runtime (); err = mapped; goto ret; }Can we just do that inside the vmalloc? For example in the
__vmap_pages_range_noflush()as entry function for all(?) helpers? So we do not need do it manually?
Agree, that would be better.
Quoted 141 lines
Quoted 27 lines
mm/vmalloc.c +7
-0@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }@@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,if (!err) err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) +__vunmap_range_noflush (addr, end); +return err; }mm/vmalloc.c static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end, · line 683
Quoted 10 lines
mm/vmalloc.c +4
-0@@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }The only one user of that function is
__vmap_pages_range_noflush()so we can cover both cases there small and huge path.Is that enough just to do unroll in the below:
__vmap_pages_range_noflush()vmap_page_range()functions? It will also fix NOFAIL case:
<snip>
mm/vmalloc.c +39
-16@@ -359,10 +359,19 @@ int vmap_page_range(unsigned long addr, unsigned long end, err =vmap_range_noflush (addr, end, phys_addr,pgprot_nx (prot), ioremap_max_page_shift); +if (err) + goto error_cleanup_range; +flush_cache_vmap (addr, end); -if (!err) - err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, + err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) + goto error_cleanup_range; + +return 0 ; + +error_cleanup_range: +__vunmap_range_noflush (addr, end);return err; }@@ -683,26 +692,35 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr; +int err; -WARN_ON (page_shift <PAGE_SHIFT ); - -if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || - page_shift ==PAGE_SHIFT ) -return vmap_small_pages_range_noflush (addr, end, prot, pages); +if (WARN_ON (addr >= end)) +return -EINVAL ; -for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { -int err; +if (WARN_ON (page_shift <PAGE_SHIFT )) +return -EINVAL ; - err =vmap_range_noflush (addr, addr + (1UL << page_shift), +if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || + page_shift ==PAGE_SHIFT ) { + err =vmap_small_pages_range_noflush (addr, end, prot, pages); + }else { +for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { + err =vmap_range_noflush (addr, addr + (1UL << page_shift),page_to_phys (pages[i]), prot, page_shift); -if (err) -return err; +if (err) +break ; - addr +=1UL << page_shift; + addr +=1UL << page_shift; + } } -return 0 ; +if (err) +__vunmap_range_noflush (start, end); + +/* 0 on success. */ +return err; } int vmap_pages_range_noflush(unsigned long addr, unsigned long end,@@ -714,7 +732,13 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +if (ret) +/* Cleanup KMSAN metadata. */ +kmsan_vunmap_range_noflush (addr, end); + +return ret; } static int __vmap_pages_range(unsigned long addr, unsigned long end, --Uladzislau Rezki
Hao Ge
reviewerRe: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions
Hi Suren
Quoted 29 lines · Suren Baghdasaryan
Quoted 25 lines · Uladzislau Rezki
Quoted 23 lines · Hao Ge
__vmap_pages_range_noflush()and friends can install some PTEs before failing and leave them mapped, and the kernel callers do not agree on who cleans them up. For example,pcpu_map_pages()andkmsan_ioremap_page_range()unmap the leftovers themselves, whilevm_module_tags_populate()and the__GFP_NOFAILretry loop in__vmalloc_area_node()relied on the mapping functions cleaning up and did not call anything likevunmap_range()themselves. When the same range is mapped again, the attempt hits the leftovers and fails, withBUG()invmap_pte_range()for huge mappings.After discussing with Suren and Ulad, we decided the cleanup belongs to
__vmap_pages_range_noflush()and friends, so the callers no longer need to unmap the partial mappings themselves. Each function now undoes the PTEs it installed itself.The rollback calls the low-level
__vunmap_range_noflush(), it just clears the PTEs of the range it is given, which is all a rollback needs. It cannot usevunmap_range_noflush()because these mapping functions also map the KMSAN shadow and origin, and for a metadata range its hook would look up the metadata of the metadata, get 0 andBUG()on addr >= end. The failed mappings were never accessed, no TLB flush needed.Quoted 2 lines
Quoted 2 lines
Fixes 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL")Fixes 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
Quoted 7 lines
I don't think this fixes
0f9b685626da. I mean it fixes it but indirectly. That patch expectsvmap_pages_range()to undo the mappings when it fails, which is a reasonable assumption. By fixingvmap_pages_xxxwe fix all its users, includingvm_module_tags_populate(). So, it indirectly fixes several callers but when adding a Fixes tag we do not list all patches we indirectly fix.
Got it, learned something new here. I used to think we had to add a Fixes tag so stable folks know which branches to backport to.
I went back and re-read Documentation/process/stable-kernel-rules.rst, and turns out that's not how it works.
Both Fixes tags should be removed then. At the end of the day we're just normalizing the API; those other commits are merely beneficiaries of this change.
Thanks for your patient guidance.
Best Regards
Hao
Quoted 171 lines
Quoted 19 lines
Quoted 19 lines
--- mm/kmsan/shadow.c | 4 ++++ mm/vmalloc.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)mm/kmsan/shadow.c +3
-0@@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, o_pages, page_shift);kmsan_leave_runtime ();if (mapped) { +/* Undo the shadow mapping set up above. */ +kmsan_enter_runtime (); +__vunmap_range_noflush (shadow_start, shadow_end);Quoted 8 lines
mm/kmsan/shadow.c int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, · line 267
Quoted 5 lines
mm/kmsan/shadow.c +1
-0+kmsan_leave_runtime (); err = mapped; goto ret; }Can we just do that inside the vmalloc? For example in the
__vmap_pages_range_noflush()as entry function for all(?) helpers? So we do not need do it manually?Agree, that would be better.
Quoted 141 lines
Quoted 27 lines
mm/vmalloc.c +7
-0@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }@@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,if (!err) err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) +__vunmap_range_noflush (addr, end); +return err; }mm/vmalloc.c static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end, · line 683
Quoted 10 lines
mm/vmalloc.c +4
-0@@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }The only one user of that function is
__vmap_pages_range_noflush()so we can cover both cases there small and huge path.Is that enough just to do unroll in the below:
__vmap_pages_range_noflush()vmap_page_range()functions? It will also fix NOFAIL case:
<snip>
mm/vmalloc.c +39
-16@@ -359,10 +359,19 @@ int vmap_page_range(unsigned long addr, unsigned long end, err =vmap_range_noflush (addr, end, phys_addr,pgprot_nx (prot), ioremap_max_page_shift); +if (err) + goto error_cleanup_range; +flush_cache_vmap (addr, end); -if (!err) - err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, + err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) + goto error_cleanup_range; + +return 0 ; + +error_cleanup_range: +__vunmap_range_noflush (addr, end);return err; }@@ -683,26 +692,35 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr; +int err; -WARN_ON (page_shift <PAGE_SHIFT ); - -if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || - page_shift ==PAGE_SHIFT ) -return vmap_small_pages_range_noflush (addr, end, prot, pages); +if (WARN_ON (addr >= end)) +return -EINVAL ; -for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { -int err; +if (WARN_ON (page_shift <PAGE_SHIFT )) +return -EINVAL ; - err =vmap_range_noflush (addr, addr + (1UL << page_shift), +if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || + page_shift ==PAGE_SHIFT ) { + err =vmap_small_pages_range_noflush (addr, end, prot, pages); + }else { +for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { + err =vmap_range_noflush (addr, addr + (1UL << page_shift),page_to_phys (pages[i]), prot, page_shift); -if (err) -return err; +if (err) +break ; - addr +=1UL << page_shift; + addr +=1UL << page_shift; + } } -return 0 ; +if (err) +__vunmap_range_noflush (start, end); + +/* 0 on success. */ +return err; } int vmap_pages_range_noflush(unsigned long addr, unsigned long end,@@ -714,7 +732,13 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +if (ret) +/* Cleanup KMSAN metadata. */ +kmsan_vunmap_range_noflush (addr, end); + +return ret; } static int __vmap_pages_range(unsigned long addr, unsigned long end, --Uladzislau Rezki
Hao Ge
reviewerRe: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions
In reply to Uladzislau RezkiHi Uladzislau
Quoted 45 lines · Uladzislau Rezki
Quoted 44 lines · Hao Ge
__vmap_pages_range_noflush()and friends can install some PTEs before failing and leave them mapped, and the kernel callers do not agree on who cleans them up. For example,pcpu_map_pages()andkmsan_ioremap_page_range()unmap the leftovers themselves, whilevm_module_tags_populate()and the__GFP_NOFAILretry loop in__vmalloc_area_node()relied on the mapping functions cleaning up and did not call anything likevunmap_range()themselves. When the same range is mapped again, the attempt hits the leftovers and fails, withBUG()invmap_pte_range()for huge mappings.After discussing with Suren and Ulad, we decided the cleanup belongs to
__vmap_pages_range_noflush()and friends, so the callers no longer need to unmap the partial mappings themselves. Each function now undoes the PTEs it installed itself.The rollback calls the low-level
__vunmap_range_noflush(), it just clears the PTEs of the range it is given, which is all a rollback needs. It cannot usevunmap_range_noflush()because these mapping functions also map the KMSAN shadow and origin, and for a metadata range its hook would look up the metadata of the metadata, get 0 andBUG()on addr >= end. The failed mappings were never accessed, no TLB flush needed.Fixes 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL")Fixes 0f9b685626da ("alloc_tag: populate memory for module tags as needed")Reported-by Sashiko <[email protected]>Signed-off-by Hao Ge <[email protected]>--- mm/kmsan/shadow.c | 4 ++++ mm/vmalloc.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)mm/kmsan/shadow.c +3
-0@@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, o_pages, page_shift);kmsan_leave_runtime ();if (mapped) { +/* Undo the shadow mapping set up above. */ +kmsan_enter_runtime (); +__vunmap_range_noflush (shadow_start, shadow_end);
Quoted 9 lines
mm/kmsan/shadow.c int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, · line 267
Quoted 5 lines
mm/kmsan/shadow.c +1
-0+kmsan_leave_runtime (); err = mapped; goto ret; }Can we just do that inside the vmalloc? For example in the
__vmap_pages_range_noflush()as entry function for all(?) helpers? So we do not need do it manually?
The hunk was for the failure path in kmsan_vmap_pages_range_noflush(). Shadow and origin are mapped there as two separate calls, and when the origin one fails the shadow is already up. __vmap_pages_range_noflush() cannot do that undo, it only sees its own range.
kmsan_vmap_pages_range_noflush() has a single caller, so if we were to move this logic into vmalloc, it would require some modifications as follows:
@@ -712,9 +724,14 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
int ret = kmsan_vmap_pages_range_noflush(addr, end, prot, pages,
page_shift, gfp_mask);
+ if (!ret)
+ ret = __vmap_pages_range_noflush(addr, end, prot, pages,
+ page_shift);
+ /* Undo whatever was mapped before the failure. */
if (ret)
- return ret;
- return __vmap_pages_range_noflush(addr, end, prot, pages, page_shift);
+ kmsan_vunmap_range_noflush(addr, end);
+
+ return ret;
}
Quoted 39 lines
Quoted 26 lines
mm/vmalloc.c +7
-0@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }@@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,if (!err) err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) +__vunmap_range_noflush (addr, end); +return err; }mm/vmalloc.c static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end, · line 683
Quoted 10 lines
mm/vmalloc.c +4
-0@@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }The only one user of that function is
__vmap_pages_range_noflush()so we can cover both cases there small and huge path.
Quoted 7 lines
Is that enough just to do unroll in the below:
__vmap_pages_range_noflush()vmap_page_range()functions? It will also fix NOFAIL case:
I see. right, __vmap_pages_range_noflush() is just above the actual page‑table mapping, so it's low‑level enough. Thanks a lot for your prototype.
I'm tidying up the code and will post a REF patch afterwards.
Thanks
Best Regards
Hao
Quoted 94 lines
<snip>
mm/vmalloc.c +39
-16@@ -359,10 +359,19 @@ int vmap_page_range(unsigned long addr, unsigned long end, err =vmap_range_noflush (addr, end, phys_addr,pgprot_nx (prot), ioremap_max_page_shift); +if (err) + goto error_cleanup_range; +flush_cache_vmap (addr, end); -if (!err) - err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, + err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) + goto error_cleanup_range; + +return 0 ; + +error_cleanup_range: +__vunmap_range_noflush (addr, end);return err; }@@ -683,26 +692,35 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr; +int err; -WARN_ON (page_shift <PAGE_SHIFT ); - -if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || - page_shift ==PAGE_SHIFT ) -return vmap_small_pages_range_noflush (addr, end, prot, pages); +if (WARN_ON (addr >= end)) +return -EINVAL ; -for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { -int err; +if (WARN_ON (page_shift <PAGE_SHIFT )) +return -EINVAL ; - err =vmap_range_noflush (addr, addr + (1UL << page_shift), +if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || + page_shift ==PAGE_SHIFT ) { + err =vmap_small_pages_range_noflush (addr, end, prot, pages); + }else { +for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { + err =vmap_range_noflush (addr, addr + (1UL << page_shift),page_to_phys (pages[i]), prot, page_shift); -if (err) -return err; +if (err) +break ; - addr +=1UL << page_shift; + addr +=1UL << page_shift; + } } -return 0 ; +if (err) +__vunmap_range_noflush (start, end); + +/* 0 on success. */ +return err; } int vmap_pages_range_noflush(unsigned long addr, unsigned long end,@@ -714,7 +732,13 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +if (ret) +/* Cleanup KMSAN metadata. */ +kmsan_vunmap_range_noflush (addr, end); + +return ret; } static int __vmap_pages_range(unsigned long addr, unsigned long end, --Uladzislau Rezki
Hao Ge
reviewerRe: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions
Quoted 72 lines · Hao Ge
Hi Uladzislau
Quoted 45 lines · Uladzislau Rezki
Quoted 44 lines · Hao Ge
__vmap_pages_range_noflush()and friends can install some PTEs before failing and leave them mapped, and the kernel callers do not agree on who cleans them up. For example,pcpu_map_pages()andkmsan_ioremap_page_range()unmap the leftovers themselves, whilevm_module_tags_populate()and the__GFP_NOFAILretry loop in__vmalloc_area_node()relied on the mapping functions cleaning up and did not call anything likevunmap_range()themselves. When the same range is mapped again, the attempt hits the leftovers and fails, withBUG()invmap_pte_range()for huge mappings.After discussing with Suren and Ulad, we decided the cleanup belongs to
__vmap_pages_range_noflush()and friends, so the callers no longer need to unmap the partial mappings themselves. Each function now undoes the PTEs it installed itself.The rollback calls the low-level
__vunmap_range_noflush(), it just clears the PTEs of the range it is given, which is all a rollback needs. It cannot usevunmap_range_noflush()because these mapping functions also map the KMSAN shadow and origin, and for a metadata range its hook would look up the metadata of the metadata, get 0 andBUG()on addr >= end. The failed mappings were never accessed, no TLB flush needed.Fixes 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL")Fixes 0f9b685626da ("alloc_tag: populate memory for module tags as needed")Reported-by Sashiko <[email protected]>Signed-off-by Hao Ge <[email protected]>--- mm/kmsan/shadow.c | 4 ++++ mm/vmalloc.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)mm/kmsan/shadow.c +3
-0@@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, o_pages, page_shift);kmsan_leave_runtime ();if (mapped) { +/* Undo the shadow mapping set up above. */ +kmsan_enter_runtime (); +__vunmap_range_noflush (shadow_start, shadow_end);Quoted 9 lines
mm/kmsan/shadow.c int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, · line 267
Quoted 5 lines
mm/kmsan/shadow.c +1
-0+kmsan_leave_runtime (); err = mapped; goto ret; }Can we just do that inside the vmalloc? For example in the
__vmap_pages_range_noflush()as entry function for all(?) helpers? So we do not need do it manually?The hunk was for the failure path in
kmsan_vmap_pages_range_noflush(). Shadow and origin are mapped there as two separate calls, and when the origin one fails the shadow is already up.__vmap_pages_range_noflush()cannot do that undo, it only sees its own range.
kmsan_vmap_pages_range_noflush()has a single caller, so if we were to move this logic into vmalloc, it would require some modifications as follows:mm/vmalloc.c +2
-0@@ -712,9 +724,14 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,int ret =kmsan_vmap_pages_range_noflush (addr, end, prot, pages, page_shift, gfp_mask); +if (!ret) + ret = __vmap_pages_range_noflush(addr, end, prot, pages,
mm/vmalloc.c int vmap_pages_range_noflush(unsigned long addr, unsigned long end, · line 736
Quoted 10 lines
mm/vmalloc.c +5
-2+ page_shift); +/* Undo whatever was mapped before the failure. */ if (ret) -return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +kmsan_vunmap_range_noflush (addr, end); + +return ret; }
I'd rather put this logic inside kmsan_vmap_pages_range_noflush
This function should clean things up on its own, not depend on the caller to do it, just like kmsan_ioremap_page_range.
https://elixir.bootlin.com/linux/v7.3-rc5/source/mm/kmsan/hooks.c#L185
Uladzislau，WDYT?
Thanks
Best Regards
Hao
Quoted 151 lines
Quoted 39 lines
Quoted 26 lines
mm/vmalloc.c +7
-0@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }@@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,if (!err) err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) +__vunmap_range_noflush (addr, end); +return err; }mm/vmalloc.c static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end, · line 683
Quoted 10 lines
mm/vmalloc.c +4
-0@@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }The only one user of that function is
__vmap_pages_range_noflush()so we can cover both cases there small and huge path.Quoted 7 lines
Is that enough just to do unroll in the below:
__vmap_pages_range_noflush()vmap_page_range()functions? It will also fix NOFAIL case:
I see. right,
__vmap_pages_range_noflush()is just above the actual page‑table mapping, so it's low‑level enough. Thanks a lot for your prototype.I'm tidying up the code and will post a REF patch afterwards.
Thanks
Best Regards
HaoQuoted 94 lines
<snip>
mm/vmalloc.c +39
-16@@ -359,10 +359,19 @@ int vmap_page_range(unsigned long addr, unsigned long end, err =vmap_range_noflush (addr, end, phys_addr,pgprot_nx (prot), ioremap_max_page_shift); +if (err) + goto error_cleanup_range; +flush_cache_vmap (addr, end); -if (!err) - err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, + err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) + goto error_cleanup_range; + +return 0 ; + +error_cleanup_range: +__vunmap_range_noflush (addr, end);return err; }@@ -683,26 +692,35 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr; +int err; -WARN_ON (page_shift <PAGE_SHIFT ); - -if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || - page_shift ==PAGE_SHIFT ) -return vmap_small_pages_range_noflush (addr, end, prot, pages); +if (WARN_ON (addr >= end)) +return -EINVAL ; -for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { -int err; +if (WARN_ON (page_shift <PAGE_SHIFT )) +return -EINVAL ; - err =vmap_range_noflush (addr, addr + (1UL << page_shift), +if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || + page_shift ==PAGE_SHIFT ) { + err =vmap_small_pages_range_noflush (addr, end, prot, pages); + }else { +for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { + err =vmap_range_noflush (addr, addr + (1UL << page_shift),page_to_phys (pages[i]), prot, page_shift); -if (err) -return err; +if (err) +break ; - addr +=1UL << page_shift; + addr +=1UL << page_shift; + } } -return 0 ; +if (err) +__vunmap_range_noflush (start, end); + +/* 0 on success. */ +return err; } int vmap_pages_range_noflush(unsigned long addr, unsigned long end,@@ -714,7 +732,13 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +if (ret) +/* Cleanup KMSAN metadata. */ +kmsan_vunmap_range_noflush (addr, end); + +return ret; } static int __vmap_pages_range(unsigned long addr, unsigned long end, --Uladzislau Rezki
Hao Ge
reviewerRe: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions
In reply to Hao GeQuoted 72 lines · Hao Ge
Hi Uladzislau
Quoted 45 lines · Uladzislau Rezki
Quoted 44 lines · Hao Ge
__vmap_pages_range_noflush()and friends can install some PTEs before failing and leave them mapped, and the kernel callers do not agree on who cleans them up. For example,pcpu_map_pages()andkmsan_ioremap_page_range()unmap the leftovers themselves, whilevm_module_tags_populate()and the__GFP_NOFAILretry loop in__vmalloc_area_node()relied on the mapping functions cleaning up and did not call anything likevunmap_range()themselves. When the same range is mapped again, the attempt hits the leftovers and fails, withBUG()invmap_pte_range()for huge mappings.After discussing with Suren and Ulad, we decided the cleanup belongs to
__vmap_pages_range_noflush()and friends, so the callers no longer need to unmap the partial mappings themselves. Each function now undoes the PTEs it installed itself.The rollback calls the low-level
__vunmap_range_noflush(), it just clears the PTEs of the range it is given, which is all a rollback needs. It cannot usevunmap_range_noflush()because these mapping functions also map the KMSAN shadow and origin, and for a metadata range its hook would look up the metadata of the metadata, get 0 andBUG()on addr >= end. The failed mappings were never accessed, no TLB flush needed.Fixes 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL")Fixes 0f9b685626da ("alloc_tag: populate memory for module tags as needed")Reported-by Sashiko <[email protected]>Signed-off-by Hao Ge <[email protected]>--- mm/kmsan/shadow.c | 4 ++++ mm/vmalloc.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)mm/kmsan/shadow.c +3
-0@@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, o_pages, page_shift);kmsan_leave_runtime ();if (mapped) { +/* Undo the shadow mapping set up above. */ +kmsan_enter_runtime (); +__vunmap_range_noflush (shadow_start, shadow_end);Quoted 9 lines
mm/kmsan/shadow.c int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end, · line 267
Quoted 5 lines
mm/kmsan/shadow.c +1
-0+kmsan_leave_runtime (); err = mapped; goto ret; }Can we just do that inside the vmalloc? For example in the
__vmap_pages_range_noflush()as entry function for all(?) helpers? So we do not need do it manually?The hunk was for the failure path in
kmsan_vmap_pages_range_noflush(). Shadow and origin are mapped there as two separate calls, and when the origin one fails the shadow is already up.__vmap_pages_range_noflush()cannot do that undo, it only sees its own range.
kmsan_vmap_pages_range_noflush()has a single caller, so if we were to move this logic into vmalloc, it would require some modifications as follows:mm/vmalloc.c +2
-0@@ -712,9 +724,14 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,int ret =kmsan_vmap_pages_range_noflush (addr, end, prot, pages, page_shift, gfp_mask); +if (!ret) + ret = __vmap_pages_range_noflush(addr, end, prot, pages,
mm/vmalloc.c int vmap_pages_range_noflush(unsigned long addr, unsigned long end, · line 736
Quoted 10 lines
mm/vmalloc.c +5
-2+ page_shift); +/* Undo whatever was mapped before the failure. */ if (ret) -return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +kmsan_vunmap_range_noflush (addr, end); + +return ret; }
I'd rather put this logic inside kmsan_vmap_pages_range_noflush. This function should clean things up on its own, not depend on the caller to do it, just like kmsan_ioremap_page_range.
https://elixir.bootlin.com/linux/v7.3-rc5/source/mm/kmsan/hooks.c#L185
Uladzislau, WDYT?
Thanks
Best Regards
Hao
Quoted 151 lines
Quoted 39 lines
Quoted 26 lines
mm/vmalloc.c +7
-0@@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }@@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,if (!err) err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) +__vunmap_range_noflush (addr, end); +return err; }mm/vmalloc.c static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end, · line 683
Quoted 10 lines
mm/vmalloc.c +4
-0@@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,if (mask &ARCH_PAGE_TABLE_SYNC_MASK )arch_sync_kernel_mappings (start, end); +/* Undo the PTEs installed before the failure. */ +if (err) +__vunmap_range_noflush (start, end); +return err; }The only one user of that function is
__vmap_pages_range_noflush()so we can cover both cases there small and huge path.Quoted 7 lines
Is that enough just to do unroll in the below:
__vmap_pages_range_noflush()vmap_page_range()functions? It will also fix NOFAIL case:
I see. right,
__vmap_pages_range_noflush()is just above the actual page‑table mapping, so it's low‑level enough. Thanks a lot for your prototype.I'm tidying up the code and will post a REF patch afterwards.
Thanks
Best Regards
HaoQuoted 94 lines
<snip>
mm/vmalloc.c +39
-16@@ -359,10 +359,19 @@ int vmap_page_range(unsigned long addr, unsigned long end, err =vmap_range_noflush (addr, end, phys_addr,pgprot_nx (prot), ioremap_max_page_shift); +if (err) + goto error_cleanup_range; +flush_cache_vmap (addr, end); -if (!err) - err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, + err =kmsan_ioremap_page_range (addr, end, phys_addr, prot, ioremap_max_page_shift); +if (err) + goto error_cleanup_range; + +return 0 ; + +error_cleanup_range: +__vunmap_range_noflush (addr, end);return err; }@@ -683,26 +692,35 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end, pgprot_t prot, struct page **pages, unsigned int page_shift) { unsigned int i, nr = (end - addr) >> PAGE_SHIFT; +unsigned long start = addr; +int err; -WARN_ON (page_shift <PAGE_SHIFT ); - -if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || - page_shift ==PAGE_SHIFT ) -return vmap_small_pages_range_noflush (addr, end, prot, pages); +if (WARN_ON (addr >= end)) +return -EINVAL ; -for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { -int err; +if (WARN_ON (page_shift <PAGE_SHIFT )) +return -EINVAL ; - err =vmap_range_noflush (addr, addr + (1UL << page_shift), +if (!IS_ENABLED (CONFIG_HAVE_ARCH_HUGE_VMALLOC ) || + page_shift ==PAGE_SHIFT ) { + err =vmap_small_pages_range_noflush (addr, end, prot, pages); + }else { +for (i =0 ; i < nr; i +=1U << (page_shift -PAGE_SHIFT )) { + err =vmap_range_noflush (addr, addr + (1UL << page_shift),page_to_phys (pages[i]), prot, page_shift); -if (err) -return err; +if (err) +break ; - addr +=1UL << page_shift; + addr +=1UL << page_shift; + } } -return 0 ; +if (err) +__vunmap_range_noflush (start, end); + +/* 0 on success. */ +return err; } int vmap_pages_range_noflush(unsigned long addr, unsigned long end,@@ -714,7 +732,13 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,if (ret)return ret; -return __vmap_pages_range_noflush (addr, end, prot, pages, page_shift); + + ret =__vmap_pages_range_noflush (addr, end, prot, pages, page_shift); +if (ret) +/* Cleanup KMSAN metadata. */ +kmsan_vunmap_range_noflush (addr, end); + +return ret; } static int __vmap_pages_range(unsigned long addr, unsigned long end, --Uladzislau Rezki
Hao Ge
reviewerPATCH v11 3/7 alloc_tag: clean up the populate failure path
In reply to Hao GeThe reservation is already stored in the maple tree when vm_module_tags_populate() fails. A failed load never unloads the module, so nothing releases the entry. Release it and roll module_tags.size back. Without the rollback a concurrent load that already passed needs_section_mem() can reuse the freed gap, skip vm_module_tags_populate() and write to unmapped memory.
--- mm/alloc_tag.c | 3 +++ 1 file changed, 3 insertions(+)
mm/alloc_tag.c +3 -0
@@ -947,6 +947,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,return ret;if (module_tags.size < offset + size) { +unsigned long prev_size = module_tags.size;int grow_res; module_tags.size = offset + size;
@@ -961,6 +962,8 @@ 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); + module_tags.size = prev_size;return ERR_PTR (grow_res); } }
Signature · 1 line
2.25.1
Re: [PATCH v11 3/7] alloc_tag: clean up the populate failure path
Quoted 10 lines · Hao Ge
The reservation is already stored in the maple tree when
vm_module_tags_populate()fails. A failed load never unloads the module, so nothing releases the entry. Release it and roll module_tags.size back. Without the rollback a concurrent load that already passedneeds_section_mem()can reuse the freed gap, skipvm_module_tags_populate()and write to unmapped memory.Fixes 4835f747d3ed ("alloc_tag: support for page allocation tag compression")Fixes 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
I think 0f9b685626da is the only one being fixed here. 4835f747d3ed contains the issue simply because 0f9b685626da already had it.
Quoted 3 lines
Quoted 28 lines
--- mm/alloc_tag.c | 3 +++ 1 file changed, 3 insertions(+)mm/alloc_tag.c +3
-1@@ -947,6 +947,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,return ret;if (module_tags.size < offset + size) { +unsigned long prev_size = module_tags.size;int grow_res; module_tags.size = offset + size;@@ -961,6 +962,8 @@ 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); + module_tags.size = prev_size;return ERR_PTR (grow_res); } } --2.25.1
Hao Ge
reviewerPATCH v11 4/7 module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections
In reply to Hao GeSHF_ALLOC means, per the ELF spec, that a section occupies memory during process execution. Some module sections occupy memory outside the regular module layout, for example the percpu section with its per-CPU allocations. The loader currently excludes such a section from the layout by clearing its SHF_ALLOC, which overloads the flag with a loader-internal meaning. apply_relocations() needs a special case for the section, and find_sec(".data..percpu") returns different results before and after layout_and_allocate().
Introduce SH_ENTSIZE_STANDALONE to mark sections with a separate allocation. The percpu section is its first user. layout_sections() and move_module() skip marked sections, and apply_relocations() goes back to testing only SHF_ALLOC. Based on a patch by Petr Pavlu [1].
.data..percpu keeps SHF_ALLOC, so it would now show up under /sys/module/*/sections/. The section has one instance per CPU and no single address to report, and the entry never existed before, so skip it in add_sect_attrs(). add_notes_attrs() indexes its attrs[] array and skips it too. No functional or userspace-visible changes otherwise.
--- include/linux/module.h | 2 ++ kernel/module/internal.h | 8 ++++++++ kernel/module/kallsyms.c | 13 +++---------- kernel/module/main.c | 32 +++++++++++++++++--------------- kernel/module/sysfs.c | 17 +++++++++++++---- 5 files changed, 43 insertions(+), 29 deletions(-)
include/linux/module.h +2 -0
@@ -325,6 +325,8 @@ enum mod_mem_type {MOD_INIT_RODATA ,MOD_MEM_NUM_TYPES , + +MOD_STANDALONE =-2 , MOD_INVALID = -1, };
kernel/module/internal.h +8 -0
@@ -29,6 +29,14 @@#define SH_ENTSIZE_TYPE_MASK ((1UL << SH_ENTSIZE_TYPE_BITS) - 1)#define SH_ENTSIZE_OFFSET_MASK ((1UL << (BITS_PER_LONG - SH_ENTSIZE_TYPE_BITS)) - 1) +/* +* Marker for sections with a separate allocation, which are not placed +* into mod->mem[]. +*/ +#define SH_ENTSIZE_STANDALONE \ + (((unsigned long)MOD_STANDALONE & SH_ENTSIZE_TYPE_MASK) \ + << SH_ENTSIZE_TYPE_SHIFT) +/* Maximum number of characters written by module_flags() */ #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
kernel/module/kallsyms.c +3 -10
@@ -76,7 +76,7 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info) }static bool is_core_symbol (const Elf_Sym *src,const Elf_Shdr *sechdrs, -unsigned int shnum,unsigned int pcpundx) +unsigned int shnum) {const Elf_Shdr *sec;enum mod_mem_type type;
@@ -86,11 +86,6 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs, !src->st_name)return false; -#ifdef CONFIG_KALLSYMS_ALL -if (src->st_shndx == pcpundx) -return true; -#endif - sec = sechdrs + src->st_shndx; type = sec->sh_entsize >>SH_ENTSIZE_TYPE_SHIFT ; if (!(sec->sh_flags & SHF_ALLOC)
@@ -131,8 +126,7 @@ void layout_symtab(struct module *mod, struct load_info *info)/* Compute total space required for the core symbols' strtab. */ for (ndst = i =0 ; i < nsrc; i++) {if (i ==0 ||is_livepatch_module (mod) || -is_core_symbol (src + i, info->sechdrs, info->hdr->e_shnum, - info->index.pcpu)) { +is_core_symbol (src + i, info->sechdrs, info->hdr->e_shnum)) { strtab_size +=strlen (&info->strtab[src[i].st_name]) +1 ; ndst++; }
@@ -199,8 +193,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)for (ndst = i =0 ; i < kallsyms->num_symtab; i++) { kallsyms->typetab[i] =elf_type (src + i, info);if (i ==0 ||is_livepatch_module (mod) || -is_core_symbol (src + i, info->sechdrs, info->hdr->e_shnum, - info->index.pcpu)) { +is_core_symbol (src + i, info->sechdrs, info->hdr->e_shnum)) {ssize_t ret; mod->core_kallsyms.typetab[ndst] =
kernel/module/main.c +17 -15
@@ -1619,12 +1619,13 @@ static int apply_relocations(struct module *mod, const struct load_info *info)/* * Don't bother with non-allocated sections. -* An exception is the percpu section, which has separate allocations -* for individual CPUs. We relocate the percpu section in the initial -* ELF template and subsequently copy it to the per-CPU destinations. +* +* Note that .data..percpu has separate allocations for +* individual CPUs. We relocate the section in the +* initial ELF template and subsequently copy it to the +* per-CPU destinations. */ -if (!(info->sechdrs[infosec].sh_flags &SHF_ALLOC ) && - (!infosec || infosec != info->index.pcpu)) +if (!(info->sechdrs[infosec].sh_flags &SHF_ALLOC ))continue ;if (info->sechdrs[i].sh_flags &SHF_RELA_LIVEPATCH )
@@ -1715,7 +1716,7 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool iif ((s->sh_flags & masks[m][0 ]) != masks[m][0 ] || (s->sh_flags & masks[m][1 ]) - || s->sh_entsize != ~0UL + || s->sh_entsize != ~0UL /* offset or standalone */ || is_init !=module_init_layout_section (sname))continue ;
@@ -1745,16 +1746,10 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i/* * Lay out the SHF_ALLOC sections in a way not dissimilar to how ld * might -- code, read-only data, read-write data, small data. Tally -* sizes, and place the offsets into sh_entsize fields: high bit means it -* belongs in init. +* sizes, and place the offsets into sh_entsize fields. */ static void layout_sections (struct module *mod,struct load_info *info) { -unsigned int i; - -for (i =0 ; i < info->hdr->e_shnum; i++) - info->sechdrs[i].sh_entsize = ~0UL ; -pr_debug ("Core section allocation order for %s:\n" , mod->name);__layout_sections (mod, info, false);
@@ -2822,7 +2817,8 @@ static int move_module(struct module *mod, struct load_info *info)Elf_Shdr *shdr = &info->sechdrs[i];const char *sname; -if (!(shdr->sh_flags &SHF_ALLOC )) +if (!(shdr->sh_flags &SHF_ALLOC ) + || shdr->sh_entsize ==SH_ENTSIZE_STANDALONE )continue ; sname = info->secstrings + shdr->sh_name;
@@ -2954,6 +2950,7 @@ core_param(module_blacklist, module_blacklist, charp, 0400);static struct module *layout_and_allocate (struct load_info *info,int flags) {struct module *mod; +unsigned int i;int err;/* Allow arches to frob section contents and sizes. */
@@ -2967,8 +2964,13 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)if (err <0 )return ERR_PTR (err); +/* Repurpose sh_entsize to track where each section is allocated. */ +for (i =0 ; i < info->hdr->e_shnum; i++) + info->sechdrs[i].sh_entsize = ~0UL ; +/* We will do a special allocation for per-cpu sections later. */ - info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long )SHF_ALLOC ; +if (info->index.pcpu) + info->sechdrs[info->index.pcpu].sh_entsize =SH_ENTSIZE_STANDALONE ;/* * Mark relevant sections as SHF_RO_AFTER_INIT so layout_sections() can
kernel/module/sysfs.c +13 -4
@@ -62,6 +62,15 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)kfree (sect_attrs); } +/* +* .data..percpu has a separate allocation per CPU and no single +* address to report. +*/ +static bool sect_visible (const struct load_info *info,unsigned int i) +{ +return !sect_empty (&info->sechdrs[i]) && i != info->index.pcpu; +} +static int add_sect_attrs (struct module *mod,const struct load_info *info) {struct module_sect_attrs *sect_attrs;
@@ -72,7 +81,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)/* Count loaded sections and allocate structures */ for (i =0 ; i < info->hdr->e_shnum; i++) -if (!sect_empty (&info->sechdrs[i])) +if (sect_visible (info, i)) nloaded++; sect_attrs =kzalloc_flex (*sect_attrs, attrs, nloaded);if (!sect_attrs)
@@ -92,7 +101,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)for (i =0 ; i < info->hdr->e_shnum; i++) {Elf_Shdr *sec = &info->sechdrs[i]; -if (sect_empty (sec)) +if (!sect_visible (info, i))continue ;sysfs_bin_attr_init (sattr); sattr->attr.name =
@@ -159,7 +168,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)/* Count notes sections and allocate structures. */ notes =0 ;for (i =0 ; i < info->hdr->e_shnum; i++) -if (!sect_empty (&info->sechdrs[i]) && +if (sect_visible (info, i) && info->sechdrs[i].sh_type ==SHT_NOTE ) ++notes;
@@ -181,7 +190,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info) nattr = ¬es_attrs->attrs[0 ];for (loaded = i =0 ; i < info->hdr->e_shnum; ++i) { -if (sect_empty (&info->sechdrs[i])) +if (!sect_visible (info, i))continue ;if (info->sechdrs[i].sh_type ==SHT_NOTE ) {sysfs_bin_attr_init (nattr);
Signature · 1 line
2.25.1
Hao Ge
reviewerPATCH v11 5/7 module: allocate codetag sections before the regular module layout
In reply to Hao GeWhether a codetag section goes to the codetag region is decided by layout_sections() and asked again in move_module(). A concurrent load can shut profiling down in between, and move_module() then copies the section to offset 0 of its regular destination, overwriting whatever is there.
Decide and allocate in one pass, before the layout. Allocation errors fail the load. On a tag area overflow profiling is already disabled, so -EAGAIN makes the section fall back to regular module data and the module still loads. The reservation is released and module_tags.size rolled back, so a concurrent load which already passed needs_section_mem() does not skip vm_module_tags_populate().
An SHT_NOBITS codetag section is zeroed explicitly, the tag area pages are not zeroed on allocation.
When profiling was toggled off the overflow check did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted /proc/allocinfo. The check no longer depends on mem_alloc_profiling_enabled().
Based on a patch by Petr Pavlu [1].
--- kernel/module/main.c | 101 +++++++++++++++++++++++-------------------- mm/alloc_tag.c | 9 ++-- 2 files changed, 60 insertions(+), 50 deletions(-)
kernel/module/main.c +54 -47
@@ -1723,20 +1723,6 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool iif (WARN_ON_ONCE (type ==MOD_INVALID ))continue ; -/* -* Do not allocate codetag memory as we load it into -* preallocated contiguous memory. -*/ -if (codetag_needs_module_section (mod, sname, s->sh_size)) { -/* -* s->sh_entsize won't be used but populate the -* type field to avoid confusion. -*/ - s->sh_entsize = ((unsigned long )(type) &SH_ENTSIZE_TYPE_MASK ) - <<SH_ENTSIZE_TYPE_SHIFT ; -continue ; - } - s->sh_entsize =module_get_offset_and_type (mod, type, s, i);pr_debug ("\t%s\n" , sname); }
@@ -2795,7 +2781,6 @@ static int move_module(struct module *mod, struct load_info *info) {int i, ret;enum mod_mem_type t =MOD_MEM_NUM_TYPES ; -bool codetag_section_found = false;for_each_mod_mem_type (type) {if (!mod->mem[type].size) {
@@ -2815,35 +2800,13 @@ static int move_module(struct module *mod, struct load_info *info)for (i =0 ; i < info->hdr->e_shnum; i++) {void *dest;Elf_Shdr *shdr = &info->sechdrs[i]; -const char *sname;if (!(shdr->sh_flags &SHF_ALLOC ) || shdr->sh_entsize ==SH_ENTSIZE_STANDALONE )continue ; - sname = info->secstrings + shdr->sh_name; -/* -* Load codetag sections separately as they might still be used -* after module unload. -*/ -if (codetag_needs_module_section (mod, sname, shdr->sh_size)) { - dest =codetag_alloc_module_section (mod, sname, shdr->sh_size, -arch_mod_section_prepend (mod, i), shdr->sh_addralign); -if (WARN_ON (!dest)) { - ret = -EINVAL ; - goto out_err; - } -if (IS_ERR (dest)) { - ret =PTR_ERR (dest); - goto out_err; - } - codetag_section_found = true; - }else { -enum mod_mem_type type = shdr->sh_entsize >>SH_ENTSIZE_TYPE_SHIFT ; -unsigned long offset = shdr->sh_entsize &SH_ENTSIZE_OFFSET_MASK ; - - dest = mod->mem[type].base + offset; - } + dest = mod->mem[shdr->sh_entsize >>SH_ENTSIZE_TYPE_SHIFT ].base + + (shdr->sh_entsize &SH_ENTSIZE_OFFSET_MASK );if (shdr->sh_type !=SHT_NOBITS ) {/*
@@ -2875,8 +2838,6 @@ static int move_module(struct module *mod, struct load_info *info)module_memory_restore_rox (mod);while (t--)module_memory_free (mod, t); -if (codetag_section_found) -codetag_free_module_sections (mod);return ret; }
@@ -2947,6 +2908,49 @@ static bool blacklisted(const char *module_name) }core_param (module_blacklist, module_blacklist, charp,0400 ); +/* +* Allocate codetag sections separately. They are loaded into preallocated +* contiguous memory because they may still be used after the module is +* unloaded. +* +* If the separate allocation overflows, allocate the section normally +* so that the module can still be loaded. +*/ +static int allocate_codetag_sections (struct load_info *info) +{ +for (unsigned int i =1 ; i < info->hdr->e_shnum; i++) { +Elf_Shdr *shdr = &info->sechdrs[i]; +const char *sname = info->secstrings + shdr->sh_name; +void *dest; + +if (!codetag_needs_module_section (info->mod, sname, shdr->sh_size)) +continue ; + + dest =codetag_alloc_module_section (info->mod, sname, shdr->sh_size, +arch_mod_section_prepend (info->mod, i), shdr->sh_addralign); +if (WARN_ON (!dest)) { +codetag_free_module_sections (info->mod); +return -EINVAL ; + } +if (dest ==ERR_PTR (-EAGAIN )) +/* Allocate the section as a regular section. */ +continue ; +if (IS_ERR (dest)) { +codetag_free_module_sections (info->mod); +return PTR_ERR (dest); + } + +if (shdr->sh_type !=SHT_NOBITS ) +memcpy (dest, (void *)shdr->sh_addr, shdr->sh_size); +else +memset (dest,0 , shdr->sh_size); + shdr->sh_addr = (unsigned long )dest; + shdr->sh_entsize =SH_ENTSIZE_STANDALONE ; + } + +return 0 ; +} +static struct module *layout_and_allocate (struct load_info *info,int flags) {struct module *mod;
@@ -2979,18 +2983,21 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)*/ module_mark_ro_after_init (info->hdr, info->sechdrs, info->secstrings); -/* -* Determine total sizes, and put offsets in sh_entsize. For now -* this is done generically; there doesn't appear to be any -* special cases for the architectures. -*/ +/* Allow codetag sections to be allocated separately first. */ + err =allocate_codetag_sections (info); +if (err) +return ERR_PTR (err); + +/* Determine total sizes and put offsets in sh_entsize. */ layout_sections (info->mod, info);layout_symtab (info->mod, info);/* Allocate and move to the final place */ err =move_module (info->mod, info); -if (err) +if (err) { +codetag_free_module_sections (info->mod);return ERR_PTR (err); + }/* Module has been copied to its final place now: return it. */ mod = (void *)info->sechdrs[info->index.mod].sh_addr;
mm/alloc_tag.c +6 -3
@@ -951,10 +951,13 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,int grow_res; module_tags.size = offset + size; -if (mem_alloc_profiling_enabled () && !tags_addressable ()) { +if (!tags_addressable ()) {shutdown_mem_profiling (true); -pr_warn ("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n" , - mod->name,NR_UNUSED_PAGEFLAG_BITS ); +pr_warn_once ("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n" , + mod->name,NR_UNUSED_PAGEFLAG_BITS ); +release_module_tags (mod, false); + module_tags.size = prev_size; +return ERR_PTR (-EAGAIN ); } grow_res =vm_module_tags_populate ();
Signature · 1 line
2.25.1
Hao Ge
reviewerPATCH v11 6/7 alloc_tag: skip percpu counter allocation when profiling is disabled
In reply to Hao GeAfter shutdown_mem_profiling() clears mem_profiling_support, needs_section_mem() returns false, so later modules have their codetag section placed as regular data and never enter the alloc_tag maple tree. codetag_load_module() still called load_module(), which allocated a percpu counter for every tag; release_module_tags() could not find these modules on unload, so the counters leaked.
Return -EOPNOTSUPP from load_module() when profiling is off:codetag_module_init() drops the module's cmod, no counters are allocated and the module loads without its tags. codetag_unload_module() now always calls free_section_mem(), since a module whose module_load() returned -EOPNOTSUPP is not in the idr but may still hold a reserved section.
--- lib/codetag.c | 10 ++++++++-- mm/alloc_tag.c | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-)
lib/codetag.c +8 -2
@@ -240,7 +240,9 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)if (err <0 ) {kfree (cmod); -return err; +/* -EOPNOTSUPP means we can load the module without its tag. */ +if (err != -EOPNOTSUPP ) +return err; }return 0 ;
@@ -388,7 +390,11 @@ void codetag_unload_module(struct module *mod) ++cttype->content_id; }up_write (&cttype->mod_lock); -if (found && cttype->desc.free_section_mem) +/* +* A module whose module_load() returned -EOPNOTSUPP is not +* in the idr but may still hold reserved section memory. +*/ +if (cttype->desc.free_section_mem) cttype->desc.free_section_mem (mod, true); }mutex_unlock (&codetag_lock);
mm/alloc_tag.c +4 -0
@@ -981,6 +981,10 @@ static int load_module(struct module *mod, struct codetag *start, struct codetagstruct alloc_tag *stop_tag;struct alloc_tag *tag; +/* Profiling disabled: load the module without its tags. */ +if (!mem_profiling_support) +return -EOPNOTSUPP ; +/* percpu counters for core allocations are already statically allocated */ if (!mod)return 0 ;
Signature · 1 line
2.25.1
[email protected]
AI reviewRe: [PATCH v11 6/7] alloc_tag: skip percpu counter allocation when profiling is disabled
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mm: crash on freeing static percpu variable when module load fails or profiling disabled --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: crash on freeing static percpu variable when module load fails or profiling disabled --
commit 1ed77a1b45b5a7a434417c336e13dff08f4838d1
Author: Hao Ge <[email protected]>
alloc_tag: skip percpu counter allocation when profiling is disabled
After shutdown_mem_profiling() clears mem_profiling_support, needs_section_mem() returns false, so later modules have their codetag section placed as regular data and never enter the alloc_tag maple tree. codetag_load_module() still called load_module(), which allocated a percpu counter for every tag; release_module_tags() could not find these modules on unload, so the counters leaked.
Quoted 4 lines
lib/codetag.c +0
-0
[ ... ]
lib/codetag.c void codetag_unload_module(struct module *mod) · line 400
Quoted 13 lines
lib/codetag.c +5
-1@@ -388,7 +390,11 @@ void codetag_unload_module(struct module *mod) ++cttype->content_id; }up_write (&cttype->mod_lock); -if (found && cttype->desc.free_section_mem) +/* +* A module whose module_load() returned -EOPNOTSUPP is not +* in the idr but may still hold reserved section memory. +*/ +if (cttype->desc.free_section_mem) cttype->desc.free_section_mem (mod, true); }mutex_unlock (&codetag_lock);
[Severity: High]
Does this unconditional call to free_section_mem() lead to a crash when freeing statically allocated percpu memory?
On architectures defining CONFIG_ARCH_MODULE_NEEDS_WEAK_PER_CPU, DEFINE_ALLOC_TAG initializes tag->counters to the statically allocated &_shared_alloc_tag.
If a module load skips dynamic counter allocation (either because mem_profiling_support is false and load_module() returns -EOPNOTSUPP, or if alloc_percpu() fails with -ENOMEM), tag->counters remains pointing to &_shared_alloc_tag.
Because free_section_mem() is now called unconditionally here on module unload or load failure, it triggers the cleanup path for these uninitialized tags:release_module_tags() -> clean_unused_counters()
In clean_unused_counters(), since the static _shared_alloc_tag has 0 bytes, it unconditionally calls free_percpu(tag->counters):
mm/alloc_tag.c:clean_unused_counters() {
...
if (!counter.bytes) {
free_percpu(tag->counters);
tag->counters = NULL;
...
}Calling free_percpu() on a statically allocated per-cpu variable that doesn't belong to a managed chunk can corrupt memory and cause a crash.
Quoted 15 lines
mm/alloc_tag.c +4
-0@@ -981,6 +981,10 @@ static int load_module(struct module *mod, struct codetag *start, struct codetagstruct alloc_tag *stop_tag;struct alloc_tag *tag; +/* Profiling disabled: load the module without its tags. */ +if (!mem_profiling_support) +return -EOPNOTSUPP ; +/* percpu counters for core allocations are already statically allocated */ if (!mod)return 0 ;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Hao Ge
reviewerPATCH v11 7/7 alloc_tag: fix the /proc/allocinfo lifecycle
In reply to Hao Geshutdown_mem_profiling() calls remove_proc_entry() from reserve_module_tags(), which runs under mod_lock held for write. remove_proc_entry() waits for readers, and a reader takes mod_lock for read in allocinfo_start():
CPU0 (insmod) CPU1 (read /proc/allocinfo)
---------------- ----------------------------
reserve_module_tags()
down_write(&mod_lock) [held]
use_pde() [in_use++]
allocinfo_start()
down_read(&mod_lock) <- blocks
shutdown_mem_profiling()
remove_proc_entry()
wait for in_use == 0 <- blocksMove remove_proc_entry() to a workqueue.
The deferred removal also affects alloc_tag_init(). The file is created before the type, so on a failure it is still there with alloc_tag_cttype NULL or an error pointer, and a reader panics in allocinfo_start(). Create the file at the end of alloc_tag_init() instead, a failed init leaves nothing behind.
If proc_create() fails, the codetag type and the module tags memory leak. Call codetag_unregister_type() and free the memory.
alloc_tag_cttype can now be freed at runtime. alloc_tag_top_users() reads it from __show_mem() without locks, read the pointer under rcu_read_lock() and take mod_lock before dropping the RCU lock, the type stays alive until then. The only caller never sleeps, drop the can_sleep argument.
--- include/linux/alloc_tag.h | 2 +- include/linux/codetag.h | 2 ++ lib/codetag.c | 26 ++++++++++++++++++++ mm/alloc_tag.c | 52 +++++++++++++++++++++++++++------------ mm/show_mem.c | 2 +- 5 files changed, 66 insertions(+), 18 deletions(-)
include/linux/alloc_tag.h +1 -1
@@ -81,7 +81,7 @@ struct codetag_bytes {s64 bytes; }; -size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count,bool can_sleep); +size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count);static inline struct alloc_tag *ct_to_alloc_tag (struct codetag *ct) {
include/linux/codetag.h +2 -0
@@ -87,6 +87,8 @@ void codetag_to_text(struct seq_buf *out, struct codetag *ct);struct codetag_type *codetag_register_type (const struct codetag_type_desc *desc); +void codetag_unregister_type (struct codetag_type *cttype); +#if defined(CONFIG_CODE_TAGGING ) && defined(CONFIG_MODULES ) bool codetag_needs_module_section(struct module *mod, const char *name,
lib/codetag.c +26 -0
@@ -429,3 +429,29 @@ codetag_register_type(const struct codetag_type_desc *desc)return cttype; } + +/** +* codetag_unregister_type - unregister a codetag type +* @cttype: the codetag type to unregister +* +* Undo codetag_register_type() and free @cttype. The caller must make +* sure no lockless reader still uses @cttype, e.g. clear the pointer +* to it and wait for an RCU grace period first. +*/ +void __init codetag_unregister_type (struct codetag_type *cttype) +{ +struct codetag_module *cmod; +unsigned long id, tmp; + +mutex_lock (&codetag_lock); +list_del (&cttype->link); +mutex_unlock (&codetag_lock); + +down_write (&cttype->mod_lock); +idr_for_each_entry_ul (&cttype->mod_idr, cmod, tmp, id) +kfree (cmod); +idr_destroy (&cttype->mod_idr); +up_write (&cttype->mod_lock); + +kfree (cttype); +}
mm/alloc_tag.c +36 -16
110 lines not on this page · raw message
mm/show_mem.c +1 -1
@@ -439,7 +439,7 @@ void __show_mem(unsigned int filter, const nodemask_t *nodemask,struct codetag_bytes tags[10 ];size_t i, nr; - nr =alloc_tag_top_users (tags,ARRAY_SIZE (tags), false); + nr =alloc_tag_top_users (tags,ARRAY_SIZE (tags));if (nr) {pr_notice ("Memory allocations (profiling is currently turned %s):\n" ,mem_alloc_profiling_enabled () ?"on" :"off" );
Signature · 1 line
2.25.1
Suren Baghdasaryan
maintainerRe: [PATCH v11 7/7] alloc_tag: fix the /proc/allocinfo lifecycle
Quoted 125 lines · Hao Ge
shutdown_mem_profiling()callsremove_proc_entry()fromreserve_module_tags(), which runs undermod_lockheld for write.remove_proc_entry()waits for readers, and a reader takesmod_lockfor read inallocinfo_start():CPU0 (insmod) CPU1 (read
/proc/allocinfo)
---------------- ----------------------------
reserve_module_tags()down_write(&mod_lock) [held] use_pde() [in_use++] allocinfo_start() down_read(&mod_lock) <- blocks shutdown_mem_profiling() remove_proc_entry() wait for in_use == 0 <- blocksMove
remove_proc_entry()to a workqueue.The deferred removal also affects
alloc_tag_init(). The file is created before the type, so on a failure it is still there withalloc_tag_cttypeNULL or an error pointer, and a reader panics inallocinfo_start(). Create the file at the end ofalloc_tag_init()instead, a failed init leaves nothing behind.If
proc_create()fails, the codetag type and the module tags memory leak. Callcodetag_unregister_type()and free the memory.
alloc_tag_cttypecan now be freed at runtime.alloc_tag_top_users()reads it from__show_mem()without locks, read the pointer underrcu_read_lock()and takemod_lockbefore dropping the RCU lock, the type stays alive until then. The only caller never sleeps, drop thecan_sleepargument.Reported-by Sashiko <[email protected]>Fixes 4835f747d3ed ("alloc_tag: support for page allocation tag compression")Signed-off-by Hao Ge <[email protected]>--- include/linux/alloc_tag.h | 2 +- include/linux/codetag.h | 2 ++ lib/codetag.c | 26 ++++++++++++++++++++ mm/alloc_tag.c | 52 +++++++++++++++++++++++++++------------ mm/show_mem.c | 2 +- 5 files changed, 66 insertions(+), 18 deletions(-)include/linux/alloc_tag.h +1
-1@@ -81,7 +81,7 @@ struct codetag_bytes {s64 bytes; }; -size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count,bool can_sleep); +size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count);static inline struct alloc_tag *ct_to_alloc_tag (struct codetag *ct) {include/linux/codetag.h +2
-0@@ -87,6 +87,8 @@ void codetag_to_text(struct seq_buf *out, struct codetag *ct);struct codetag_type *codetag_register_type (const struct codetag_type_desc *desc); +void codetag_unregister_type (struct codetag_type *cttype); +#if defined(CONFIG_CODE_TAGGING ) && defined(CONFIG_MODULES ) bool codetag_needs_module_section(struct module *mod, const char *name,lib/codetag.c +26
-0@@ -429,3 +429,29 @@ codetag_register_type(const struct codetag_type_desc *desc)return cttype; } + +/** +* codetag_unregister_type - unregister a codetag type +* @cttype: the codetag type to unregister +* +* Undo codetag_register_type() and free @cttype. The caller must make +* sure no lockless reader still uses @cttype, e.g. clear the pointer +* to it and wait for an RCU grace period first. +*/ +void __init codetag_unregister_type (struct codetag_type *cttype) +{ +struct codetag_module *cmod; +unsigned long id, tmp; + +mutex_lock (&codetag_lock); +list_del (&cttype->link); +mutex_unlock (&codetag_lock); + +down_write (&cttype->mod_lock); +idr_for_each_entry_ul (&cttype->mod_idr, cmod, tmp, id) +kfree (cmod); +idr_destroy (&cttype->mod_idr); +up_write (&cttype->mod_lock); + +kfree (cttype); +}mm/alloc_tag.c +2
-1@@ -15,6 +15,7 @@#include <linux/seq_file.h> #include <linux/string_choices.h> #include <linux/vmalloc.h> +#include <linux/workqueue.h> #include <linux/kmemleak.h> #include <uapi/linux/alloc_tag.h> @@ -484,22 +485,28 @@ static const struct proc_ops allocinfo_proc_ops = {#endif }; -size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count,bool can_sleep) +size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count) {
mm/alloc_tag.c static const struct proc_ops allocinfo_proc_ops = { · line 498
Quoted 10 lines
mm/alloc_tag.c +4
-1struct codetag_iterator iter; +struct codetag_type *cttype;struct codetag *ct;struct codetag_bytes n;unsigned int i, nr =0 ; +bool locked; -if (IS_ERR_OR_NULL (alloc_tag_cttype)) +rcu_read_lock (); + cttype =READ_ONCE (alloc_tag_cttype);
Code looks correct to me but if you are changing alloc_tag_cttype to
be accessed under RCU, then you should declare it as __rcu and use rcu_assign_pointer()/rcu_access_pointer()/rcu_dereference()/rcu_dereference_protected().
Quoted 108 lines
mm/alloc_tag.c +30
-14+if (IS_ERR_OR_NULL (cttype)) { +rcu_read_unlock ();return 0 ; + } -if (can_sleep) -codetag_lock_module_list (alloc_tag_cttype); -else if (!codetag_trylock_module_list (alloc_tag_cttype)) + locked =codetag_trylock_module_list (cttype); +rcu_read_unlock (); +if (!locked)return 0 ; - iter =codetag_get_ct_iter (alloc_tag_cttype); + iter =codetag_get_ct_iter (cttype);while ((ct =codetag_next_ct (&iter))) {struct alloc_tag_counters counter =alloc_tag_read (ct_to_alloc_tag (ct));@@ -520,7 +527,7 @@ size_t alloc_tag_top_users(struct codetag_bytes *tags, size_t count, bool can_sl } } -codetag_unlock_module_list (alloc_tag_cttype); +codetag_unlock_module_list (cttype);return nr; }@@ -591,6 +598,13 @@ void pgalloc_tag_swap(struct folio *new, struct folio *old)put_page_tag_ref (handle_new); } +static void remove_allocinfo_file (struct work_struct *work) +{ +remove_proc_entry (ALLOCINFO_FILE_NAME ,NULL ); +} + +static DECLARE_WORK(remove_allocinfo_work, remove_allocinfo_file); +static void shutdown_mem_profiling (bool remove_file) {if (mem_alloc_profiling_enabled ())@@ -600,7 +614,7 @@ static void shutdown_mem_profiling(bool remove_file)return ;if (remove_file) -remove_proc_entry (ALLOCINFO_FILE_NAME ,NULL ); +schedule_work (&remove_allocinfo_work); mem_profiling_support = false; }@@ -1351,16 +1365,10 @@ static int __init alloc_tag_init(void)return 0 ; } -if (!proc_create (ALLOCINFO_FILE_NAME ,0400 ,NULL , &allocinfo_proc_ops)) { -pr_err ("Failed to create %s file\n" ,ALLOCINFO_FILE_NAME ); -shutdown_mem_profiling (false); -return -ENOMEM ; - } - res =alloc_mod_tags_mem ();if (res) {pr_err ("Failed to reserve address space for module tags, errno = %d\n" , res); -shutdown_mem_profiling (true); +shutdown_mem_profiling (false);return res; }@@ -1368,10 +1376,22 @@ static int __init alloc_tag_init(void)if (IS_ERR (alloc_tag_cttype)) {pr_err ("Allocation tags registration failed, errno = %pe\n" , alloc_tag_cttype);free_mod_tags_mem (); -shutdown_mem_profiling (true); +shutdown_mem_profiling (false);return PTR_ERR (alloc_tag_cttype); } +if (!proc_create (ALLOCINFO_FILE_NAME ,0400 ,NULL , &allocinfo_proc_ops)) { +struct codetag_type *cttype = alloc_tag_cttype; + +pr_err ("Failed to create %s file\n" ,ALLOCINFO_FILE_NAME ); +shutdown_mem_profiling (false); +WRITE_ONCE (alloc_tag_cttype,NULL ); +synchronize_rcu (); +codetag_unregister_type (cttype); +free_mod_tags_mem (); +return -ENOMEM ; + } +return 0 ; }module_init (alloc_tag_init);mm/show_mem.c +1
-2@@ -439,7 +439,7 @@ void __show_mem(unsigned int filter, const nodemask_t *nodemask,struct codetag_bytes tags[10 ];size_t i, nr; - nr =alloc_tag_top_users (tags,ARRAY_SIZE (tags), false); + nr =alloc_tag_top_users (tags,ARRAY_SIZE (tags));if (nr) {pr_notice ("Memory allocations (profiling is currently turned %s):\n" ,mem_alloc_profiling_enabled () ?"on" :"off" ); --2.25.1
Hao Ge
reviewerRe: [PATCH v11 7/7] alloc_tag: fix the /proc/allocinfo lifecycle
Hi Suren
Quoted 133 lines · Suren Baghdasaryan
Quoted 132 lines · Hao Ge
shutdown_mem_profiling()callsremove_proc_entry()fromreserve_module_tags(), which runs undermod_lockheld for write.remove_proc_entry()waits for readers, and a reader takesmod_lockfor read inallocinfo_start():CPU0 (insmod) CPU1 (read
/proc/allocinfo)
---------------- ----------------------------
reserve_module_tags()down_write(&mod_lock) [held] use_pde() [in_use++] allocinfo_start() down_read(&mod_lock) <- blocks shutdown_mem_profiling() remove_proc_entry() wait for in_use == 0 <- blocksMove
remove_proc_entry()to a workqueue.The deferred removal also affects
alloc_tag_init(). The file is created before the type, so on a failure it is still there withalloc_tag_cttypeNULL or an error pointer, and a reader panics inallocinfo_start(). Create the file at the end ofalloc_tag_init()instead, a failed init leaves nothing behind.If
proc_create()fails, the codetag type and the module tags memory leak. Callcodetag_unregister_type()and free the memory.
alloc_tag_cttypecan now be freed at runtime.alloc_tag_top_users()reads it from__show_mem()without locks, read the pointer underrcu_read_lock()and takemod_lockbefore dropping the RCU lock, the type stays alive until then. The only caller never sleeps, drop thecan_sleepargument.Reported-by Sashiko <[email protected]>Fixes 4835f747d3ed ("alloc_tag: support for page allocation tag compression")Signed-off-by Hao Ge <[email protected]>--- include/linux/alloc_tag.h | 2 +- include/linux/codetag.h | 2 ++ lib/codetag.c | 26 ++++++++++++++++++++ mm/alloc_tag.c | 52 +++++++++++++++++++++++++++------------ mm/show_mem.c | 2 +- 5 files changed, 66 insertions(+), 18 deletions(-)include/linux/alloc_tag.h +1
-1@@ -81,7 +81,7 @@ struct codetag_bytes {s64 bytes; }; -size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count,bool can_sleep); +size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count);static inline struct alloc_tag *ct_to_alloc_tag (struct codetag *ct) {include/linux/codetag.h +2
-0@@ -87,6 +87,8 @@ void codetag_to_text(struct seq_buf *out, struct codetag *ct);struct codetag_type *codetag_register_type (const struct codetag_type_desc *desc); +void codetag_unregister_type (struct codetag_type *cttype); +#if defined(CONFIG_CODE_TAGGING ) && defined(CONFIG_MODULES ) bool codetag_needs_module_section(struct module *mod, const char *name,lib/codetag.c +26
-0@@ -429,3 +429,29 @@ codetag_register_type(const struct codetag_type_desc *desc)return cttype; } + +/** +* codetag_unregister_type - unregister a codetag type +* @cttype: the codetag type to unregister +* +* Undo codetag_register_type() and free @cttype. The caller must make +* sure no lockless reader still uses @cttype, e.g. clear the pointer +* to it and wait for an RCU grace period first. +*/ +void __init codetag_unregister_type (struct codetag_type *cttype) +{ +struct codetag_module *cmod; +unsigned long id, tmp; + +mutex_lock (&codetag_lock); +list_del (&cttype->link); +mutex_unlock (&codetag_lock); + +down_write (&cttype->mod_lock); +idr_for_each_entry_ul (&cttype->mod_idr, cmod, tmp, id) +kfree (cmod); +idr_destroy (&cttype->mod_idr); +up_write (&cttype->mod_lock); + +kfree (cttype); +}mm/alloc_tag.c +4
-1@@ -15,6 +15,7 @@#include <linux/seq_file.h> #include <linux/string_choices.h> #include <linux/vmalloc.h> +#include <linux/workqueue.h> #include <linux/kmemleak.h> #include <uapi/linux/alloc_tag.h> @@ -484,22 +485,28 @@ static const struct proc_ops allocinfo_proc_ops = {#endif }; -size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count,bool can_sleep) +size_t alloc_tag_top_users (struct codetag_bytes *tags,size_t count) {struct codetag_iterator iter; +struct codetag_type *cttype;struct codetag *ct;struct codetag_bytes n;unsigned int i, nr =0 ; +bool locked;
Quoted 7 lines
mm/alloc_tag.c static const struct proc_ops allocinfo_proc_ops = { · line 498
Quoted 3 lines
mm/alloc_tag.c +2
-1-if (IS_ERR_OR_NULL (alloc_tag_cttype)) +rcu_read_lock (); + cttype =READ_ONCE (alloc_tag_cttype);Code looks correct to me but if you are changing
alloc_tag_cttypeto
be accessed under RCU, then you should declare it as__rcuand use rcu_assign_pointer()/rcu_access_pointer()/rcu_dereference()/rcu_dereference_protected().
Yes. I missed that. Will fix in the next version.
Thanks
Best Regards
Hao
Quoted 109 lines
Quoted 109 lines
mm/alloc_tag.c +30
-14+if (IS_ERR_OR_NULL (cttype)) { +rcu_read_unlock ();return 0 ; + } -if (can_sleep) -codetag_lock_module_list (alloc_tag_cttype); -else if (!codetag_trylock_module_list (alloc_tag_cttype)) + locked =codetag_trylock_module_list (cttype); +rcu_read_unlock (); +if (!locked)return 0 ; - iter =codetag_get_ct_iter (alloc_tag_cttype); + iter =codetag_get_ct_iter (cttype);while ((ct =codetag_next_ct (&iter))) {struct alloc_tag_counters counter =alloc_tag_read (ct_to_alloc_tag (ct));@@ -520,7 +527,7 @@ size_t alloc_tag_top_users(struct codetag_bytes *tags, size_t count, bool can_sl } } -codetag_unlock_module_list (alloc_tag_cttype); +codetag_unlock_module_list (cttype);return nr; }@@ -591,6 +598,13 @@ void pgalloc_tag_swap(struct folio *new, struct folio *old)put_page_tag_ref (handle_new); } +static void remove_allocinfo_file (struct work_struct *work) +{ +remove_proc_entry (ALLOCINFO_FILE_NAME ,NULL ); +} + +static DECLARE_WORK(remove_allocinfo_work, remove_allocinfo_file); +static void shutdown_mem_profiling (bool remove_file) {if (mem_alloc_profiling_enabled ())@@ -600,7 +614,7 @@ static void shutdown_mem_profiling(bool remove_file)return ;if (remove_file) -remove_proc_entry (ALLOCINFO_FILE_NAME ,NULL ); +schedule_work (&remove_allocinfo_work); mem_profiling_support = false; }@@ -1351,16 +1365,10 @@ static int __init alloc_tag_init(void)return 0 ; } -if (!proc_create (ALLOCINFO_FILE_NAME ,0400 ,NULL , &allocinfo_proc_ops)) { -pr_err ("Failed to create %s file\n" ,ALLOCINFO_FILE_NAME ); -shutdown_mem_profiling (false); -return -ENOMEM ; - } - res =alloc_mod_tags_mem ();if (res) {pr_err ("Failed to reserve address space for module tags, errno = %d\n" , res); -shutdown_mem_profiling (true); +shutdown_mem_profiling (false);return res; }@@ -1368,10 +1376,22 @@ static int __init alloc_tag_init(void)if (IS_ERR (alloc_tag_cttype)) {pr_err ("Allocation tags registration failed, errno = %pe\n" , alloc_tag_cttype);free_mod_tags_mem (); -shutdown_mem_profiling (true); +shutdown_mem_profiling (false);return PTR_ERR (alloc_tag_cttype); } +if (!proc_create (ALLOCINFO_FILE_NAME ,0400 ,NULL , &allocinfo_proc_ops)) { +struct codetag_type *cttype = alloc_tag_cttype; + +pr_err ("Failed to create %s file\n" ,ALLOCINFO_FILE_NAME ); +shutdown_mem_profiling (false); +WRITE_ONCE (alloc_tag_cttype,NULL ); +synchronize_rcu (); +codetag_unregister_type (cttype); +free_mod_tags_mem (); +return -ENOMEM ; + } +return 0 ; }module_init (alloc_tag_init);mm/show_mem.c +1
-2@@ -439,7 +439,7 @@ void __show_mem(unsigned int filter, const nodemask_t *nodemask,struct codetag_bytes tags[10 ];size_t i, nr; - nr =alloc_tag_top_users (tags,ARRAY_SIZE (tags), false); + nr =alloc_tag_top_users (tags,ARRAY_SIZE (tags));if (nr) {pr_notice ("Memory allocations (profiling is currently turned %s):\n" ,mem_alloc_profiling_enabled () ?"on" :"off" ); --2.25.1
Andrew Morton
maintainerQuoted 18 lines · On Tue, 29 Sep 2026 16:20:07 +0800 Hao Ge <[email protected]> wrote:
With profiling toggled off, the overflow check in
reserve_module_tags()did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted/proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns-EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.Review of the earlier series by Sashiko turned up more problems.
One is a race.
layout_sections()andmove_module()both askedcodetag_needs_module_section()where a codetag section goes, andmem_profiling_supportcan change between the two calls, for instance when another module load overflows the tag index and shuts profiling down.move_module()then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.
Quoted 5 lines
v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before
layout_sections()and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on-EAGAINthe section is laid out as regular module data right in the same load.
Thanks.
Seven patches, all cc:stable. Why is a backport proposed? Documentation/process/stable-kernel-rules.rst gives guidelines - does this series meet them? Does Suren have thoughts?
Also, Sashiko has been busy again:
https://sashiko.dev/#/patchset/[email protected]
Suren Baghdasaryan
maintainerQuoted 27 lines · Andrew Morton
Quoted 18 lines · On Tue, 29 Sep 2026 16:20:07 +0800 Hao Ge <[email protected]> wrote:
With profiling toggled off, the overflow check in
reserve_module_tags()did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted/proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns-EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.Review of the earlier series by Sashiko turned up more problems.
One is a race.
layout_sections()andmove_module()both askedcodetag_needs_module_section()where a codetag section goes, andmem_profiling_supportcan change between the two calls, for instance when another module load overflows the tag index and shuts profiling down.move_module()then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.Quoted 5 lines
v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before
layout_sections()and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on-EAGAINthe section is laid out as regular module data right in the same load.
Quoted 5 lines
Thanks.
Seven patches, all cc:stable. Why is a backport proposed?
Documentation/process/stable-kernel-rules.rstgives guidelines - does this series meet them? Does Suren have thoughts?
I think vmalloc fix in patch#2 should be posted separately and clearly quialifies for backporting as if fixes issues Hao found at [1].
The rest are also bug fixes but their size is concerning. I'll review them first before advising on which ones should be backported. Some fixes address issues that happen very rarely and if a fix is too complex it might make sense to leave it be or fix it some other way with less churn. Please give me a couple of days to review them. With the upcoming LPC I'm trying to multiplex between several projects.
[1] https://lore.kernel.org/all/[email protected]/
Quoted 3 lines
Also, Sashiko has been busy again:
https://sashiko.dev/#/patchset/[email protected]
Hao Ge
reviewerHi Suren and Andrew
Quoted 28 lines · Suren Baghdasaryan
Quoted 27 lines · Andrew Morton
Quoted 23 lines · On Tue, 29 Sep 2026 16:20:07 +0800 Hao Ge <[email protected]> wrote:
With profiling toggled off, the overflow check in
reserve_module_tags()did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted/proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns-EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.Review of the earlier series by Sashiko turned up more problems.
One is a race.
layout_sections()andmove_module()both askedcodetag_needs_module_section()where a codetag section goes, andmem_profiling_supportcan change between the two calls, for instance when another module load overflows the tag index and shuts profiling down.move_module()then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before
layout_sections()and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on-EAGAINthe section is laid out as regular module data right in the same load.
Quoted 5 lines
Quoted 5 lines
Thanks.
Seven patches, all cc:stable. Why is a backport proposed?
Documentation/process/stable-kernel-rules.rstgives guidelines - does this series meet them? Does Suren have thoughts?
Thanks, I've just gone through this carefully.
Quoted 3 lines
I think vmalloc fix in patch#2 should be posted separately and clearly quialifies for backporting as if fixes issues Hao found at [1].
Agreed. Patch #2 was added to this patch series because vm_module_tags_populate suffers from the same issue. If we want to be more conservative, I'd prefer to keep vm_module_tags_populate as it was in V10:
https://lore.kernel.org/all/[email protected]/
Then we can submit a fix for __GFP_NOFAIL retry loop in __vmalloc_area_node() as a hotfix together with our new patch #2. While at it, we can also check if there are any unnecessary calls to vunmap_range family functions that can be cleaned up now that patch #2 is applied.
Quoted 7 lines
The rest are also bug fixes but their size is concerning. I'll review them first before advising on which ones should be backported. Some fixes address issues that happen very rarely and if a fix is too complex it might make sense to leave it be or fix it some other way with less churn. Please give me a couple of days to review them. With the upcoming LPC I'm trying to multiplex between several projects.
Thanks, sorry for all the hassle on you folks.
Thanks
Best Regards
Hao
Quoted 6 lines
[1] https://lore.kernel.org/all/[email protected]/
Quoted 3 lines
Also, Sashiko has been busy again:
https://sashiko.dev/#/patchset/[email protected]
Uladzislau Rezki
maintainerQuoted 44 lines · Hao Ge
Hi Suren and Andrew
Quoted 28 lines · Suren Baghdasaryan
Quoted 27 lines · Andrew Morton
Quoted 23 lines · On Tue, 29 Sep 2026 16:20:07 +0800 Hao Ge <[email protected]> wrote:
With profiling toggled off, the overflow check in
reserve_module_tags()did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted/proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns-EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.Review of the earlier series by Sashiko turned up more problems.
One is a race.
layout_sections()andmove_module()both askedcodetag_needs_module_section()where a codetag section goes, andmem_profiling_supportcan change between the two calls, for instance when another module load overflows the tag index and shuts profiling down.move_module()then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before
layout_sections()and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on-EAGAINthe section is laid out as regular module data right in the same load.Quoted 5 lines
Quoted 5 lines
Thanks.
Seven patches, all cc:stable. Why is a backport proposed?
Documentation/process/stable-kernel-rules.rstgives guidelines - does this series meet them? Does Suren have thoughts?Thanks, I've just gone through this carefully.
Quoted 3 lines
I think vmalloc fix in patch#2 should be posted separately and clearly quialifies for backporting as if fixes issues Hao found at [1].
Quoted 9 lines
Agreed. Patch #2 was added to this patch series because
vm_module_tags_populatesuffers from the same issue. If we want to be more conservative, I'd prefer to keepvm_module_tags_populateas it was in V10:
https://lore.kernel.org/all/[email protected]/Then we can submit a fix for
__GFP_NOFAILretry loop in__vmalloc_area_node()as a hotfix together with our new patch #2. While at it, we can also check if there are any unnecessary calls tovunmap_rangefamily functions that can be cleaned up now that patch #2 is applied.
Yep, this should be a separate fix for vmalloc.
--
Uladzislau Rezki
Suren Baghdasaryan
maintainerQuoted 56 lines · Hao Ge
Hi Suren and Andrew
Quoted 28 lines · Suren Baghdasaryan
Quoted 27 lines · Andrew Morton
Quoted 23 lines · On Tue, 29 Sep 2026 16:20:07 +0800 Hao Ge <[email protected]> wrote:
With profiling toggled off, the overflow check in
reserve_module_tags()did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted/proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns-EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.Review of the earlier series by Sashiko turned up more problems.
One is a race.
layout_sections()andmove_module()both askedcodetag_needs_module_section()where a codetag section goes, andmem_profiling_supportcan change between the two calls, for instance when another module load overflows the tag index and shuts profiling down.move_module()then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before
layout_sections()and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on-EAGAINthe section is laid out as regular module data right in the same load.Quoted 5 lines
Quoted 5 lines
Thanks.
Seven patches, all cc:stable. Why is a backport proposed?
Documentation/process/stable-kernel-rules.rstgives guidelines - does this series meet them? Does Suren have thoughts?Thanks, I've just gone through this carefully.
Quoted 3 lines
I think vmalloc fix in patch#2 should be posted separately and clearly quialifies for backporting as if fixes issues Hao found at [1].
Agreed. Patch #2 was added to this patch series because
vm_module_tags_populatesuffers from the same issue. If we want to be more conservative, I'd prefer to keepvm_module_tags_populateas it was in V10:
https://lore.kernel.org/all/[email protected]/Then we can submit a fix for
__GFP_NOFAILretry loop in__vmalloc_area_node()as a hotfix together with our new patch #2. While at it, we can also check if there are any unnecessary calls tovunmap_rangefamily functions that can be cleaned up now that patch #2 is applied.Quoted 1 line
Quoted 8 lines
Quoted 6 lines
The rest are also bug fixes but their size is concerning. I'll review them first before advising on which ones should be backported. Some fixes address issues that happen very rarely and if a fix is too complex it might make sense to leave it be or fix it some other way with less churn. Please give me a couple of days to review them. With the upcoming LPC I'm trying to multiplex between several projects.
Thanks, sorry for all the hassle on you folks.
You have no reasons to be sorry. It's me who is delaying the reviews.
Quoted 11 lines
Thanks
Best Regards
HaoQuoted 6 lines
[1] https://lore.kernel.org/all/[email protected]/
Quoted 3 lines
Also, Sashiko has been busy again:
https://sashiko.dev/#/patchset/[email protected]
Suren Baghdasaryan
maintainerQuoted 42 lines · Hao Ge
Hi Suren and Andrew
Quoted 28 lines · Suren Baghdasaryan
Quoted 27 lines · Andrew Morton
Quoted 23 lines · On Tue, 29 Sep 2026 16:20:07 +0800 Hao Ge <[email protected]> wrote:
With profiling toggled off, the overflow check in
reserve_module_tags()did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted/proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns-EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.Review of the earlier series by Sashiko turned up more problems.
One is a race.
layout_sections()andmove_module()both askedcodetag_needs_module_section()where a codetag section goes, andmem_profiling_supportcan change between the two calls, for instance when another module load overflows the tag index and shuts profiling down.move_module()then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before
layout_sections()and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on-EAGAINthe section is laid out as regular module data right in the same load.Quoted 5 lines
Quoted 5 lines
Thanks.
Seven patches, all cc:stable. Why is a backport proposed?
Documentation/process/stable-kernel-rules.rstgives guidelines - does this series meet them? Does Suren have thoughts?Thanks, I've just gone through this carefully.
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
I think vmalloc fix in patch#2 should be posted separately and clearly quialifies for backporting as if fixes issues Hao found at [1].
Agreed. Patch #2 was added to this patch series because
vm_module_tags_populatesuffers from the same issue. If we want to be more conservative, I'd prefer to keepvm_module_tags_populateas it was in V10:
https://lore.kernel.org/all/[email protected]/
I disagree with this approach. That means we are introducing an extra code inside vm_module_tags_populate() only to clean it up later. Instead please post Patch #2 separately. That posting should have your current patch that changes vmap_pages_xxx() to clean up on failure, followed by patch(es) which remove unnecessary cleanup that some functions currently perform by themselves. This patchset should definitely be tagged for stable.
Once that patchset is accepted, the rest of your current patchset fixing alloc_tags can be posted with correct assumption that vmap_pages_range() cleans up its partial mapping when it fails.
I know this sequence will take longer to upstream but I think it's the right way to go.
Quoted 26 lines
Then we can submit a fix for
__GFP_NOFAILretry loop in__vmalloc_area_node()as a hotfix together with our new patch #2. While at it, we can also check if there are any unnecessary calls tovunmap_rangefamily functions that can be cleaned up now that patch #2 is applied.Quoted 7 lines
The rest are also bug fixes but their size is concerning. I'll review them first before advising on which ones should be backported. Some fixes address issues that happen very rarely and if a fix is too complex it might make sense to leave it be or fix it some other way with less churn. Please give me a couple of days to review them. With the upcoming LPC I'm trying to multiplex between several projects.
Thanks, sorry for all the hassle on you folks.
Thanks
Best Regards
HaoQuoted 6 lines
[1] https://lore.kernel.org/all/[email protected]/
Quoted 3 lines
Also, Sashiko has been busy again:
https://sashiko.dev/#/patchset/[email protected]
Hao Ge
reviewerHi Suren
Quoted 51 lines · Suren Baghdasaryan
Quoted 42 lines · Hao Ge
Hi Suren and Andrew
Quoted 28 lines · Suren Baghdasaryan
Quoted 27 lines · Andrew Morton
Quoted 23 lines · On Tue, 29 Sep 2026 16:20:07 +0800 Hao Ge <[email protected]> wrote:
With profiling toggled off, the overflow check in
reserve_module_tags()did not run, a module could load with more tags than the page flags can address, and re-enabling profiling then silently corrupted/proc/allocinfo. On overflow the fix shuts profiling down, releases the reservation and returns-EAGAIN, and the codetag section lands as regular module data in the same load, so the module loads without profiling.Review of the earlier series by Sashiko turned up more problems.
One is a race.
layout_sections()andmove_module()both askedcodetag_needs_module_section()where a codetag section goes, andmem_profiling_supportcan change between the two calls, for instance when another module load overflows the tag index and shuts profiling down.move_module()then copied the codetag section to offset 0 of its regular destination and clobbered the first section placed in that region.v7 reworks where codetag sections are allocated, on a prototype by Petr Pavlu [1]. The allocation runs before
layout_sections()and the placement is decided in one step, so nothing re-asks the question and the race is gone. The retry is gone too, on-EAGAINthe section is laid out as regular module data right in the same load.Quoted 5 lines
Quoted 5 lines
Thanks.
Seven patches, all cc:stable. Why is a backport proposed?
Documentation/process/stable-kernel-rules.rstgives guidelines - does this series meet them? Does Suren have thoughts?Thanks, I've just gone through this carefully.
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
I think vmalloc fix in patch#2 should be posted separately and clearly quialifies for backporting as if fixes issues Hao found at [1].
Agreed. Patch #2 was added to this patch series because
vm_module_tags_populatesuffers from the same issue. If we want to be more conservative, I'd prefer to keepvm_module_tags_populateas it was in V10:
https://lore.kernel.org/all/[email protected]/
Quoted 7 lines
I disagree with this approach. That means we are introducing an extra code inside
vm_module_tags_populate()only to clean it up later. Instead please post Patch #2 separately. That posting should have your current patch that changesvmap_pages_xxx()to clean up on failure, followed by patch(es) which remove unnecessary cleanup that some functions currently perform by themselves. This patchset should definitely be tagged for stable.
You are right. Sorry for the confusion - my earlier deviation came from not fully understanding the rules for getting patches into the stable trees.
Quoted 6 lines
Once that patchset is accepted, the rest of your current patchset fixing
alloc_tagscan be posted with correct assumption thatvmap_pages_range()cleans up its partial mapping when it fails.I know this sequence will take longer to upstream but I think it's the right way to go.
Make sense. Thanks for your continued support.
Best Regards
Hao
Quoted 27 lines
Quoted 27 lines
Then we can submit a fix for
__GFP_NOFAILretry loop in__vmalloc_area_node()as a hotfix together with our new patch #2. While at it, we can also check if there are any unnecessary calls tovunmap_rangefamily functions that can be cleaned up now that patch #2 is applied.Quoted 7 lines
The rest are also bug fixes but their size is concerning. I'll review them first before advising on which ones should be backported. Some fixes address issues that happen very rarely and if a fix is too complex it might make sense to leave it be or fix it some other way with less churn. Please give me a couple of days to review them. With the upcoming LPC I'm trying to multiplex between several projects.
Thanks, sorry for all the hassle on you folks.
Thanks
Best Regards
HaoQuoted 6 lines
[1] https://lore.kernel.org/all/[email protected]/
Quoted 3 lines
Also, Sashiko has been busy again:
https://sashiko.dev/#/patchset/[email protected]