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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 May 2023 16:00:54 -0700
From:   Beau Belgrave <beaub@...ux.microsoft.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-trace-kernel@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        David Vernet <void@...ifault.com>, dthaler@...rosoft.com,
        brauner@...nel.org, hch@...radead.org
Subject: Re: [PATCH] tracing/user_events: Run BPF program if attached

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@...ux-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);