On Wed, Apr 17, 2024 at 12:53 PM Peter Xu <peterx@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx/ > > > > > > Cc: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx> > > > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx> > > > --- > > > 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
