diff mbox series

[v5,02/25] mm: userfault: return VM_FAULT_RETRY on signals

Message ID 20190620022008.19172-3-peterx@redhat.com (mailing list archive)
State New, archived
Headers show
Series userfaultfd: write protection support | expand

Commit Message

Peter Xu June 20, 2019, 2:19 a.m. UTC 
The idea comes from the upstream discussion between Linus and Andrea:

  https://lkml.org/lkml/2017/10/30/560

A summary to the issue: there was a special path in handle_userfault()
in the past that we'll return a VM_FAULT_NOPAGE when we detected
non-fatal signals when waiting for userfault handling.  We did that by
reacquiring the mmap_sem before returning.  However that brings a risk
in that the vmas might have changed when we retake the mmap_sem and
even we could be holding an invalid vma structure.

This patch removes the special path and we'll return a VM_FAULT_RETRY
with the common path even if we have got such signals.  Then for all
the architectures that is passing in VM_FAULT_ALLOW_RETRY into
handle_mm_fault(), we check not only for SIGKILL but for all the rest
of userspace pending signals right after we returned from
handle_mm_fault().  This can allow the userspace to handle nonfatal
signals faster than before.

This patch is a preparation work for the next patch to finally remove
the special code path mentioned above in handle_userfault().

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
Reviewed-by: Jerome Glisse <jglisse@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/alpha/mm/fault.c      |  2 +-
 arch/arc/mm/fault.c        | 11 ++++-------
 arch/arm/mm/fault.c        |  6 +++---
 arch/arm64/mm/fault.c      |  6 +++---
 arch/hexagon/mm/vm_fault.c |  2 +-
 arch/ia64/mm/fault.c       |  2 +-
 arch/m68k/mm/fault.c       |  2 +-
 arch/microblaze/mm/fault.c |  2 +-
 arch/mips/mm/fault.c       |  2 +-
 arch/nds32/mm/fault.c      |  6 +++---
 arch/nios2/mm/fault.c      |  2 +-
 arch/openrisc/mm/fault.c   |  2 +-
 arch/parisc/mm/fault.c     |  2 +-
 arch/powerpc/mm/fault.c    |  2 ++
 arch/riscv/mm/fault.c      |  4 ++--
 arch/s390/mm/fault.c       |  9 ++++++---
 arch/sh/mm/fault.c         |  4 ++++
 arch/sparc/mm/fault_32.c   |  3 +++
 arch/sparc/mm/fault_64.c   |  3 +++
 arch/um/kernel/trap.c      |  5 ++++-
 arch/unicore32/mm/fault.c  |  4 ++--
 arch/x86/mm/fault.c        |  6 +++++-
 arch/xtensa/mm/fault.c     |  3 +++
 23 files changed, 56 insertions(+), 34 deletions(-)

Comments

Linus Torvalds June 22, 2019, 6:02 p.m. UTC | #1 
So I still think this all *may* ok, but at a minimum some of the
comments are misleading, and we need more docs on what happens with
normal signals.

I'm picking on just the first one I noticed, but I think there were
other architectures with this too:

