On Mon, May 15, 2023 at 02:33:05PM -0400, Steven Rostedt wrote:
> On Mon, 15 May 2023 09:57:07 -0700
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
>
> > Thank you for these details. Answer below...
>
> Thanks for this well thought out reply!
>
>
[...]
> >
> > > if (unlikely(ret <= 0)) {
> > > if (!fixup_fault)
> > > return -EFAULT;
> > >
> > > if (!user_event_enabler_queue_fault(mm, enabler, *attempt))
> > > pr_warn("user_events: Unable to queue fault handler\n");
> >
> > This part looks questionable.
> >
> > The only users of fixup_user_fault() were futex and KVM.
> > Now user_events are calling it too from user_event_mm_fault_in() where
> > "bool unlocked;" is uninitialized and state of this flag is not checked
> > after fixup_user_fault() call.
> > Not an MM expert, but this is suspicious.
>
> Hmm, yeah, this should be:
>
> static int user_event_mm_fault_in()
> {
> bool unlocked = false;
>
> [..]
>
> out:
> if (!unlocked)
> mmap_read_unlock(mm->mm);
> }
>
> Good catch!
>
I don't believe that's correct. fixup_user_fault() re-acquires the
mmap lock, and when it does, it lets you know via unlocked getting set
to true. IE: Something COULD have changed in the mmap during this call,
but the lock is still held.
See comments here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/gup.c#n1287
Thanks,
-Beau
>
> Thank you Alexei for asking these. The above are all valid concerns.
>
> -- Steve
>
