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

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

On Wed, May 17, 2023 at 12:37:11PM -0700, Linus Torvalds wrote:
> On Wed, May 17, 2023 at 12:36 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > .. this is the patch that I think should go on top of it to fix the
> > misleading "safe" and the incorrect RCU walk.
> 
> Let's actually attach the patch too. Duh.
> 
>                Linus

>  kernel/trace/trace_events_user.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index b2aecbfbbd24..054e28cc5ad4 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -439,7 +439,7 @@ static bool user_event_enabler_exists(struct user_event_mm *mm,
>  	struct user_event_enabler *enabler;
>  	struct user_event_enabler *next;
>  
> -	list_for_each_entry_safe(enabler, next, &mm->enablers, link) {
> +	list_for_each_entry(enabler, next, &mm->enablers, link) {
>  		if (enabler->addr == uaddr &&
>  		    (enabler->values & ENABLE_VAL_BIT_MASK) == bit)
>  			return true;
> @@ -455,19 +455,19 @@ static void user_event_enabler_update(struct user_event *user)
>  	struct user_event_mm *next;
>  	int attempt;
>  
> +	lockdep_assert_held(&event_mutex);
> +
>  	while (mm) {
>  		next = mm->next;
>  		mmap_read_lock(mm->mm);
> -		rcu_read_lock();
>  
> -		list_for_each_entry_rcu(enabler, &mm->enablers, link) {
> +		list_for_each_entry(enabler, &mm->enablers, link) {
>  			if (enabler->event == user) {
>  				attempt = 0;
>  				user_event_enabler_write(mm, enabler, true, &attempt);
>  			}
>  		}
>  
> -		rcu_read_unlock();
>  		mmap_read_unlock(mm->mm);
>  		user_event_mm_put(mm);
>  		mm = next;

Do you mind giving me your Signed-off-by for these?

I plan to do a series where I take these patches and then also fix up a
few comments and the link namings as you suggested.

First patch is clean, second patch I made the following changes and
after that passed all the self-tests without bug splats with
CONFIG_PROVE_LOCKING/RCU and ATOMIC_SLEEP:

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index b2aecbfbbd24..2f70dabb0f71 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -437,9 +437,8 @@ static bool user_event_enabler_exists(struct user_event_mm *mm,
                                      unsigned long uaddr, unsigned char bit)
 {
        struct user_event_enabler *enabler;
-       struct user_event_enabler *next;

-       list_for_each_entry_safe(enabler, next, &mm->enablers, link) {
+       list_for_each_entry(enabler, &mm->enablers, link) {
                if (enabler->addr == uaddr &&
                    (enabler->values & ENABLE_VAL_BIT_MASK) == bit)
                        return true;
@@ -495,7 +494,9 @@ static bool user_event_enabler_dup(struct user_event_enabler *orig,
        enabler->values = orig->values & ENABLE_VAL_DUP_MASK;

        refcount_inc(&enabler->event->refcnt);
-       list_add_rcu(&enabler->link, &mm->enablers);
+
+       /* Enablers not exposed yet, RCU not required */
+       list_add(&enabler->link, &mm->enablers);