On Wed, Jun 19, 2019 at 7:20 PM Peter Xu <peterx@redhat.com> wrote:
>
> diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
> index 6836095251ed..3517820aea07 100644
> --- a/arch/arc/mm/fault.c
> +++ b/arch/arc/mm/fault.c
> @@ -139,17 +139,14 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
>          */
>         fault = handle_mm_fault(vma, address, flags);
>
> -       if (fatal_signal_pending(current)) {
> -
> +       if (unlikely((fault & VM_FAULT_RETRY) && signal_pending(current))) {
> +               if (fatal_signal_pending(current) && !user_mode(regs))
> +                       goto no_context;
>                 /*
>                  * if fault retry, mmap_sem already relinquished by core mm
>                  * so OK to return to user mode (with signal handled first)
>                  */
> -               if (fault & VM_FAULT_RETRY) {
> -                       if (!user_mode(regs))
> -                               goto no_context;
> -                       return;
> -               }
> +               return;
>         }

So note how the end result of this is:

 (a) if a fatal signal is pending, and we're returning to kernel mode,
we do the exception handling

 (b) otherwise, if *any* signal is pending, we'll just return and
retry the page fault

I have nothing against (a), and (b) is likely also ok, but it's worth
noting that (b) happens for kernel returns too. But the comment talks
about returning to user mode.

Is it ok to return to kernel mode when signals are pending? The signal
won't be handled, and we'll just retry the access.

Will we possibly keep retrying forever? When we take the fault again,
we'll set the FAULT_FLAG_ALLOW_RETRY again, so any fault handler that
says "if it allows retry, and signals are pending, just return" would
keep never making any progress, and we'd be stuck taking page faults
in kernel mode forever.

So I think the x86 code sequence is the much safer and more correct
one, because it will actually retry once, and set FAULT_FLAG_TRIED
(and it will clear the "FAULT_FLAG_ALLOW_RETRY" flag - but you'll
remove that clearing later in the series).

> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 46df4c6aae46..dcd7c1393be3 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1463,16 +1463,20 @@ void do_user_addr_fault(struct pt_regs *regs,
>          * that we made any progress. Handle this case first.
>          */
>         if (unlikely(fault & VM_FAULT_RETRY)) {
> +               bool is_user = flags & FAULT_FLAG_USER;
> +
>                 /* Retry at most once */
>                 if (flags & FAULT_FLAG_ALLOW_RETRY) {
>                         flags &= ~FAULT_FLAG_ALLOW_RETRY;
>                         flags |= FAULT_FLAG_TRIED;
> +                       if (is_user && signal_pending(tsk))
> +                               return;
>                         if (!fatal_signal_pending(tsk))
>                                 goto retry;
>                 }
>
>                 /* User mode? Just return to handle the fatal exception */
> -               if (flags & FAULT_FLAG_USER)
> +               if (is_user)
>                         return;
>
>                 /* Not returning to user mode? Handle exceptions or die: */

However, I think the real issue is that it just needs documentation
that a fault handler must not react to signal_pending() as part of the
fault handling itself (ie the VM_FAULT_RETRY can not be *because* of a
non-fatal signal), and there needs to be some guarantee of forward
progress.

At that point the "infinite page faults in kernel mode due to pending
signals" issue goes away. But it's not obvious in this patch, at
least.

               Linus
Peter Xu June 24, 2019, 7:42 a.m. UTC | #2 
On Sat, Jun 22, 2019 at 11:02:48AM -0700, Linus Torvalds wrote:
> So I still think this all *may* ok, but at a minimum some of the
> comments are misleading, and we need more docs on what happens with
> normal signals.
> 
> I'm picking on just the first one I noticed, but I think there were
> other architectures with this too:
> 
> On Wed, Jun 19, 2019 at 7:20 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
> > index 6836095251ed..3517820aea07 100644
> > --- a/arch/arc/mm/fault.c
> > +++ b/arch/arc/mm/fault.c
> > @@ -139,17 +139,14 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
> >          */
> >         fault = handle_mm_fault(vma, address, flags);
> >
> > -       if (fatal_signal_pending(current)) {
> > -
> > +       if (unlikely((fault & VM_FAULT_RETRY) && signal_pending(current))) {
> > +               if (fatal_signal_pending(current) && !user_mode(regs))
> > +                       goto no_context;
> >                 /*
> >                  * if fault retry, mmap_sem already relinquished by core mm
> >                  * so OK to return to user mode (with signal handled first)
> >                  */
> > -               if (fault & VM_FAULT_RETRY) {
> > -                       if (!user_mode(regs))
> > -                               goto no_context;
> > -                       return;
> > -               }
> > +               return;
> >         }
> 
> So note how the end result of this is:
> 
>  (a) if a fatal signal is pending, and we're returning to kernel mode,
> we do the exception handling
> 
>  (b) otherwise, if *any* signal is pending, we'll just return and
> retry the page fault
> 
> I have nothing against (a), and (b) is likely also ok, but it's worth
> noting that (b) happens for kernel returns too. But the comment talks
> about returning to user mode.

True.  So even with the content of this patch, I should at least touch
up the comment but I obviously missed that.  Though when reading
through the reply I think it's the patch content that might need a
fixup rather than the comment...

> 
> Is it ok to return to kernel mode when signals are pending? The signal
> won't be handled, and we'll just retry the access.
> 
> Will we possibly keep retrying forever? When we take the fault again,
> we'll set the FAULT_FLAG_ALLOW_RETRY again, so any fault handler that
> says "if it allows retry, and signals are pending, just return" would
> keep never making any progress, and we'd be stuck taking page faults
> in kernel mode forever.
> 
> So I think the x86 code sequence is the much safer and more correct
> one, because it will actually retry once, and set FAULT_FLAG_TRIED
> (and it will clear the "FAULT_FLAG_ALLOW_RETRY" flag - but you'll
> remove that clearing later in the series).

Indeed at least the ARC code has more functional change than what has
been stated in the commit message (which is only about faster signal
handling).  I wasn't paying much attention before because I don't see
"multiple retries" a big problem here and after all that's what we
finally want to achieve with the follow up patches... But I agree that
maybe I should be even more explicit in this patch.  Do you think
below change (to be squashed into this patch) looks good to you?
That's also an example only with ARC architecture but I can do similar
things to the other archs if you prefer:

                /*
                 * if fault retry, mmap_sem already relinquished by core mm
                 * so OK to return to user mode (with signal handled first)
                 */
-               return;
+               if (user_mode(regs))
+                       return;

> 
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 46df4c6aae46..dcd7c1393be3 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1463,16 +1463,20 @@ void do_user_addr_fault(struct pt_regs *regs,
> >          * that we made any progress. Handle this case first.
> >          */
> >         if (unlikely(fault & VM_FAULT_RETRY)) {
> > +               bool is_user = flags & FAULT_FLAG_USER;
> > +
> >                 /* Retry at most once */
> >                 if (flags & FAULT_FLAG_ALLOW_RETRY) {
> >                         flags &= ~FAULT_FLAG_ALLOW_RETRY;
> >                         flags |= FAULT_FLAG_TRIED;
> > +                       if (is_user && signal_pending(tsk))
> > +                               return;
> >                         if (!fatal_signal_pending(tsk))
> >                                 goto retry;
> >                 }
> >
> >                 /* User mode? Just return to handle the fatal exception */
> > -               if (flags & FAULT_FLAG_USER)
> > +               if (is_user)
> >                         return;
> >
> >                 /* Not returning to user mode? Handle exceptions or die: */
> 
> However, I think the real issue is that it just needs documentation
> that a fault handler must not react to signal_pending() as part of the
> fault handling itself (ie the VM_FAULT_RETRY can not be *because* of a
> non-fatal signal), and there needs to be some guarantee of forward
> progress.

Should we still be able to react on signal_pending() as part of fault
handling (because that's what this patch wants to do, at least for an
user-mode page fault)?  Please kindly correct me if I misunderstood...

> 
> At that point the "infinite page faults in kernel mode due to pending
> signals" issue goes away. But it's not obvious in this patch, at
> least.

Thanks,
Linus Torvalds June 24, 2019, 1:31 p.m. UTC | #3 
On Mon, Jun 24, 2019 at 3:43 PM Peter Xu <peterx@redhat.com> wrote:
>
> Should we still be able to react on signal_pending() as part of fault
> handling (because that's what this patch wants to do, at least for an
> user-mode page fault)?  Please kindly correct me if I misunderstood...

I think that with this patch (modulo possible fix-ups) then yes, as
long as we're returning to user mode we can do signal_pending() and
return RETRY.

But I think we really want to add a new FAULT_FLAG_INTERRUPTIBLE bit
for that (the same way we already have FAULT_FLAG_KILLABLE for things
that can react to fatal signals), and only do it when that is set.
Then the page fault handler can set that flag when it's doing a
user-mode page fault.

Does that sound reasonable?

               Linus
Peter Xu June 25, 2019, 5:30 a.m. UTC | #4 
On Mon, Jun 24, 2019 at 09:31:42PM +0800, Linus Torvalds wrote:
> On Mon, Jun 24, 2019 at 3:43 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Should we still be able to react on signal_pending() as part of fault
> > handling (because that's what this patch wants to do, at least for an
> > user-mode page fault)?  Please kindly correct me if I misunderstood...
> 
> I think that with this patch (modulo possible fix-ups) then yes, as
> long as we're returning to user mode we can do signal_pending() and
> return RETRY.
> 
> But I think we really want to add a new FAULT_FLAG_INTERRUPTIBLE bit
> for that (the same way we already have FAULT_FLAG_KILLABLE for things
> that can react to fatal signals), and only do it when that is set.
> Then the page fault handler can set that flag when it's doing a
> user-mode page fault.
> 
> Does that sound reasonable?

Yes that sounds reasonable to me, and that matches perfectly with
TASK_INTERRUPTIBLE and TASK_KILLABLE.  The only thing that I am a bit
uncertain is whether we should define FAULT_FLAG_INTERRUPTIBLE as a
new bit or make it simply a combination of:

  FAULT_FLAG_KILLABLE | FAULT_FLAG_USER

The problem is that when we do set_current_state() with either
TASK_INTERRUPTIBLE or TASK_KILLABLE we'll only choose one of them, but
never both.  Here since the fault flag is a bitmask then if we
introduce a new FAULT_FLAG_INTERRUPTIBLE bit and use it in the fault
flags then we should probably be sure that FAULT_FLAG_KILLABLE is also
set when with that (since IMHO it won't make much sense to make a page
fault "interruptable" but "un-killable"...).  Considering that
TASK_INTERRUPTIBLE should also always in user-mode page faults so this
dependency seems to exist with FAULT_FLAG_USER.  Then I'm thinking
maybe using the combination to express the meaning that "we would like
this page fault to be interruptable, even for general userspace
signals" would be nicer?

AFAIK currently only handle_userfault() have such code to handle
normal signals besides SIGKILL, and it was trying to detect this using
this rule already:

	return_to_userland =
		(vmf->flags & (FAULT_FLAG_USER|FAULT_FLAG_KILLABLE)) ==
		(FAULT_FLAG_USER|FAULT_FLAG_KILLABLE);

Then if we define that globally and officially then we can probably
replace this simply with:

	return_to_userland = vmf->flags & FAULT_FLAG_INTERRUPTIBLE;

What do you think?

Thanks,
Linus Torvalds June 26, 2019, 1:59 a.m. UTC | #5 
On Tue, Jun 25, 2019 at 1:31 PM Peter Xu <peterx@redhat.com> wrote:
>
> Yes that sounds reasonable to me, and that matches perfectly with
> TASK_INTERRUPTIBLE and TASK_KILLABLE.  The only thing that I am a bit
> uncertain is whether we should define FAULT_FLAG_INTERRUPTIBLE as a
> new bit or make it simply a combination of:
>
>   FAULT_FLAG_KILLABLE | FAULT_FLAG_USER

It needs to be a new bit, I think.

Some things could potentially care about the difference between "can I
abort this thing because the task will *die* and never see the end
result" and "can I abort this thing because it will be retried".

For a regular page fault, maybe FAULT_FLAG_INTERRUPTBLE will always be
set for the same things that set FAULT_FLAG_KILLABLE when it happens
from user mode, but at least conceptually I think they are different,
and it could make a difference for things like get_user_pages() or
similar.

Also, I actually don't think we should ever expose FAULT_FLAG_USER to
any fault handlers anyway. It has a very specific meaning for memory
cgroup handling, and no other fault handler should likely ever care
about "was this a user fault". So I'd actually prefer for people to
ignore and forget that hacky flag entirely, rather than give it subtle
semantic meaning together with KILLABLE.

[ Side note: this is the point where I may soon lose internet access,
so I'll probably not be able to participate in the discussion any more
for a while ]

             Linus
Peter Xu June 26, 2019, 7:43 a.m. UTC | #6 
On Wed, Jun 26, 2019 at 09:59:58AM +0800, Linus Torvalds wrote:
> On Tue, Jun 25, 2019 at 1:31 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Yes that sounds reasonable to me, and that matches perfectly with
> > TASK_INTERRUPTIBLE and TASK_KILLABLE.  The only thing that I am a bit
> > uncertain is whether we should define FAULT_FLAG_INTERRUPTIBLE as a
> > new bit or make it simply a combination of:
> >
> >   FAULT_FLAG_KILLABLE | FAULT_FLAG_USER
> 
> It needs to be a new bit, I think.
> 
> Some things could potentially care about the difference between "can I
> abort this thing because the task will *die* and never see the end
> result" and "can I abort this thing because it will be retried".
> 
> For a regular page fault, maybe FAULT_FLAG_INTERRUPTBLE will always be
> set for the same things that set FAULT_FLAG_KILLABLE when it happens
> from user mode, but at least conceptually I think they are different,
> and it could make a difference for things like get_user_pages() or
> similar.
> 
> Also, I actually don't think we should ever expose FAULT_FLAG_USER to
> any fault handlers anyway. It has a very specific meaning for memory
> cgroup handling, and no other fault handler should likely ever care
> about "was this a user fault". So I'd actually prefer for people to
> ignore and forget that hacky flag entirely, rather than give it subtle
> semantic meaning together with KILLABLE.

OK.

> 
> [ Side note: this is the point where I may soon lose internet access,
> so I'll probably not be able to participate in the discussion any more
> for a while ]

Appreciate for these suggestions.  I'll prepare something with that
new bit and see whether that could be accepted.  I'll also try to
split those out of the bigger series.

Thanks,
diff mbox series

Patch

diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c
index 188fc9256baf..8a2ef90b4bfc 100644
--- a/arch/alpha/mm/fault.c
+++ b/arch/alpha/mm/fault.c
@@ -150,7 +150,7 @@  do_page_fault(unsigned long address, unsigned long mmcsr,
 	   the fault.  */
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
index 6836095251ed..3517820aea07 100644
--- a/arch/arc/mm/fault.c
+++ b/arch/arc/mm/fault.c
@@ -139,17 +139,14 @@  void do_page_fault(unsigned long address, struct pt_regs *regs)
 	 */
 	fault = handle_mm_fault(vma, address, flags);
 
-	if (fatal_signal_pending(current)) {
-
+	if (unlikely((fault & VM_FAULT_RETRY) && signal_pending(current))) {
+		if (fatal_signal_pending(current) && !user_mode(regs))
+			goto no_context;
 		/*
 		 * if fault retry, mmap_sem already relinquished by core mm
 		 * so OK to return to user mode (with signal handled first)
 		 */
-		if (fault & VM_FAULT_RETRY) {
-			if (!user_mode(regs))
-				goto no_context;
-			return;
-		}
+		return;
 	}
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 58f69fa07df9..c41c021bbe40 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -314,12 +314,12 @@  do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 
 	fault = __do_page_fault(mm, addr, fsr, flags, tsk);
 
-	/* If we need to retry but a fatal signal is pending, handle the
+	/* If we need to retry but a signal is pending, handle the
 	 * signal first. We do not need to release the mmap_sem because
 	 * it would already be released in __lock_page_or_retry in
 	 * mm/filemap.c. */
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) {
-		if (!user_mode(regs))
+	if (unlikely(fault & VM_FAULT_RETRY && signal_pending(current))) {
+		if (fatal_signal_pending(current) && !user_mode(regs))
 			goto no_context;
 		return 0;
 	}
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index a30818ed9c60..890ec3a693e6 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -513,13 +513,13 @@  static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 
 	if (fault & VM_FAULT_RETRY) {
 		/*
-		 * If we need to retry but a fatal signal is pending,
+		 * If we need to retry but a signal is pending,
 		 * handle the signal first. We do not need to release
 		 * the mmap_sem because it would already be released
 		 * in __lock_page_or_retry in mm/filemap.c.
 		 */
-		if (fatal_signal_pending(current)) {
-			if (!user_mode(regs))
+		if (signal_pending(current)) {
+			if (fatal_signal_pending(current) && !user_mode(regs))
 				goto no_context;
 			return 0;
 		}
diff --git a/arch/hexagon/mm/vm_fault.c b/arch/hexagon/mm/vm_fault.c
index b7a99aa5b0ba..febb4f96ba6f 100644
--- a/arch/hexagon/mm/vm_fault.c
+++ b/arch/hexagon/mm/vm_fault.c
@@ -91,7 +91,7 @@  void do_page_fault(unsigned long address, long cause, struct pt_regs *regs)
 
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	/* The most common case -- we are done. */
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 5baeb022f474..62c2d39d2bed 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -163,7 +163,7 @@  ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
 	 */
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/m68k/mm/fault.c b/arch/m68k/mm/fault.c
index 9b6163c05a75..d9808a807ab8 100644
--- a/arch/m68k/mm/fault.c
+++ b/arch/m68k/mm/fault.c
@@ -138,7 +138,7 @@  int do_page_fault(struct pt_regs *regs, unsigned long address,
 	fault = handle_mm_fault(vma, address, flags);
 	pr_debug("handle_mm_fault returns %x\n", fault);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return 0;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/microblaze/mm/fault.c b/arch/microblaze/mm/fault.c
index 202ad6a494f5..4fd2dbd0c5ca 100644
--- a/arch/microblaze/mm/fault.c
+++ b/arch/microblaze/mm/fault.c
@@ -217,7 +217,7 @@  void do_page_fault(struct pt_regs *regs, unsigned long address,
 	 */
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index 73d8a0f0b810..92374fd091d2 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -154,7 +154,7 @@  static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
 	 */
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
diff --git a/arch/nds32/mm/fault.c b/arch/nds32/mm/fault.c
index 68d5f2a27f38..da777de8a62e 100644
--- a/arch/nds32/mm/fault.c
+++ b/arch/nds32/mm/fault.c
@@ -206,12 +206,12 @@  void do_page_fault(unsigned long entry, unsigned long addr,
 	fault = handle_mm_fault(vma, addr, flags);
 
 	/*
-	 * If we need to retry but a fatal signal is pending, handle the
+	 * If we need to retry but a signal is pending, handle the
 	 * signal first. We do not need to release the mmap_sem because it
 	 * would already be released in __lock_page_or_retry in mm/filemap.c.
 	 */
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) {
-		if (!user_mode(regs))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current)) {
+		if (fatal_signal_pending(current) && !user_mode(regs))
 			goto no_context;
 		return;
 	}
diff --git a/arch/nios2/mm/fault.c b/arch/nios2/mm/fault.c
index 6a2e716b959f..bdb1f9db75ba 100644
--- a/arch/nios2/mm/fault.c
+++ b/arch/nios2/mm/fault.c
@@ -133,7 +133,7 @@  asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long cause,
 	 */
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/openrisc/mm/fault.c b/arch/openrisc/mm/fault.c
index 9eee5bf3db27..f9f47dc32f94 100644
--- a/arch/openrisc/mm/fault.c
+++ b/arch/openrisc/mm/fault.c
@@ -161,7 +161,7 @@  asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address,
 
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
index c8e8b7c05558..29422eec329d 100644
--- a/arch/parisc/mm/fault.c
+++ b/arch/parisc/mm/fault.c
@@ -303,7 +303,7 @@  void do_page_fault(struct pt_regs *regs, unsigned long code,
 
 	fault = handle_mm_fault(vma, address, flags);
 
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index ec6b7ad70659..c2168b298c82 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -616,6 +616,8 @@  static int __do_page_fault(struct pt_regs *regs, unsigned long address,
 			 */
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
 			flags |= FAULT_FLAG_TRIED;
+			if (is_user && signal_pending(current))
+				return 0;
 			if (!fatal_signal_pending(current))
 				goto retry;
 		}
diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
index 3e2708c626a8..4aa7a2343353 100644
--- a/arch/riscv/mm/fault.c
+++ b/arch/riscv/mm/fault.c
@@ -111,11 +111,11 @@  asmlinkage void do_page_fault(struct pt_regs *regs)
 	fault = handle_mm_fault(vma, addr, flags);
 
 	/*
-	 * If we need to retry but a fatal signal is pending, handle the
+	 * If we need to retry but a signal is pending, handle the
 	 * signal first. We do not need to release the mmap_sem because it
 	 * would already be released in __lock_page_or_retry in mm/filemap.c.
 	 */
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(tsk))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(tsk))
 		return;
 
 	if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index df75d574246d..94087ba285be 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -493,9 +493,12 @@  static inline vm_fault_t do_exception(struct pt_regs *regs, int access)
 	 * the fault.
 	 */
 	fault = handle_mm_fault(vma, address, flags);
-	/* No reason to continue if interrupted by SIGKILL. */
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) {
-		fault = VM_FAULT_SIGNAL;
+	/* Do not continue if interrupted by signals. */
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current)) {
+		if (fatal_signal_pending(current))
+			fault = VM_FAULT_SIGNAL;
+		else
+			fault = 0;
 		if (flags & FAULT_FLAG_RETRY_NOWAIT)
 			goto out_up;
 		goto out;
diff --git a/arch/sh/mm/fault.c b/arch/sh/mm/fault.c
index 6defd2c6d9b1..baf5d73df40c 100644
--- a/arch/sh/mm/fault.c
+++ b/arch/sh/mm/fault.c
@@ -506,6 +506,10 @@  asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
 			 * have already released it in __lock_page_or_retry
 			 * in mm/filemap.c.
 			 */
+
+			if (user_mode(regs) && signal_pending(tsk))
+				return;
+
 			goto retry;
 		}
 	}
diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
index b0440b0edd97..a2c83104fe35 100644
--- a/arch/sparc/mm/fault_32.c
+++ b/arch/sparc/mm/fault_32.c
@@ -269,6 +269,9 @@  asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
 			 * in mm/filemap.c.
 			 */
 
+			if (user_mode(regs) && signal_pending(tsk))
+				return;
+
 			goto retry;
 		}
 	}
diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c
index 8f8a604c1300..cad71ec5c7b3 100644
--- a/arch/sparc/mm/fault_64.c
+++ b/arch/sparc/mm/fault_64.c
@@ -467,6 +467,9 @@  asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
 			 * in mm/filemap.c.
 			 */
 
+			if (user_mode(regs) && signal_pending(current))
+				return;
+
 			goto retry;
 		}
 	}
diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index 0e8b6158f224..05dcd4c5f0d5 100644
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -76,8 +76,11 @@  int handle_page_fault(unsigned long address, unsigned long ip,
 
 		fault = handle_mm_fault(vma, address, flags);
 
-		if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+		if ((fault & VM_FAULT_RETRY) && signal_pending(current)) {
+			if (is_user && !fatal_signal_pending(current))
+				err = 0;
 			goto out_nosemaphore;
+		}
 
 		if (unlikely(fault & VM_FAULT_ERROR)) {
 			if (fault & VM_FAULT_OOM) {
diff --git a/arch/unicore32/mm/fault.c b/arch/unicore32/mm/fault.c
index b9a3a50644c1..3611f19234a1 100644
--- a/arch/unicore32/mm/fault.c
+++ b/arch/unicore32/mm/fault.c
@@ -248,11 +248,11 @@  static int do_pf(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 
 	fault = __do_pf(mm, addr, fsr, flags, tsk);
 
-	/* If we need to retry but a fatal signal is pending, handle the
+	/* If we need to retry but a signal is pending, handle the
 	 * signal first. We do not need to release the mmap_sem because
 	 * it would already be released in __lock_page_or_retry in
 	 * mm/filemap.c. */
-	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+	if ((fault & VM_FAULT_RETRY) && signal_pending(current))
 		return 0;
 
 	if (!(fault & VM_FAULT_ERROR) && (flags & FAULT_FLAG_ALLOW_RETRY)) {
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 46df4c6aae46..dcd7c1393be3 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1463,16 +1463,20 @@  void do_user_addr_fault(struct pt_regs *regs,
 	 * that we made any progress. Handle this case first.
 	 */
 	if (unlikely(fault & VM_FAULT_RETRY)) {
+		bool is_user = flags & FAULT_FLAG_USER;
+
 		/* Retry at most once */
 		if (flags & FAULT_FLAG_ALLOW_RETRY) {
 			flags &= ~FAULT_FLAG_ALLOW_RETRY;
 			flags |= FAULT_FLAG_TRIED;
+			if (is_user && signal_pending(tsk))
+				return;
 			if (!fatal_signal_pending(tsk))
 				goto retry;
 		}
 
 		/* User mode? Just return to handle the fatal exception */
-		if (flags & FAULT_FLAG_USER)
+		if (is_user)
 			return;
 
 		/* Not returning to user mode? Handle exceptions or die: */
diff --git a/arch/xtensa/mm/fault.c b/arch/xtensa/mm/fault.c
index 2ab0e0dcd166..792dad5e2f12 100644
--- a/arch/xtensa/mm/fault.c
+++ b/arch/xtensa/mm/fault.c
@@ -136,6 +136,9 @@  void do_page_fault(struct pt_regs *regs)
 			 * in mm/filemap.c.
 			 */
 
+			if (user_mode(regs) && signal_pending(current))
+				return;
+
 			goto retry;
 		}
 	}