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

On Wed, May 17, 2023 at 11:15:14AM -0700, Linus Torvalds wrote:

On Wed, May 17, 2023 at 10:22 AM Beau Belgrave
[off-list ref] wrote:

quoted

On Tue, May 16, 2023 at 08:03:09PM -0700, Linus Torvalds wrote:

quoted

So what is it that could even race and change the list that is the
cause of that rcu-ness?

Processes that fork() with previous user_events need to be duplicated.

BS.

Really. Stop making stuff up.

The above statement is clearly not true - just LOOK AT THE CODE.

user_event_mm_dup() puts a new mm into the global list before the
enablers list is fully populated. As it stands now, since it's in the
global list, it can get enumerated in a small timing window via the
tracing subsystem register callbacks when someone enables the event via
ftrace/perf.

Here's the loop in question:

                list_for_each_entry_rcu(enabler, &mm->enablers, link) {
                        if (enabler->event == user) {
                                attempt = 0;
                                user_event_enabler_write(mm, enabler,
true, &attempt);
                        }
                }

and AT THE VERY TOP OF user_event_enabler_write() we have this:

        lockdep_assert_held(&event_mutex);

so either nobody has ever tested this code with lockdep enabled, or we
hold that lock.

And if nobody has ever tested the code, then it's broken anyway. That
code N#EEDS the mutex lock. It needs to stop thinking it's RCU-safe,
when it clearly isn't.

So I ask again: why is that code using RCU list traversal, when it
already holds the lock that makes the RCU'ness COMPLETELY POINTLESS.

And again, that pointless RCU locking around this all seems to be the
*only* reason for all these issues with pin_user_pages_remote().

So I claim that this code is garbage.  Somebody didn't think about locking.

Now, it's true that during fork, we have *another* RCU loop, but that
one is harmless: that's not the one that does all this page pinning.

Now, that one *does* do

        list_add_rcu(&enabler->link, &mm->enablers);

without actually holding any locks, but in this case 'mm' is a newly
allocated private thing of a task that hasn't even been exposed to the
world yet, so nobody should be able to even see it. So that code lacks
the proper locking for the new list, but it does so because there is
nothing that can race with the new list (and the old list is
read-only, so RCU traversal of the old list works).

Well, that's the problem I was trying to point out. The fork path calls
user_event_mm_dup() -> user_event_mm_create(), which DO expose this to
the trace world. I definitely need to fix that, then I can drop these RCU
paths in the enablers. It has been exposed out to the tracing tracepoint
paths, but it has yet to be exposed out to the newly forked process that
could cause data writes, add new events, disable them, etc.

So that "list_add_rcu()" there could probably be just a "list_add()",
with a comment saying "this is new, nobody can see it".

Yes, after I fix the ordering of the mm add to the tracing global list.
That is clearly something I should have done originally and caused
confusion and extra RCU usage that is unneeded.

And if something *can* race it it and can see the new list, then it
had damn well needs that mutex lock anyway, because that "something"
could be actually modifying it. But that's separate from the page
pinning situation.

So again, I claim that the RCU'ness of the pin_user_pages part is
broken and should simply not exist.

quoted

quoted

Other code in that file happily just does

        mutex_lock(&event_mutex);

        list_for_each_entry_safe(enabler, next, &mm->enablers, link)

with no RCU anywhere. Why does user_event_enabler_update() not do that?

This is due to the fork() case above without taking the event_mutex.

See above. Your thinking is confused, and the code is broken.

If somebody can see the new list while it is created during fork(),
then you need the event_mutex to protect the creation of it.

And if nobody can see it, then you don't need any RCU protection against it.

Those are the two choices. You can't have it both ways.

quoted

quoted

Oh, and even those other loops are a bit strange. Why do they use the
"_safe" variant, even when they just traverse the list without
changing it? Look at user_event_enabler_exists(), for example.

The other places in the code that do this either will remove the event
depending on the situation during the for_each, or they only hold the
register lock and don't hold the event_mutex.

So?

That "safe" variant doesn't imply any locking. It does *not* protect
against events being removed. It *purely* protects against the loop
itself removing entries.

So this code:

        list_for_each_entry_safe(enabler, next, &mm->enablers, link) {
                if (enabler->addr == uaddr &&
                    (enabler->values & ENABLE_VAL_BIT_MASK) == bit)
                        return true;
        }

is simply nonsensical. There is no reason for the "safe". It does not
make anything safer.

The above loop is only safe under the mutex (it would need to be the
"rcu" variant to be safe to traverse without locking), and since it
isn't modifying the list, there's no reason for the safe.

End result: the "safe" part is simply wrong.

Got it, I was confused.

If the intention is "rcu" because of lack of locking, then the code needs to
 (a) get the rcu read lock
 (b) use the _rcu variant of the list traversal

And if the intention is that it's done under the proper 'event_mutex'
lock, then the "safe" part should simply be dropped.

               Linus

Thanks,
-Beau