[PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[Peter Xu]

I noticed a few collision usage on VM_FAULT_* definition in the page fault
path on arm/arm64/s390 where the VM_FAULT_* can overlap with the generic
definition of vm_fault_reason.

The major overlapped part being VM_FAULT_HINDEX_MASK which is used only by
the hugetlb hwpoisoning.

I'm not sure whether any of them can have a real impact, but that does not
look like to be expected.  I didn't copy stable, if anyone thinks it should
please shoot.  Nor did I test them in any form - I just changed the
allocations from top bits and added a comment for each of them.

Please have a look, thanks.

Peter Xu (3):
  mm/arm: Define private VM_FAULT_* reasons from top bits
  mm/arm64: Define private VM_FAULT_* reasons from top bits
  mm/s390: Define private VM_FAULT_* reasons from top bits

 arch/arm/mm/fault.c   |  8 ++++++--
 arch/arm64/mm/fault.c |  8 ++++++--
 arch/s390/mm/fault.c  | 14 +++++++++-----
 3 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] mm/arm: Define private VM_FAULT_* reasons from top bits

[Peter Xu]

The current definition already collapse with the generic definition of
vm_fault_reason.  Move the private definitions to allocate bits from the
top of uint so they won't collapse anymore.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/arm/mm/fault.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 2418f1efabd8..67879d995835 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -216,8 +216,12 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 }
 
 #ifdef CONFIG_MMU
-#define VM_FAULT_BADMAP		((__force vm_fault_t)0x010000)
-#define VM_FAULT_BADACCESS	((__force vm_fault_t)0x020000)
+/*
+ * Allocate private vm_fault_reason from top.  Please make sure it won't
+ * collide with vm_fault_reason.
+ */
+#define VM_FAULT_BADMAP		((__force vm_fault_t)0x80000000)
+#define VM_FAULT_BADACCESS	((__force vm_fault_t)0x40000000)
 
 static inline bool is_permission_fault(unsigned int fsr)
 {
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] mm/arm64: Define private VM_FAULT_* reasons from top bits

[Peter Xu]

The current definition already collapse with the generic definition of
vm_fault_reason.  Move the private definitions to allocate bits from the
top of uint so they won't collapse anymore.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/arm64/mm/fault.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 596f46dabe4e..44fd16f58f94 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -480,8 +480,12 @@ static void do_bad_area(unsigned long far, unsigned long esr,
 	}
 }
 
-#define VM_FAULT_BADMAP		0x010000
-#define VM_FAULT_BADACCESS	0x020000
+/*
+ * Allocate private vm_fault_reason from top.  Please make sure it won't
+ * collide with vm_fault_reason.
+ */
+#define VM_FAULT_BADMAP		((__force vm_fault_t)0x80000000)
+#define VM_FAULT_BADACCESS	((__force vm_fault_t)0x40000000)
 
 static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr,
 				  unsigned int mm_flags, unsigned long vm_flags,
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] mm/s390: Define private VM_FAULT_* reasons from top bits

[Peter Xu]

The current definition already collapse with the generic definition of
vm_fault_reason.  Move the private definitions to allocate bits from the
top of uint so they won't collapse anymore.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/s390/mm/fault.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 9649d9382e0a..cebfbd6dcbaf 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -46,11 +46,15 @@
 #define __SUBCODE_MASK 0x0600
 #define __PF_RES_FIELD 0x8000000000000000ULL
 
-#define VM_FAULT_BADCONTEXT	((__force vm_fault_t) 0x010000)
-#define VM_FAULT_BADMAP		((__force vm_fault_t) 0x020000)
-#define VM_FAULT_BADACCESS	((__force vm_fault_t) 0x040000)
-#define VM_FAULT_SIGNAL		((__force vm_fault_t) 0x080000)
-#define VM_FAULT_PFAULT		((__force vm_fault_t) 0x100000)
+/*
+ * Allocate private vm_fault_reason from top.  Please make sure it won't
+ * collide with vm_fault_reason.
+ */
+#define VM_FAULT_BADCONTEXT	((__force vm_fault_t) 0x80000000)
+#define VM_FAULT_BADMAP		((__force vm_fault_t) 0x40000000)
+#define VM_FAULT_BADACCESS	((__force vm_fault_t) 0x20000000)
+#define VM_FAULT_SIGNAL		((__force vm_fault_t) 0x10000000)
+#define VM_FAULT_PFAULT		((__force vm_fault_t) 0x8000000)
 
 enum fault_type {
 	KERNEL_FAULT,
-- 
2.37.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] mm/s390: Define private VM_FAULT_* reasons from top bits

[Heiko Carstens]

On Sun, Feb 05, 2023 at 06:17:04PM -0500, Peter Xu wrote:
> The current definition already collapse with the generic definition of
> vm_fault_reason.  Move the private definitions to allocate bits from the
> top of uint so they won't collapse anymore.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/s390/mm/fault.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Thanks a lot!

