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

On Mon, May 15, 2023 at 02:33:05PM -0400, Steven Rostedt wrote:

On Mon, 15 May 2023 09:57:07 -0700
Alexei Starovoitov [off-list ref] wrote:

quoted

Thank you for these details. Answer below...

Thanks for this well thought out reply!

[...]

quoted

quoted

	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