在 2022/4/25 13:52, Anshuman Khandual 写道:
On 4/24/22 09:40, Tong Tiangen wrote:
在 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.
Although there might not be a build problem as such, but should non subscribing
platforms get their page table helpers instrumented with page table check hooks
in the first place ? The commit message should address these questions.
Add description in commit message to explain that:
Non subscription platforms will call a fallback ptc stubs when getting
their page table helpers[1] in include/linux/pgtable.h.
[1]
ptep_clear/ptep_get_and_clear/pmdp_huge_get_and_clear/pudp_huge_get_and_clear
Am i right? :)
Thanks,
Tong.
Signed-off-by: Tong Tiangen <tongtiangen@xxxxxxxxxx>
Acked-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
---
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?
But other generic page table helpers already have page table check hooks
instrumented without IS_ENABLED() checks, then why this is any different.
Maybe i understand what you said, the more reasonable implement is:
static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned
long address, pte_t *ptep)
{
pte_t pte = *ptep;
pte_clear(mm, address, ptep);
page_table_check_pte_clear(mm, address, pte);
return pte;
}
static inline void ptep_clear(struct mm_struct *mm, unsigned long addr,
pte_t *ptep)
{
ptep_get_and_clear(mm, address, ptep);
}
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 */
.
.