Applied.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[Matthew Wilcox]

On Sun, Feb 05, 2023 at 06:17:01PM -0500, Peter Xu wrote:
> I noticed a few collision usage on VM_FAULT_* definition in the page fault
> path on arm/arm64/s390 where the VM_FAULT_* can overlap with the generic
> definition of vm_fault_reason.
> 
> The major overlapped part being VM_FAULT_HINDEX_MASK which is used only by
> the hugetlb hwpoisoning.
> 
> I'm not sure whether any of them can have a real impact, but that does not
> look like to be expected.  I didn't copy stable, if anyone thinks it should
> please shoot.  Nor did I test them in any form - I just changed the
> allocations from top bits and added a comment for each of them.

This seems like a bad way to do it.  Why not just put these VM_FAULT_*
definitions in linux/mm_types.h?  Then we'll see them when adding new
VM_FAULT codes.  Sure, they won't be used by every architecture, but
so what?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[Peter Xu]

On Mon, Feb 06, 2023 at 12:10:53AM +0000, Matthew Wilcox wrote:
> On Sun, Feb 05, 2023 at 06:17:01PM -0500, Peter Xu wrote:
> > I noticed a few collision usage on VM_FAULT_* definition in the page fault
> > path on arm/arm64/s390 where the VM_FAULT_* can overlap with the generic
> > definition of vm_fault_reason.
> > 
> > The major overlapped part being VM_FAULT_HINDEX_MASK which is used only by
> > the hugetlb hwpoisoning.
> > 
> > I'm not sure whether any of them can have a real impact, but that does not
> > look like to be expected.  I didn't copy stable, if anyone thinks it should
> > please shoot.  Nor did I test them in any form - I just changed the
> > allocations from top bits and added a comment for each of them.
> 
> This seems like a bad way to do it.  Why not just put these VM_FAULT_*
> definitions in linux/mm_types.h?  Then we'll see them when adding new
> VM_FAULT codes.  Sure, they won't be used by every architecture, but
> so what?

My initial version actually contains a few VM_FAULT_PRIVATE_N there, but I
noticed only the minority uses that, especially there's s390 which takes 5
entries.  I didn't had my mind straight on which's the best to go, then I
removed them and posted this simpler version, with comment on each to fix
the issues, more in a sense of raising the problem first.

I agree it isn't a problem at all, not until 32 bits all used up.  But that
seems to slightly encourage more archs from using the new private entries
which I wanted to avoid.

If to take a closer look, we may not really need that much private entries.
With s390, what I read is:

  - VM_FAULT_BADMAP could be replaced directly with VM_FAULT_SIGSEGV?
  - VM_FAULT_PFAULT could be replaced directly with VM_FAULT_BADCONTEXT?

Then if I'm not wrong we can already reduce 5->3 private entries.

I didn't directly change that because I am not 100% confident and I can't
test them myself.  It'll be great if arch people can have a look at either
s390 and arm to see whether there's chance of simplifcations first.  So the
patchset is more of raising the collision issue first, meanwhile great to
attract attention for arch people to refactor upon it.

I can also try to reduce the private entries and introduce PRIVATE entries
accordingly as you suggested, but I'll need more help on reviews and tests
than this one.

Thanks,

-- 
Peter Xu


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[Matthew Wilcox]

On Sun, Feb 05, 2023 at 07:54:35PM -0500, Peter Xu wrote:
> On Mon, Feb 06, 2023 at 12:10:53AM +0000, Matthew Wilcox wrote:
> > On Sun, Feb 05, 2023 at 06:17:01PM -0500, Peter Xu wrote:
> > > I noticed a few collision usage on VM_FAULT_* definition in the page fault
> > > path on arm/arm64/s390 where the VM_FAULT_* can overlap with the generic
> > > definition of vm_fault_reason.
> > > 
> > > The major overlapped part being VM_FAULT_HINDEX_MASK which is used only by
> > > the hugetlb hwpoisoning.
> > > 
> > > I'm not sure whether any of them can have a real impact, but that does not
> > > look like to be expected.  I didn't copy stable, if anyone thinks it should
> > > please shoot.  Nor did I test them in any form - I just changed the
> > > allocations from top bits and added a comment for each of them.
> > 
> > This seems like a bad way to do it.  Why not just put these VM_FAULT_*
> > definitions in linux/mm_types.h?  Then we'll see them when adding new
> > VM_FAULT codes.  Sure, they won't be used by every architecture, but
> > so what?
> 
> My initial version actually contains a few VM_FAULT_PRIVATE_N there, but I

That wasn't what I meant.  I meant putting VM_FAULT_BADMAP and
VM_FAULT_SIGSEGV in mm_types.h.  Not having "Here is a range of reserved
arch private ones".


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[Peter Xu]

