Re: [PATCH v4 7/9] mm/vmalloc: undo partial mappings inside the mapping functions

[Hao Ge <hao.ge@linux.dev>]

Hi Suren

Thanks for your review.

On 2026/11/12 09:15, Suren Baghdasaryan wrote:
> On Tue, Nov 10, 2026 at 11:25 AM Hao Ge <hao.ge@linux.dev> wrote:
>>
>> The vmap entry points may install some PTEs and then fail, leaving a
>> partial mapping behind.
>>
>> The callers do not agree on who cleans it up. pcpu_map_pages() and
>> kmsan_ioremap_page_range() roll back what they mapped, while
>> vm_module_tags_populate() and the __GFP_NOFAIL retry loop in
>> __vmalloc_area_node() expect a clean range from the mapping function.
>> For them the leftovers stay mapped, and the next attempt to map the
>> same range trips the WARN_ON(!pte_none()) in vmap_pages_pte_range().
>> A leftover huge pmd is even worse. During the retry, vmap_try_huge_pmd()
>> calls pmd_free_pte_page() on it, and the x86 implementation cannot
>> tell a leaf entry from a page table and frees the mapped page.
>>
>> After discussing with Suren and Ulad, we decided to put the rollback
>> into the entry points of the vmap API rather than into every
>> low-level helper. __vmap_pages_range_noflush() undoes the whole range
>> it was asked to map when it fails, and vmap_page_range() undoes its
>> range for the ioremap-style mappings.
>>
>> The rollback is __vunmap_range_noflush(), which only clears the PTEs.
>> No TLB flush is needed, nothing has touched the mapping yet.
>> vmap_pages_range_noflush() also drops the KMSAN metadata of the
>> failed mapping.
>>
>> The undo walk skips its cond_resched() when the caller's gfp mask
>> cannot block. __vmalloc() with GFP_ATOMIC or GFP_NOWAIT can run in
>> atomic context, where cond_resched() cannot schedule.
>> __vunmap_range_noflush() takes a can_resched argument for that, which
>> the sleepable callers pass as true.
>>
>> Suggested-by: Uladyslau Rezki <urezki@gmail.com>
>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>> ---
>>  include/linux/kmsan.h |  8 ++++--
>>  mm/kmsan/hooks.c      | 18 ++++++++-----
>>  mm/kmsan/shadow.c     |  7 ++---
>>  mm/vmalloc.c          | 61 +++++++++++++++++++++++++++++++++----------
>>  mm/vmalloc.h          |  6 +++--
>>  5 files changed, 72 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/linux/kmsan.h b/include/linux/kmsan.h
>> index 7da9fd506b39..0016c032d1e7 100644
>> --- a/include/linux/kmsan.h
>> +++ b/include/linux/kmsan.h
>> @@ -150,11 +150,14 @@ int __must_check kmsan_vmap_pages_range_noflush(unsigned long start,
>>   * kmsan_vunmap_kernel_range_noflush() - Notify KMSAN about a vunmap.
>>   * @start: start of vunmapped range.
>>   * @end:   end of vunmapped range.
>> + * @can_resched: whether the unmap walk may call cond_resched(); pass
>> + * false from atomic context.
>>   *
>>   * KMSAN unmaps the contiguous metadata ranges created by
>>   * kmsan_map_kernel_range_noflush().
>>   */
>> -void kmsan_vunmap_range_noflush(unsigned long start, unsigned long end);
>> +void kmsan_vunmap_range_noflush(unsigned long start, unsigned long end,
>> +                               bool can_resched);
>>
>>  /**
>>   * kmsan_ioremap_page_range() - Notify KMSAN about a ioremap_page_range() call.
>> @@ -355,7 +358,8 @@ static inline int __must_check kmsan_vmap_pages_range_noflush(
>>  }
>>
>>  static inline void kmsan_vunmap_range_noflush(unsigned long start,
>> -                                             unsigned long end)
>> +                                             unsigned long end,
>> +                                             bool can_resched)
>>  {
>>  }
>>
>> diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
>> index 107f66841274..0cd1edaa1120 100644
>> --- a/mm/kmsan/hooks.c
>> +++ b/mm/kmsan/hooks.c
>> @@ -136,13 +136,16 @@ static unsigned long vmalloc_origin(unsigned long addr)
>>                                                  KMSAN_META_ORIGIN);
>>  }
>>
>> -void kmsan_vunmap_range_noflush(unsigned long start, unsigned long end)
>> +void kmsan_vunmap_range_noflush(unsigned long start, unsigned long end,
>> +                               bool can_resched)
>>  {
>>         if (!kmsan_enabled)
>>                 return;
>>
>> -       __vunmap_range_noflush(vmalloc_shadow(start), vmalloc_shadow(end));
>> -       __vunmap_range_noflush(vmalloc_origin(start), vmalloc_origin(end));
>> +       __vunmap_range_noflush(vmalloc_shadow(start), vmalloc_shadow(end),
>> +                              can_resched);
>> +       __vunmap_range_noflush(vmalloc_origin(start), vmalloc_origin(end),
>> +                              can_resched);
>>         flush_cache_vmap(vmalloc_shadow(start), vmalloc_shadow(end));
>>         flush_cache_vmap(vmalloc_origin(start), vmalloc_origin(end));
>>  }
>> @@ -171,8 +174,9 @@ static void kmsan_iounmap_pages(unsigned long start, unsigned long end)
>>                 if (origin)
>>                         __free_pages(origin, KMSAN_IOREMAP_META_ORDER);
>>         }
>> -       __vunmap_range_noflush(shadow_start, shadow_end);
>> -       __vunmap_range_noflush(origin_start, origin_end);
>> +       /* ioremap metadata mappings are always GFP_KERNEL, so resched is safe. */
>> +       __vunmap_range_noflush(shadow_start, shadow_end, true);
>> +       __vunmap_range_noflush(origin_start, origin_end, true);
>>         flush_tlb_kernel_range(shadow_start, shadow_end);
>>         flush_tlb_kernel_range(origin_start, origin_end);
>>  }
>> @@ -207,7 +211,7 @@ int kmsan_ioremap_page_range(unsigned long start, unsigned long end,
>>                 mapped = __vmap_pages_range_noflush(
>>                         vmalloc_shadow(start + off),
>>                         vmalloc_shadow(start + off + PAGE_SIZE), prot, &shadow,
>> -                       PAGE_SHIFT);
>> +                       PAGE_SHIFT, gfp_mask);
>>                 if (mapped) {
>>                         err = mapped;
>>                         goto ret;
>> @@ -217,7 +221,7 @@ int kmsan_ioremap_page_range(unsigned long start, unsigned long end,
>>                 mapped = __vmap_pages_range_noflush(
>>                         vmalloc_origin(start + off),
>>                         vmalloc_origin(start + off + PAGE_SIZE), prot, &origin,
>> -                       PAGE_SHIFT);
>> +                       PAGE_SHIFT, gfp_mask);
>>                 if (mapped) {
>>                         err = mapped;
>>                         goto ret;
>> diff --git a/mm/kmsan/shadow.c b/mm/kmsan/shadow.c
>> index 2166086d3dc3..452760a333e4 100644
>> --- a/mm/kmsan/shadow.c
>> +++ b/mm/kmsan/shadow.c
>> @@ -247,7 +247,7 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
>>         origin_end = vmalloc_meta((void *)end, KMSAN_META_ORIGIN);
>>         kmsan_enter_runtime();
>>         mapped = __vmap_pages_range_noflush(shadow_start, shadow_end, prot,
>> -                                           s_pages, page_shift);
>> +                                           s_pages, page_shift, gfp_mask);
>>         kmsan_leave_runtime();
>>         if (mapped) {
>>                 err = mapped;
>> @@ -255,12 +255,13 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
>>         }
>>         kmsan_enter_runtime();
>>         mapped = __vmap_pages_range_noflush(origin_start, origin_end, prot,
>> -                                           o_pages, page_shift);
>> +                                           o_pages, page_shift, gfp_mask);
>>         kmsan_leave_runtime();
>>         if (mapped) {
>>                 /* Undo the shadow mapping set up above. */
>>                 kmsan_enter_runtime();
>> -               __vunmap_range_noflush(shadow_start, shadow_end);
>> +               __vunmap_range_noflush(shadow_start, shadow_end,
>> +                                      gfpflags_allow_blocking(gfp_mask));
>>                 kmsan_leave_runtime();
>>                 err = mapped;
>>                 goto ret;
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index 3f98056f8c7e..d1986f68eab4 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -372,8 +372,12 @@ 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)
>> +       if (err) {
>> +               /* Callers are ioremap and arch setup code, always sleepable. */
>> +               __vunmap_range_noflush(addr, end, true);
>> +       } else {
>>                 flush_cache_vmap(addr, end);
>> +       }
>>
>>         return err;
>>  }
>> @@ -429,7 +433,7 @@ static void vunmap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
>>  }
>>
>>  static void vunmap_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
>> -                            pgtbl_mod_mask *mask)
>> +                            pgtbl_mod_mask *mask, bool can_resched)
>>  {
>>         pmd_t *pmd;
>>         unsigned long next;
>> @@ -451,12 +455,14 @@ static void vunmap_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
>>                         continue;
>>                 vunmap_pte_range(pmd, addr, next, mask);
>>
>> -               cond_resched();
>> +               /* The undo path may run in atomic context. */
>> +               if (can_resched)
>> +                       cond_resched();
>>         } while (pmd++, addr = next, addr != end);
>>  }
>>
>>  static void vunmap_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
>> -                            pgtbl_mod_mask *mask)
>> +                            pgtbl_mod_mask *mask, bool can_resched)
>>  {
>>         pud_t *pud;
>>         unsigned long next;
>> @@ -476,12 +482,12 @@ static void vunmap_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end,
>>                 }
>>                 if (pud_none_or_clear_bad(pud))
>>                         continue;
>> -               vunmap_pmd_range(pud, addr, next, mask);
>> +               vunmap_pmd_range(pud, addr, next, mask, can_resched);
>>         } while (pud++, addr = next, addr != end);
>>  }
>>
>>  static void vunmap_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long end,
>> -                            pgtbl_mod_mask *mask)
>> +                            pgtbl_mod_mask *mask, bool can_resched)
>>  {
>>         p4d_t *p4d;
>>         unsigned long next;
>> @@ -496,7 +502,7 @@ static void vunmap_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long end,
>>
>>                 if (p4d_none_or_clear_bad(p4d))
>>                         continue;
>> -               vunmap_pud_range(p4d, addr, next, mask);
>> +               vunmap_pud_range(p4d, addr, next, mask, can_resched);
>>         } while (p4d++, addr = next, addr != end);
>>  }
>>
>> @@ -512,7 +518,8 @@ static void vunmap_p4d_range(pgd_t *pgd, unsigned long addr, unsigned long end,
>>   *
>>   * This is an internal function only. Do not use outside mm/.
>>   */
>> -void __vunmap_range_noflush(unsigned long start, unsigned long end)
>> +void __vunmap_range_noflush(unsigned long start, unsigned long end,
>> +                           bool can_resched)
>>  {
>>         unsigned long next;
>>         pgd_t *pgd;
>> @@ -529,7 +536,7 @@ void __vunmap_range_noflush(unsigned long start, unsigned long end)
>>                         mask |= PGTBL_PGD_MODIFIED;
>>                 if (pgd_none_or_clear_bad(pgd))
>>                         continue;
>> -               vunmap_p4d_range(pgd, addr, next, &mask);
>> +               vunmap_p4d_range(pgd, addr, next, &mask, can_resched);
>>         } while (pgd++, addr = next, addr != end);
>>
>>         if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
>> @@ -538,8 +545,9 @@ void __vunmap_range_noflush(unsigned long start, unsigned long end)
>>
>>  void vunmap_range_noflush(unsigned long start, unsigned long end)
>>  {
>> -       kmsan_vunmap_range_noflush(start, end);
>> -       __vunmap_range_noflush(start, end);
>> +       /* The unmap paths are sleepable, keep the resched point. */
>> +       kmsan_vunmap_range_noflush(start, end, true);
>> +       __vunmap_range_noflush(start, end, true);
>>  }
>>
>>  /**
>> @@ -727,18 +735,32 @@ static int vmap_pages_range_noflush_walk(unsigned long addr, unsigned long end,
>>   * The caller is responsible for calling flush_cache_vmap() after this
>>   * function returns successfully and before the addresses are accessed.
>>   *
>> + * On failure the mapping is torn down before returning, so a retry
>> + * sees a clean range. When @gfp_mask cannot block, the teardown
>> + * skips cond_resched(), it may run in atomic context.
>> + *
>>   * This is an internal function only. Do not use outside mm/.
>>   */
>>  int __vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>> -               pgprot_t prot, struct page **pages, unsigned int page_shift)
>> +               pgprot_t prot, struct page **pages, unsigned int page_shift,
>> +               gfp_t gfp_mask)
>>  {
>> +       int err;
>> +
>>         if (WARN_ON_ONCE(page_shift < PAGE_SHIFT))
>>                 return -EINVAL;
>>
>>         if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMALLOC))
>>                 page_shift = PAGE_SHIFT;
>>
>> -       return vmap_pages_range_noflush_walk(addr, end, prot, pages, page_shift);
>> +       err = vmap_pages_range_noflush_walk(addr, end, prot, pages, page_shift);
>> +
>> +       /* Undo the PTEs installed before the failure. */
>> +       if (err)
>> +               __vunmap_range_noflush(addr, end,
>> +                                      gfpflags_allow_blocking(gfp_mask));
> 
> I remember we discussed this before that __vunmap_range_noflush() will
> be unmapping the entire range between addr and end instead of the
> range vmap_pages_range_noflush_walk() actially mapped and your reply
> was that this keeps things simple and avoids adding extra code in the
> common path when we do not fail. This was bothering me, so I looked a
> bit deeper and I think there is an issue with this approach.
> vmap_pages_pte_range() fails with -EBUSY if it encounters an already
> installed PTE. So, if this happens, we immediately bail out of mapping
> the range and will be unmapping the entire range, including these
> existing PTEs. I don't think that's ok.
>

