Re: [PATCH] tracing/user_events: Run BPF program if attached

[Beau Belgrave <beaub@linux.microsoft.com>]

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@gmail.com> 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
>