Re: [External] Re: [RFC PATCH] x86/fault: move might_sleep() out of mmap read lock

Muchun Song
2021-11-26T05:57:05Z

On Thu, Nov 25, 2021 at 6:45 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Muchun, Dave!
>
> On Mon, Nov 22 2021 at 14:59, Muchun Song wrote:
> > On Fri, Nov 19, 2021 at 11:04 PM Dave Hansen <dave.hansen@intel.com> wrote:
> >> >
> >> > + might_sleep();
> >> > +
> >> > /*
> >> > * Kernel-mode access to the user address space should only occur
> >> > * on well-defined single instructions listed in the exception
> >> > @@ -1346,13 +1348,6 @@ void do_user_addr_fault(struct pt_regs *regs,
> >> > }
> >> > retry:
> >> > mmap_read_lock(mm);
> >> > - } else {
> >> > - /*
> >> > - * The above down_read_trylock() might have succeeded in
> >> > - * which case we'll have missed the might_sleep() from
> >> > - * down_read():
> >> > - */
> >> > - might_sleep();
> >> > }
> >> >
> >> > vma = find_vma(mm, address);
> >>
> >> The comment is stale, which isn't great. The might_sleep() is already
> >> in the fast path. So, moving it up above makes a lot of sense just in
> >> terms of simplicity.
>
> I don't think so. The point is:
>
> if (unlikely(!mmap_read_trylock(mm))) {
> if (!user_mode(regs) && !search_exception_tables(regs->ip)) {
> /*
> * Fault from code in kernel from
> * which we do not expect faults.
> */
> bad_area_nosemaphore(regs, error_code, address);
> return;
> }
>
> Moving it up will make the might_sleep() splat more important than an
> unexpected fault when the unexpected fault happens in e.g. a preemption
> disabled region. That's wrong because the important information in this
> case is not the might sleep splat. The important information is the
> fault itself.
>
> But moving it up is even more wrong for spurious faults which are
> correctly handled in that case via:
>
> bad_area_nosemaphore()
> __bad_area_nosemaphore()
> kernelmode_fixup_or_oops()
> handle(AMD erratum #91)
> is_prefetch()
>
> So if such a spurious fault happens in a condition which would trigger
> the might_sleep() splat then moving might_sleep() before the trylock()
> will cause false positives. So, no. It's going to stay where it is.

Got it. I didn't realize those cases. Thank you for your patient
explanation.

>
> > Without this patch, I didn't see the might_sleep() in the fast path. What
> > am I missing here?
>
> I have no idea what you are doing. If the trylock() succeeds and the
> fault happened in e.g. a preemption disabled region then the
> might_sleep() in the else path will trigger no matter what.
>
> Thanks,
>
> tglx