| Message ID | 20231114071130.197966-1-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite. > But that got dropped by > commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to replace savedwrite") > > With the change in this patch numa fault pte (pte_protnone()) gets mapped as regular user pte > with RWX cleared (no-access) whereas earlier it used to be mapped _PAGE_PRIVILEGED. > > Hash fault handling code did get some WARN_ON added because those > functions are not expected to get called with _PAGE_READ cleared. > commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page fault path") > explains the details. You say "did get" which makes me think you're talking about the past. But I think you're referring to the WARN_ON you are adding in this patch? > Also revert commit 1abce0580b89 ("powerpc/64s: Fix __pte_needs_flush() false positive warning") That could be done separately as a follow-up couldn't it? Would reduce the diff size. > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > arch/powerpc/include/asm/book3s/64/pgtable.h | 9 +++------ > arch/powerpc/include/asm/book3s/64/tlbflush.h | 9 ++------- > arch/powerpc/mm/book3s64/hash_utils.c | 7 +++++++ > 3 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h > index cb77eddca54b..2cc58ac74080 100644 > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h > @@ -17,12 +17,6 @@ > #define _PAGE_EXEC 0x00001 /* execute permission */ > #define _PAGE_WRITE 0x00002 /* write access allowed */ > #define _PAGE_READ 0x00004 /* read access allowed */ > -#define _PAGE_NA _PAGE_PRIVILEGED > -#define _PAGE_NAX _PAGE_EXEC > -#define _PAGE_RO _PAGE_READ > -#define _PAGE_ROX (_PAGE_READ | _PAGE_EXEC) > -#define _PAGE_RW (_PAGE_READ | _PAGE_WRITE) > -#define _PAGE_RWX (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC) Those are unrelated I think? > #define _PAGE_PRIVILEGED 0x00008 /* kernel access only */ > #define _PAGE_SAO 0x00010 /* Strong access order */ > #define _PAGE_NON_IDEMPOTENT 0x00020 /* non idempotent memory */ > @@ -529,6 +523,9 @@ static inline bool pte_user(pte_t pte) > } > > #define pte_access_permitted pte_access_permitted > +/* > + * execute-only mappings return false > + */ That would fit better in the existing comment block inside the function I think. Normally this location would be a function description comment. > static inline bool pte_access_permitted(pte_t pte, bool write) > { > /* ie. here cheers
Le 01/12/2023 à 11:35, Michael Ellerman a écrit : > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: >> There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite. >> But that got dropped by >> commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to replace savedwrite") >> >> With the change in this patch numa fault pte (pte_protnone()) gets mapped as regular user pte >> with RWX cleared (no-access) whereas earlier it used to be mapped _PAGE_PRIVILEGED. >> >> Hash fault handling code did get some WARN_ON added because those >> functions are not expected to get called with _PAGE_READ cleared. >> commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page fault path") >> explains the details. > > You say "did get" which makes me think you're talking about the past. > But I think you're referring to the WARN_ON you are adding in this patch? > >> Also revert commit 1abce0580b89 ("powerpc/64s: Fix __pte_needs_flush() false positive warning") > > That could be done separately as a follow-up couldn't it? Would reduce > the diff size. > >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> arch/powerpc/include/asm/book3s/64/pgtable.h | 9 +++------ >> arch/powerpc/include/asm/book3s/64/tlbflush.h | 9 ++------- >> arch/powerpc/mm/book3s64/hash_utils.c | 7 +++++++ >> 3 files changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h >> index cb77eddca54b..2cc58ac74080 100644 >> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h >> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h >> @@ -17,12 +17,6 @@ >> #define _PAGE_EXEC 0x00001 /* execute permission */ >> #define _PAGE_WRITE 0x00002 /* write access allowed */ >> #define _PAGE_READ 0x00004 /* read access allowed */ >> -#define _PAGE_NA _PAGE_PRIVILEGED > >> -#define _PAGE_NAX _PAGE_EXEC >> -#define _PAGE_RO _PAGE_READ >> -#define _PAGE_ROX (_PAGE_READ | _PAGE_EXEC) >> -#define _PAGE_RW (_PAGE_READ | _PAGE_WRITE) >> -#define _PAGE_RWX (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC) > > Those are unrelated I think? They are related. Those exists only because _PAGE_NA is different from the one defined in asm/pgtable-masks.h As soon as you remove _PAGE_PRIVILEGED from _PAGE_NA, everything become standard and is taken from asm/pgtable-masks.h > >> #define _PAGE_PRIVILEGED 0x00008 /* kernel access only */ >> #define _PAGE_SAO 0x00010 /* Strong access order */ >> #define _PAGE_NON_IDEMPOTENT 0x00020 /* non idempotent memory */ >> @@ -529,6 +523,9 @@ static inline bool pte_user(pte_t pte) >> } >> >> #define pte_access_permitted pte_access_permitted >> +/* >> + * execute-only mappings return false >> + */ > > That would fit better in the existing comment block inside the function > I think. Normally this location would be a function description comment. > >> static inline bool pte_access_permitted(pte_t pte, bool write) >> { >> /* > ie. here > > cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: >> There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite. >> But that got dropped by >> commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to replace savedwrite") >> >> With the change in this patch numa fault pte (pte_protnone()) gets mapped as regular user pte >> with RWX cleared (no-access) whereas earlier it used to be mapped _PAGE_PRIVILEGED. >> >> Hash fault handling code did get some WARN_ON added because those >> functions are not expected to get called with _PAGE_READ cleared. >> commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page fault path") >> explains the details. > > You say "did get" which makes me think you're talking about the past. > But I think you're referring to the WARN_ON you are adding in this patch? That is correct. Will update this as "Hash fault handing code gets some WARN_ON added in this patch ..." ? > > >> Also revert commit 1abce0580b89 ("powerpc/64s: Fix __pte_needs_flush() false positive warning") > > That could be done separately as a follow-up couldn't it? Would reduce > the diff size. > Will split that to a separate patch. >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> arch/powerpc/include/asm/book3s/64/pgtable.h | 9 +++------ >> arch/powerpc/include/asm/book3s/64/tlbflush.h | 9 ++------- >> arch/powerpc/mm/book3s64/hash_utils.c | 7 +++++++ >> 3 files changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h >> index cb77eddca54b..2cc58ac74080 100644 >> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h >> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h >> @@ -17,12 +17,6 @@ >> #define _PAGE_EXEC 0x00001 /* execute permission */ >> #define _PAGE_WRITE 0x00002 /* write access allowed */ >> #define _PAGE_READ 0x00004 /* read access allowed */ >> -#define _PAGE_NA _PAGE_PRIVILEGED > >> -#define _PAGE_NAX _PAGE_EXEC >> -#define _PAGE_RO _PAGE_READ >> -#define _PAGE_ROX (_PAGE_READ | _PAGE_EXEC) >> -#define _PAGE_RW (_PAGE_READ | _PAGE_WRITE) >> -#define _PAGE_RWX (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC) > > Those are unrelated I think? > If we don't require _PAGE_NA we can fallback to generic version. >> #define _PAGE_PRIVILEGED 0x00008 /* kernel access only */ >> #define _PAGE_SAO 0x00010 /* Strong access order */ >> #define _PAGE_NON_IDEMPOTENT 0x00020 /* non idempotent memory */ >> @@ -529,6 +523,9 @@ static inline bool pte_user(pte_t pte) >> } >> >> #define pte_access_permitted pte_access_permitted >> +/* >> + * execute-only mappings return false >> + */ > > That would fit better in the existing comment block inside the function > I think. Normally this location would be a function description comment. > Will move. >> static inline bool pte_access_permitted(pte_t pte, bool write) >> { >> /* > ie. here > > cheers Thanks -aneesh
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h index cb77eddca54b..2cc58ac74080 100644 --- a/arch/powerpc/include/asm/book3s/64/pgtable.h +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h @@ -17,12 +17,6 @@ #define _PAGE_EXEC 0x00001 /* execute permission */ #define _PAGE_WRITE 0x00002 /* write access allowed */ #define _PAGE_READ 0x00004 /* read access allowed */ -#define _PAGE_NA _PAGE_PRIVILEGED -#define _PAGE_NAX _PAGE_EXEC -#define _PAGE_RO _PAGE_READ -#define _PAGE_ROX (_PAGE_READ | _PAGE_EXEC) -#define _PAGE_RW (_PAGE_READ | _PAGE_WRITE) -#define _PAGE_RWX (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC) #define _PAGE_PRIVILEGED 0x00008 /* kernel access only */ #define _PAGE_SAO 0x00010 /* Strong access order */ #define _PAGE_NON_IDEMPOTENT 0x00020 /* non idempotent memory */ @@ -529,6 +523,9 @@ static inline bool pte_user(pte_t pte) } #define pte_access_permitted pte_access_permitted +/* + * execute-only mappings return false + */ static inline bool pte_access_permitted(pte_t pte, bool write) { /* diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h index 1950c1b825b4..fd642b729775 100644 --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h @@ -158,11 +158,6 @@ static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, */ } -static inline bool __pte_protnone(unsigned long pte) -{ - return (pte & (pgprot_val(PAGE_NONE) | _PAGE_RWX)) == pgprot_val(PAGE_NONE); -} - static inline bool __pte_flags_need_flush(unsigned long oldval, unsigned long newval) { @@ -179,8 +174,8 @@ static inline bool __pte_flags_need_flush(unsigned long oldval, /* * We do not expect kernel mappings or non-PTEs or not-present PTEs. */ - VM_WARN_ON_ONCE(!__pte_protnone(oldval) && oldval & _PAGE_PRIVILEGED); - VM_WARN_ON_ONCE(!__pte_protnone(newval) && newval & _PAGE_PRIVILEGED); + VM_WARN_ON_ONCE(oldval & _PAGE_PRIVILEGED); + VM_WARN_ON_ONCE(newval & _PAGE_PRIVILEGED); VM_WARN_ON_ONCE(!(oldval & _PAGE_PTE)); VM_WARN_ON_ONCE(!(newval & _PAGE_PTE)); VM_WARN_ON_ONCE(!(oldval & _PAGE_PRESENT)); diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c index ad2afa08e62e..0626a25b0d72 100644 --- a/arch/powerpc/mm/book3s64/hash_utils.c +++ b/arch/powerpc/mm/book3s64/hash_utils.c @@ -310,9 +310,16 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags, unsigned long flags else rflags |= 0x3; } + VM_WARN_ONCE(!(pteflags & _PAGE_RWX), "no-access mapping request"); } else { if (pteflags & _PAGE_RWX) rflags |= 0x2; + /* + * We should never hit this in normal fault handling because + * a permission check (check_pte_access()) will bubble this + * to higher level linux handler even for PAGE_NONE. + */ + VM_WARN_ONCE(!(pteflags & _PAGE_RWX), "no-access mapping request"); if (!((pteflags & _PAGE_WRITE) && (pteflags & _PAGE_DIRTY))) rflags |= 0x1; }
There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite. But that got dropped by commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to replace savedwrite") With the change in this patch numa fault pte (pte_protnone()) gets mapped as regular user pte with RWX cleared (no-access) whereas earlier it used to be mapped _PAGE_PRIVILEGED. Hash fault handling code did get some WARN_ON added because those functions are not expected to get called with _PAGE_READ cleared. commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page fault path") explains the details. Also revert commit 1abce0580b89 ("powerpc/64s: Fix __pte_needs_flush() false positive warning") Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- arch/powerpc/include/asm/book3s/64/pgtable.h | 9 +++------ arch/powerpc/include/asm/book3s/64/tlbflush.h | 9 ++------- arch/powerpc/mm/book3s64/hash_utils.c | 7 +++++++ 3 files changed, 12 insertions(+), 13 deletions(-)