On Mon, Feb 06, 2023 at 02:51:18AM +0000, Matthew Wilcox wrote:
> That wasn't what I meant.  I meant putting VM_FAULT_BADMAP and
> VM_FAULT_SIGSEGV in mm_types.h.  Not having "Here is a range of reserved
> arch private ones".

VM_FAULT_SIGSEGV is there already; I assume you meant adding them all
directly into vm_fault_reason.

Then I don't think it's a good idea..

Currently vm_fault_reason is a clear interface for handle_mm_fault() for
not only arch pffault handlers but also soft faults like GUP.

If handle_mm_fault() doesn't return VM_FAULT_BADMAP at all, I don't think
we should have it as public API at all.  When arch1 people reading the
VM_FAULT_ documents, it shouldn't care about some fault reason that only
happens with arch2.  Gup shouldn't care about it either.

Logically a new page fault handler should handle all the retval of
vm_fault_reason afaiu.  That shouldn't include e.g. VM_FAULT_BADMAP either.

Thanks,

-- 
Peter Xu


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[Matthew Wilcox]

On Sun, Feb 05, 2023 at 10:18:30PM -0500, Peter Xu wrote:
> On Mon, Feb 06, 2023 at 02:51:18AM +0000, Matthew Wilcox wrote:
> > That wasn't what I meant.  I meant putting VM_FAULT_BADMAP and
> > VM_FAULT_SIGSEGV in mm_types.h.  Not having "Here is a range of reserved
> > arch private ones".
> 
> VM_FAULT_SIGSEGV is there already; I assume you meant adding them all
> directly into vm_fault_reason.
> 
> Then I don't think it's a good idea..
> 
> Currently vm_fault_reason is a clear interface for handle_mm_fault() for
> not only arch pffault handlers but also soft faults like GUP.
> 
> If handle_mm_fault() doesn't return VM_FAULT_BADMAP at all, I don't think
> we should have it as public API at all.  When arch1 people reading the
> VM_FAULT_ documents, it shouldn't care about some fault reason that only
> happens with arch2.  Gup shouldn't care about it either.
> 
> Logically a new page fault handler should handle all the retval of
> vm_fault_reason afaiu.  That shouldn't include e.g. VM_FAULT_BADMAP either.

Hmm, right.  Looking specifically at how s390 uses VM_FAULT_BADMAP,
it just seems to be a badly structured fault.c.  Seems to me that
do_fault_error() should take an extra si_code argument, and
instead of returning VM_FAULT_BADACCESS / VM_FAULT_BADMAP from
various functions, those functions should call do_fault_error()
directly, passing it VM_FAULT_SIGSEGV and the appropriate si_code.

But this is all on the s390 people to fix; I don't want to break their
arch by trying it myself.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] mm/arch: Fix a few collide definition on private use of VM_FAULT_*

[Heiko Carstens]

On Mon, Feb 06, 2023 at 05:09:57AM +0000, Matthew Wilcox wrote:
> On Sun, Feb 05, 2023 at 10:18:30PM -0500, Peter Xu wrote:
> > On Mon, Feb 06, 2023 at 02:51:18AM +0000, Matthew Wilcox wrote:
> > > That wasn't what I meant.  I meant putting VM_FAULT_BADMAP and
> > > VM_FAULT_SIGSEGV in mm_types.h.  Not having "Here is a range of reserved
> > > arch private ones".
> > 
> > VM_FAULT_SIGSEGV is there already; I assume you meant adding them all
> > directly into vm_fault_reason.
> > 
> > Then I don't think it's a good idea..
> > 
> > Currently vm_fault_reason is a clear interface for handle_mm_fault() for
> > not only arch pffault handlers but also soft faults like GUP.
> > 
> > If handle_mm_fault() doesn't return VM_FAULT_BADMAP at all, I don't think
> > we should have it as public API at all.  When arch1 people reading the
> > VM_FAULT_ documents, it shouldn't care about some fault reason that only
> > happens with arch2.  Gup shouldn't care about it either.
> > 
> > Logically a new page fault handler should handle all the retval of
> > vm_fault_reason afaiu.  That shouldn't include e.g. VM_FAULT_BADMAP either.
> 
> Hmm, right.  Looking specifically at how s390 uses VM_FAULT_BADMAP,
> it just seems to be a badly structured fault.c.  Seems to me that
> do_fault_error() should take an extra si_code argument, and
> instead of returning VM_FAULT_BADACCESS / VM_FAULT_BADMAP from
> various functions, those functions should call do_fault_error()
> directly, passing it VM_FAULT_SIGSEGV and the appropriate si_code.
> 
> But this is all on the s390 people to fix; I don't want to break their
> arch by trying it myself.

Yes, will take a look at it. For now I will apply Peter's patch in order to
get rid of the collision.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel