On Wed, Feb 01, 2023 at 02:48:22PM -0500, Peter Xu wrote:
> I do also see a common pattern of the possibility to have a generic fault
> handler like generic_page_fault().
>
> It probably should start with taking the mmap_sem until providing some
> retval that is much easier to digest further by the arch-dependent code, so
> it can directly do something rather than parsing the bitmask in a
> duplicated way (hence the new retval should hopefully not a bitmask anymore
> but a "what to do").
>
> Maybe it can be something like:
>
> /**
> * enum page_fault_retval - Higher level fault retval, generalized from
> * vm_fault_reason above that is only used by hardware page fault handlers.
> * It generalizes the bitmask-versioned retval into something that the arch
> * dependent code should react upon.
> *
> * @PF_RET_COMPLETED: The page fault is completed successfully
> * @PF_RET_BAD_AREA: The page fault address falls in a bad area
> * (e.g., vma not found, expand_stack() fails..)
FWIW, there's a fun discrepancy - VM_FAULT_SIGSEGV may yield SEGV_MAPERR
or SEGV_ACCERR; depends upon the architecture. Not that there'd been
many places that return VM_FAULT_SIGSEGV these days... Good thing, too,
since otherwise e.g. csky would oops...
> * @PF_RET_ACCESS_ERR: The page fault has access errors
> * (e.g., write fault on !VM_WRITE vmas)
> * @PF_RET_KERN_FIXUP: The page fault requires kernel fixups
> * (e.g., during copy_to_user() but fault failed?)
> * @PF_RET_HWPOISON: The page fault encountered poisoned pages
> * @PF_RET_SIGNAL: The page fault encountered poisoned pages
??
> * ...
> */
> enum page_fault_retval {
> PF_RET_DONE = 0,
> PF_RET_BAD_AREA,
> PF_RET_ACCESS_ERR,
> PF_RET_KERN_FIXUP,
> PF_RET_HWPOISON,
> PF_RET_SIGNAL,
> ...
> };
>
> As a start we may still want to return some more information (perhaps still
> the vm_fault_t alongside? Or another union that will provide different
> information based on different PF_RET_*). One major thing is I see how we
> handle VM_FAULT_HWPOISON and also the fact that we encode something more
> into the bitmask on page sizes (VM_FAULT_HINDEX_MASK).
>
> So the generic helper could, hopefully, hide the complexity of:
>
> - Taking and releasing of mmap lock
> - find_vma(), and also relevant checks on access or stack handling
Umm... arm is a bit special here:
if (addr < FIRST_USER_ADDRESS)
return VM_FAULT_BADMAP;
with no counterparts elsewhere.
> - handle_mm_fault() itself (of course...)
> - detect signals
> - handle page fault retries (so, in the new layer of retval there should
> have nothing telling it to retry; it should always be the ultimate result)
agreed.
- unlock mmap; don't leave that to caller.
> - parse different errors into "what the arch code should do", and
> generalize the common ones, e.g.
> - OOM, do pagefault_out_of_memory() for user-mode
> - VM_FAULT_SIGSEGV, which should be able to merge into PF_RET_BAD_AREA?
> - ...
AFAICS, all errors in kernel mode => no_context.
> It'll simplify things if we can unify some small details like whether the
> -EFAULT above should contain a sigbus.
>
> A trivial detail I found when I was looking at this is, x86_64 passes in
> different signals to kernelmode_fixup_or_oops() - in do_user_addr_fault()
> there're three call sites and each of them pass over a differerent signal.
> IIUC that will only make a difference if there's a nested page fault during
> the vsyscall emulation (but I may be wrong too because I'm new to this
> code), and I have no idea when it'll happen and whether that needs to be
> strictly followed.
>From my (very incomplete so far) dig through that pile:
Q: do we still have the cases when handle_mm_fault() does
not return any of VM_FAULT_COMPLETED | VM_FAULT_RETRY | VM_FAULT_ERROR?
That gets treated as unlock + VM_FAULT_COMPLETED, but do we still need
that?
Q: can VM_FAULT_RETRY be mixed with anything in VM_FAULT_ERROR?
What locking, if that happens?
* details of storing the fault details (for ptrace, mostly)
vary a lot; no chance to unify, AFAICS.
* requirements for vma flags also differ; e.g. read fault on
alpha is explicitly OK with absence of VM_READ if VM_WRITE is there.
Probably should go by way of arm and pass the mask that must
have non-empty intersection with vma->vm_flags? Because *that*
is very likely to be a part of ABI - mmap(2) callers that rely
upon the flags being OK for given architecture are quite possible.
* mmap lock is also quite variable in how it's taken;
x86 and arm have fun dance with trylock/search for exception handler/etc.
Other architectures do not; OTOH, there's a prefetch stuck in itanic
variant, with comment about mmap_sem being performance-critical...
* logics for stack expansion includes this twist:
if (!(vma->vm_flags & VM_GROWSDOWN))
goto map_err;
if (user_mode(regs)) {
/* Accessing the stack below usp is always a bug. The
"+ 256" is there due to some instructions doing
pre-decrement on the stack and that doesn't show up
until later. */
if (address + 256 < rdusp())
goto map_err;
}
if (expand_stack(vma, address))
goto map_err;
That's m68k; ISTR similar considerations elsewhere, but I could be
wrong.