You were right, I did not really think the -EBUSY case through.
when the walk trips over a PTE that is already there, everything
from that point on is somebody else's mapping, and the undo as
it stood would clear it. sorry for this.


> A fix might be quite simple. vmap_pages_range_noflush_walk() already
> counts how many pages we processed in nr variable. If we propagate
> that one level up then we could unmap only the range we actually
> mapped.

Right, thank you very much for helping sort this out. This makes
it a lot clearer.

Thanks
Best Regards
Hao

> 
>> +
>> +       return err;
>>  }
>>
>>  int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>> @@ -750,7 +772,18 @@ 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,
>> +                                        gfp_mask);
>> +       /*
>> +        * 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,
>> +                                          gfpflags_allow_blocking(gfp_mask));
>> +
>> +       return ret;
>>  }
>>
>>  static int __vmap_pages_range(unsigned long addr, unsigned long end,
>> diff --git a/mm/vmalloc.h b/mm/vmalloc.h
>> index d8372a4f65c4..9ea28845f713 100644
>> --- a/mm/vmalloc.h
>> +++ b/mm/vmalloc.h
>> @@ -40,10 +40,12 @@ void clear_vm_uninitialized_flag(struct vm_struct *vm);
>>
>>  int __must_check __vmap_pages_range_noflush(unsigned long addr,
>>                 unsigned long end, pgprot_t prot,
>> -               struct page **pages, unsigned int page_shift);
>> +               struct page **pages, unsigned int page_shift,
>> +               gfp_t gfp_mask);
>>
>>  void vunmap_range_noflush(unsigned long start, unsigned long end);
>>
>> -void __vunmap_range_noflush(unsigned long start, unsigned long end);
>> +void __vunmap_range_noflush(unsigned long start, unsigned long end,
>> +                           bool can_resched);
>>
>>  #endif /* __MM_VMALLOC_H */
>> --
>> 2.25.1
>>