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

[Peter Xu <peterx@redhat.com>]

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,

-- 
Peter Xu