linux-kernel - Re: [PATCH] mm: avoid zeroing user movable page twice with init_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdV1hkwajxDWk6AWj_QR_qPkEni0u=tnQWdt1-M83NE0ig@mail.gmail.com>
Date: Thu, 5 Dec 2024 09:19:26 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Zi Yan <ziy@...dia.com>
Cc: Matthew Wilcox <willy@...radead.org>, Vlastimil Babka <vbabka@...e.cz>, linux-mm@...ck.org, 
	Andrew Morton <akpm@...ux-foundation.org>, David Hildenbrand <david@...hat.com>, 
	Miaohe Lin <linmiaohe@...wei.com>, Kefeng Wang <wangkefeng.wang@...wei.com>, 
	John Hubbard <jhubbard@...dia.com>, "Huang, Ying" <ying.huang@...el.com>, 
	Ryan Roberts <ryan.roberts@....com>, Alexander Potapenko <glider@...gle.com>, 
	Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org, 
	linux-mips@...r.kernel.org
Subject: Re: [PATCH] mm: avoid zeroing user movable page twice with init_on_alloc=1

Hi Zi,

On Wed, Dec 4, 2024 at 5:58 PM Zi Yan <ziy@...dia.com> wrote:
> On 4 Dec 2024, at 11:29, Matthew Wilcox wrote:
> > On Wed, Dec 04, 2024 at 11:16:51AM -0500, Zi Yan wrote:
> >>> So maybe the clearing done as part of page allocator isn't enough here.
> >>>
> >> Basically, mips needs to flush data cache if kmap address is aliased to
> >
> > People use "aliased" in contronym ways.  Do you mean "has a
> > non-congruent alias" or "has a congruent alias"?
>
> I mean if kmap address goes into a different cache line than userspace
> address, a cache flush is needed to make sure data is visible to
> userspace.
>
> >
> >> userspace address. This means when mips has THP on, the patch below
> >> is not enough to fix the issue.
> >>
> >> In post_alloc_hook(), it does not make sense to pass userspace address
> >> in to determine whether to flush dcache or not.
> >>
> >> One way to fix it is to add something like arch_userpage_post_alloc()
> >> to flush dcache if kmap address is aliased to userspace address.
> >> But my questions are that
> >> 1) if kmap address will always be the same for two separate kmap_local() calls,
> >
> > No.  It just takes the next address in the stack.
>
> So this fix will not work, since it is possible that first kmap and second
> kmap have different pages_do_alias() return values.
>
> Another way would be to make a special case for mips, like below.
> But that looks ugly, let me think about it more.
>
> diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
> index bc3e3484c1bf..ef3c6f0b9159 100644
> --- a/arch/mips/include/asm/page.h
> +++ b/arch/mips/include/asm/page.h
> @@ -95,6 +95,19 @@ struct vm_area_struct;
>  extern void copy_user_highpage(struct page *to, struct page *from,
>         unsigned long vaddr, struct vm_area_struct *vma);
>
> +struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
> +                                  unsigned long vaddr)
> + {
> +       struct folio *folio;
> +
> +       folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr);
> +       if (folio)
> +               clear_user_highpage(&folio->page, vaddr);
> +
> +       return folio;
> + }
> +#define vma_alloc_zeroed_movable_folio vma_alloc_zeroed_movable_folio
> +
>  #define __HAVE_ARCH_COPY_USER_HIGHPAGE
>
>  /*
> diff --git a/mm/internal.h b/mm/internal.h
> index cb8d8e8e3ffa..d513fa683aa3 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1287,7 +1287,8 @@ void touch_pmd(struct vm_area_struct *vma, unsigned long addr,
>
>  static inline bool alloc_zeroed(void)
>  {
> -       return static_branch_maybe(CONFIG_INIT_ON_ALLOC_DEFAULT_ON,
> +       return !IS_ENABLED(CONFIG_MIPS) &&
> +               static_branch_maybe(CONFIG_INIT_ON_ALLOC_DEFAULT_ON,
>                         &init_on_alloc);
>  }

After adding a missing static inline, #include <linux/gfp.h>, and still
getting compile failures, I gave up...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds