Message-ID: <CA+CK2bACyxNgFACfSu=3ro-nn-+PXCMf6Mug5G0G1oWpWabq4Q@mail.gmail.com>
Date: Wed, 17 Apr 2024 13:17:26 -0400
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Peter Xu <peterx@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	David Hildenbrand <david@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Nadav Amit <nadav.amit@...il.com>, Axel Rasmussen <axelrasmussen@...gle.com>
Subject: Re: [PATCH] mm/page_table_check: Support userfault wr-protect entries

On Wed, Apr 17, 2024 at 12:53 PM Peter Xu <peterx@...hat.com> wrote:
>
> On Tue, Apr 16, 2024 at 05:34:50PM -0400, Pasha Tatashin wrote:
> > Hi Peter,
>
> Pasha,
>
> >
> > Thanks for this patch, I like this extra checking logic, my comments below:
>
> Thanks for taking a look.
>
> >
> > On Mon, Apr 15, 2024 at 4:53 PM Peter Xu <peterx@...hat.com> wrote:
> > >
> > > Allow page_table_check hooks to check over userfaultfd wr-protect criteria
> > > upon pgtable updates.  The rule is no co-existance allowed for any writable
> > > flag against userfault wr-protect flag.
> > >
> > > This should be better than c2da319c2e, where we used to only sanitize such
> > > issues during a pgtable walk, but when hitting such issue we don't have a
> > > good chance to know where does that writable bit came from [1], so that
> > > even the pgtable walk exposes a kernel bug (which is still helpful on
> > > triaging) but not easy to track and debug.
> > >
> > > Now we switch to track the source.  It's much easier too with the recent
> > > introduction of page table check.
> > >
> > > There are some limitations with using the page table check here for
> > > userfaultfd wr-protect purpose:
> > >
> > >   - It is only enabled with explicit enablement of page table check configs
> > >   and/or boot parameters, but should be good enough to track at least
> > >   syzbot issues, as syzbot should enable PAGE_TABLE_CHECK[_ENFORCED] for
> > >   x86 [1].  We used to have DEBUG_VM but it's now off for most distros,
> > >   while distros also normally not enable PAGE_TABLE_CHECK[_ENFORCED], which
> > >   is similar.
> > >
> > >   - It conditionally works with the ptep_modify_prot API.  It will be
> > >   bypassed when e.g. XEN PV is enabled, however still work for most of the
> > >   rest scenarios, which should be the common cases so should be good
> > >   enough.
> > >
> > >   - Hugetlb check is a bit hairy, as the page table check cannot identify
> > >   hugetlb pte or normal pte via trapping at set_pte_at(), because of the
> > >   current design where hugetlb maps every layers to pte_t... For example,
> > >   the default set_huge_pte_at() can invoke set_pte_at() directly and lose
> > >   the hugetlb context, treating it the same as a normal pte_t. So far it's
> > >   fine because we have huge_pte_uffd_wp() always equals to pte_uffd_wp() as
> > >   long as supported (x86 only).  It'll be a bigger problem when we'll
> > >   define _PAGE_UFFD_WP differently at various pgtable levels, because then
> > >   one huge_pte_uffd_wp() per-arch will stop making sense first.. as of now
> > >   we can leave this for later too.
> > >
> > > This patch also removes commit c2da319c2e altogether, as we have something
> > > better now.
> > >
> > > [1] https://lore.kernel.org/all/000000000000dce0530615c89210@google.com/
> > >
> > > Cc: Pasha Tatashin <pasha.tatashin@...een.com>
> > > Signed-off-by: Peter Xu <peterx@...hat.com>
> > > ---
> > >  arch/x86/include/asm/pgtable.h | 18 +-----------------
> > >  mm/page_table_check.c          | 32 +++++++++++++++++++++++++++++++-
> >
> > Please add the new logic to: Documentation/mm/page_table_check.rst
>
> Will do.
>
> >
> > >  2 files changed, 32 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> > > index 273f7557218c..65b8e5bb902c 100644
> > > --- a/arch/x86/include/asm/pgtable.h
> > > +++ b/arch/x86/include/asm/pgtable.h
> > > @@ -388,23 +388,7 @@ static inline pte_t pte_wrprotect(pte_t pte)
> > >  #ifdef CONFIG_HAVE_ARCH_USERFAULTFD_WP
> > >  static inline int pte_uffd_wp(pte_t pte)
> > >  {
> > > -       bool wp = pte_flags(pte) & _PAGE_UFFD_WP;
> > > -
> > > -#ifdef CONFIG_DEBUG_VM
> > > -       /*
> > > -        * Having write bit for wr-protect-marked present ptes is fatal,
> > > -        * because it means the uffd-wp bit will be ignored and write will
> > > -        * just go through.
> > > -        *
> > > -        * Use any chance of pgtable walking to verify this (e.g., when
> > > -        * page swapped out or being migrated for all purposes). It means
> > > -        * something is already wrong.  Tell the admin even before the
> > > -        * process crashes. We also nail it with wrong pgtable setup.
> > > -        */
> > > -       WARN_ON_ONCE(wp && pte_write(pte));
> > > -#endif
> > > -
> > > -       return wp;
> > > +       return pte_flags(pte) & _PAGE_UFFD_WP;
> > >  }
> > >
> > >  static inline pte_t pte_mkuffd_wp(pte_t pte)
> > > diff --git a/mm/page_table_check.c b/mm/page_table_check.c
> > > index af69c3c8f7c2..d4eb1212f0f5 100644
> > > --- a/mm/page_table_check.c
> > > +++ b/mm/page_table_check.c
> > > @@ -7,6 +7,8 @@
> > >  #include <linux/kstrtox.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/page_table_check.h>
> > > +#include <linux/swap.h>
> > > +#include <linux/swapops.h>
> > >
> > >  #undef pr_fmt
> > >  #define pr_fmt(fmt)    "page_table_check: " fmt
> > > @@ -182,6 +184,23 @@ void __page_table_check_pud_clear(struct mm_struct *mm, pud_t pud)
> > >  }
> > >  EXPORT_SYMBOL(__page_table_check_pud_clear);
> > >
> > > +/* Whether the swap entry cached writable information */
> > > +static inline bool swap_cached_writable(swp_entry_t entry)
> > > +{
> > > +       unsigned type = swp_type(entry);
> > > +
> > > +       return type == SWP_DEVICE_EXCLUSIVE_WRITE ||
> > > +           type == SWP_MIGRATION_WRITE;
> > > +}
> > > +
> > > +static inline void __page_table_check_pte(pte_t pte)
> >
> > may be something like:
> > page_table_check_new_pte() ? Naming is starting to get confusing. The
> > idea for this function is to check the pte that is about to be set
> > into the page table.
>
> But then we keep __page_table_check_ptes_set() as is?
>
> It feels more natural if we keep using those underscores if all the rest
> does so.  The "_new" is also not matching with what you used to have as

In mm/page_table_check.c, function names with an underscore prefix are
intended for global symbols with internal use only. All local
functions, such as page_table_check_set() and
page_table_check_clear(), do not have this prefix as we do not pollute
the global namespace.

> "_set".  If you see that's how I carefully chose the current name, with the
> hope to match everything..
>
> No strong opinions on these, but let me know your final choice of such
> name.  I'm happy to align that to your preference.
>
> >
> > > +{
> > > +       if (pte_present(pte) && pte_uffd_wp(pte))
> > > +               WARN_ON_ONCE(pte_write(pte));
> > > +       else if (is_swap_pte(pte) && pte_swp_uffd_wp(pte))
> > > +               WARN_ON_ONCE(swap_cached_writable(pte_to_swp_entry(pte)));
> > > +}
> > > +
> > >  void __page_table_check_ptes_set(struct mm_struct *mm, pte_t *ptep, pte_t pte,
> > >                 unsigned int nr)
> > >  {
> > > @@ -190,18 +209,29 @@ void __page_table_check_ptes_set(struct mm_struct *mm, pte_t *ptep, pte_t pte,
> > >         if (&init_mm == mm)
> > >                 return;
> > >
> > > -       for (i = 0; i < nr; i++)
> > > +       for (i = 0; i < nr; i++) {
> > > +               __page_table_check_pte(pte);
> >
> > This should really be called only once after this loop.
>
> This is also my intention to keep it in the loop just to make it as generic
> e.g. to have no assumption of "ignoring PFNs", and I didn't worry on perf
> much as we'll read/write these ptes anyway, also because it's only enabled
> for debugging kernels.
>
> But I made it at least inaccurate by checking pte not *ptep..
> How about I move it out, rename it to __page_table_check_pte_flags(pte)?

Sounds good. I like:
 page_table_check_pte_flags()
 page_table_check_pmd_flags()

Pasha

Powered by blists - more mailing lists