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

[Peter Xu]

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@xxxxxxxxxx> 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,

-- 
Peter Xu