Messages in this thread |  | | Date | Sun, 24 Apr 2022 12:10:07 +0800 | Subject | Re: [PATCH -next v5 3/5] mm: page_table_check: add hooks to public helpers | From | Tong Tiangen <> |
| |
在 2022/4/22 14:05, Anshuman Khandual 写道: > > > On 4/21/22 13:50, Tong Tiangen wrote: >> Move ptep_clear() to the include/linux/pgtable.h and add page table check >> relate hooks to some helpers, it's prepare for support page table check >> feature on new architecture. > > Could instrumenting generic page table helpers (fallback instances when its > corresponding __HAVE_ARCH_XXX is not defined on the platform), might add all > the page table check hooks into paths on platforms which have not subscribed > ARCH_SUPPORTS_PAGE_TABLE_CHECK in the first place ? Although these looks have > !CONFIG_PAGE_TABLE_CHECK fallback stubs in the header, hence a build problem > gets avoided.
Right, build problems are avoided by fallback stubs in the header file.
> >> >> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com> >> Acked-by: Pasha Tatashin <pasha.tatashin@soleen.com> >> --- >> arch/x86/include/asm/pgtable.h | 10 ---------- >> include/linux/pgtable.h | 26 ++++++++++++++++++-------- >> 2 files changed, 18 insertions(+), 18 deletions(-) >> >> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h >> index 564abe42b0f7..51cd39858f81 100644 >> --- a/arch/x86/include/asm/pgtable.h >> +++ b/arch/x86/include/asm/pgtable.h >> @@ -1073,16 +1073,6 @@ static inline pte_t ptep_get_and_clear_full(struct mm_struct *mm, >> return pte; >> } >> >> -#define __HAVE_ARCH_PTEP_CLEAR > > AFICS X86 is the only platform subscribing __HAVE_ARCH_PTEP_CLEAR. Hence if > this is getting dropped for generic ptep_clear(), then no need to add back > #ifnded __HAVE_ARCH_PTEP_CLEAR construct. Generic ptep_clear() is the only > definition for all platforms ? > > Also if this patch is trying to drop off __HAVE_ARCH_PTEP_CLEAR along with > other page table check related changes, it needs to be done via a separate > patch instead.
Agreed. IMO, this fix can be patched later.
> >> -static inline void ptep_clear(struct mm_struct *mm, unsigned long addr, >> - pte_t *ptep) >> -{ >> - if (IS_ENABLED(CONFIG_PAGE_TABLE_CHECK)) >> - ptep_get_and_clear(mm, addr, ptep); >> - else >> - pte_clear(mm, addr, ptep); >> -} >> - >> #define __HAVE_ARCH_PTEP_SET_WRPROTECT >> static inline void ptep_set_wrprotect(struct mm_struct *mm, >> unsigned long addr, pte_t *ptep) >> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h >> index 49ab8ee2d6d7..10d2d91edf20 100644 >> --- a/include/linux/pgtable.h >> +++ b/include/linux/pgtable.h >> @@ -12,6 +12,7 @@ >> #include <linux/bug.h> >> #include <linux/errno.h> >> #include <asm-generic/pgtable_uffd.h> >> +#include <linux/page_table_check.h> >> >> #if 5 - defined(__PAGETABLE_P4D_FOLDED) - defined(__PAGETABLE_PUD_FOLDED) - \ >> defined(__PAGETABLE_PMD_FOLDED) != CONFIG_PGTABLE_LEVELS >> @@ -272,14 +273,6 @@ static inline bool arch_has_hw_pte_young(void) >> } >> #endif >> >> -#ifndef __HAVE_ARCH_PTEP_CLEAR >> -static inline void ptep_clear(struct mm_struct *mm, unsigned long addr, >> - pte_t *ptep) >> -{ >> - pte_clear(mm, addr, ptep); >> -} >> -#endif >> - >> #ifndef __HAVE_ARCH_PTEP_GET_AND_CLEAR >> static inline pte_t ptep_get_and_clear(struct mm_struct *mm, >> unsigned long address, >> @@ -287,10 +280,22 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, >> { >> pte_t pte = *ptep; >> pte_clear(mm, address, ptep); >> + page_table_check_pte_clear(mm, address, pte); >> return pte; >> } >> #endif >> >> +#ifndef __HAVE_ARCH_PTEP_CLEAR >> +static inline void ptep_clear(struct mm_struct *mm, unsigned long addr, >> + pte_t *ptep) >> +{ >> + if (IS_ENABLED(CONFIG_PAGE_TABLE_CHECK)) >> + ptep_get_and_clear(mm, addr, ptep); >> + else >> + pte_clear(mm, addr, ptep); > > Could not this be reworked to avoid IS_ENABLED() ? This is confusing. If the page > table hooks can be added to all potential page table paths via generic helpers, > irrespective of CONFIG_PAGE_TABLE_CHECK option, there is no rationale for doing > a IS_ENABLED() check here. >
From the perspective of code logic, we need to check the pte before being cleared. Whether pte check is required depends on IS_ENABLED().
Are there any suggestions for better implementation?
Thank you, Tong.
>> +} >> +#endif >> + >> #ifndef __HAVE_ARCH_PTEP_GET >> static inline pte_t ptep_get(pte_t *ptep) >> { >> @@ -360,7 +365,10 @@ static inline pmd_t pmdp_huge_get_and_clear(struct mm_struct *mm, >> pmd_t *pmdp) >> { >> pmd_t pmd = *pmdp; >> + >> pmd_clear(pmdp); >> + page_table_check_pmd_clear(mm, address, pmd); >> + >> return pmd; >> } >> #endif /* __HAVE_ARCH_PMDP_HUGE_GET_AND_CLEAR */ >> @@ -372,6 +380,8 @@ static inline pud_t pudp_huge_get_and_clear(struct mm_struct *mm, >> pud_t pud = *pudp; >> >> pud_clear(pudp); >> + page_table_check_pud_clear(mm, address, pud); >> + >> return pud; >> } >> #endif /* __HAVE_ARCH_PUDP_HUGE_GET_AND_CLEAR */ > .
|  |