On 19/10/26 11:15 am, Hao Ge wrote:
> __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. pcpu_map_pages() and kmsan_ioremap_page_range()
> roll back what they mapped before the failure, while
> vm_module_tags_populate() and the __GFP_NOFAIL retry loop in
> __vmalloc_area_node() rely on the mapping functions cleaning up and
> do not call anything like vunmap_range() themselves. When the same
> range is mapped again, the attempt hits the leftovers and fails, with
> a BUG() in vmap_pte_range() for huge mappings and a warning on the
> small-page path.
>
> 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(), it only clears the PTEs,
> no TLB flush, nothing has touched these mappings.
> vmap_pages_range_noflush() drops the KMSAN metadata when the data
> mapping fails, and an empty range bails out with -EINVAL now, which
> used to BUG_ON() on the small-page path and map nothing at all on
> the huge-page one.
>
> Reported-by: Sashiko <[email protected]>
> Suggested-by: Uladyslau Rezki <[email protected]>
> Cc: [email protected]
> Signed-off-by: Hao Ge <[email protected]>
> ---
> mm/vmalloc.c | 44 ++++++++++++++++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 44e0eb32708f..75aa5acf5566 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -363,6 +363,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;
> }
>
> @@ -683,27 +686,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 = 0;
> +
> + if (WARN_ON_ONCE(addr >= end))
> + return -EINVAL;
IIUC this is a general change independent of this patch? Caller could pass
addr >= end and if we take the huge mapping path, we lose the sanity check.
So this could become a separate patch.
>
> if (WARN_ON_ONCE(page_shift < PAGE_SHIFT))
> return -EINVAL;
>
> if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMALLOC) ||
> - page_shift == PAGE_SHIFT)
> - return vmap_small_pages_range_noflush(addr, end, prot, pages);
> -
> - for (i = 0; i < nr; i += 1U << (page_shift - PAGE_SHIFT)) {
> - int err;
> -
> - err = vmap_range_noflush(addr, addr + (1UL << page_shift),
> + 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;
> + /* Undo the PTEs installed before the failure. */
> + if (err)
> + __vunmap_range_noflush(start, end);
> +
> + return err;
> }
>
> int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> @@ -715,7 +726,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,
