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

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Peter Xu <peterx@redhat.com>
Cc: Linux-MM <linux-mm@kvack.org>,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	David Hildenbrand <david@redhat.com>,
	Hugh Dickins <hughd@google.com>, Maya Gokhale <gokhale2@llnl.gov>,
	Jerome Glisse <jglisse@redhat.com>,
	Pavel Emelyanov <xemul@virtuozzo.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Martin Cracauer <cracauer@cons.org>,
	Denis Plotnikov <dplotnikov@virtuozzo.com>,
	Shaohua Li <shli@fb.com>, Andrea Arcangeli <aarcange@redhat.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Marty McFadden <mcfadden8@llnl.gov>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	Mel Gorman <mgorman@suse.de>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v5 02/25] mm: userfault: return VM_FAULT_RETRY on signals
Date: Sat, 22 Jun 2019 11:02:48 -0700	[thread overview]
Message-ID: <CAHk-=wiGphH2UL+To5rASyFoCk6=9bROUkGDWSa_rMu9Kgb0yw@mail.gmail.com> (raw)
In-Reply-To: <20190620022008.19172-3-peterx@redhat.com>

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