[GIT PULL] tracing: Two event pid filtering bug fixes

[GIT PULL] tracing: Two event pid filtering bug fixes

[Steven Rostedt]

Linus,

Two fixes to event pid filtering:

- Have created events reflect the current state of pid filtering

- Test pid filtering on discard test of recorded logic.
  (Also clean up the if statement to be cleaner).


Please pull the latest trace-v5.16-rc2-2 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v5.16-rc2-2

Tag SHA1: f819c68bfbd1fc780e85843700adf7b4f29f844d
Head SHA1: a55f224ff5f238013de8762c4287117e47b86e22


Steven Rostedt (VMware) (2):
      tracing: Check pid filtering when creating events
      tracing: Fix pid filtering when triggers are attached

----
 kernel/trace/trace.h        | 24 ++++++++++++++++++------
 kernel/trace/trace_events.c | 12 ++++++++++++
 2 files changed, 30 insertions(+), 6 deletions(-)
---------------------------
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 6b60ab9475ed..38715aa6cfdf 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1366,14 +1366,26 @@ __event_trigger_test_discard(struct trace_event_file *file,
 	if (eflags & EVENT_FILE_FL_TRIGGER_COND)
 		*tt = event_triggers_call(file, buffer, entry, event);
 
-	if (test_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &file->flags) ||
-	    (unlikely(file->flags & EVENT_FILE_FL_FILTERED) &&
-	     !filter_match_preds(file->filter, entry))) {
-		__trace_event_discard_commit(buffer, event);
-		return true;
-	}
+	if (likely(!(file->flags & (EVENT_FILE_FL_SOFT_DISABLED |
+				    EVENT_FILE_FL_FILTERED |
+				    EVENT_FILE_FL_PID_FILTER))))
+		return false;
+
+	if (file->flags & EVENT_FILE_FL_SOFT_DISABLED)
+		goto discard;
+
+	if (file->flags & EVENT_FILE_FL_FILTERED &&
+	    !filter_match_preds(file->filter, entry))
+		goto discard;
+
+	if ((file->flags & EVENT_FILE_FL_PID_FILTER) &&
+	    trace_event_ignore_this_pid(file))
+		goto discard;
 
 	return false;
+ discard:
+	__trace_event_discard_commit(buffer, event);
+	return true;
 }
 
 /**
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 4021b9a79f93..f8965fd50d3b 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2678,12 +2678,24 @@ static struct trace_event_file *
 trace_create_new_event(struct trace_event_call *call,
 		       struct trace_array *tr)
 {
+	struct trace_pid_list *no_pid_list;
+	struct trace_pid_list *pid_list;
 	struct trace_event_file *file;
+	unsigned int first;
 
 	file = kmem_cache_alloc(file_cachep, GFP_TRACE);
 	if (!file)
 		return NULL;
 
+	pid_list = rcu_dereference_protected(tr->filtered_pids,
+					     lockdep_is_held(&event_mutex));
+	no_pid_list = rcu_dereference_protected(tr->filtered_no_pids,
+					     lockdep_is_held(&event_mutex));
+
+	if (!trace_pid_list_first(pid_list, &first) ||
+	    !trace_pid_list_first(pid_list, &first))
+		file->flags |= EVENT_FILE_FL_PID_FILTER;
+
 	file->event_call = call;
 	file->tr = tr;
 	atomic_set(&file->sm_ref, 0);

Re: [GIT PULL] tracing: Two event pid filtering bug fixes

[Linus Torvalds]

On Sat, Nov 27, 2021 at 10:28 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Two fixes to event pid filtering:
>
> - Have created events reflect the current state of pid filtering
>
> - Test pid filtering on discard test of recorded logic.
>   (Also clean up the if statement to be cleaner).

I could not parse either of those statements. The second one in
particular is just a jumble of random words in a random order.

I tried to make it make sense by looking at the commits themselves,
but who knows. Maybe I made it worse by turning it from
incomprehensible to actively wrong.

Please make those explanations more clear in the future, ok?

               Linus

Re: [GIT PULL] tracing: Two event pid filtering bug fixes

[Steven Rostedt]

On Sat, 27 Nov 2021 12:12:55 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Nov 27, 2021 at 10:28 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Two fixes to event pid filtering:
> >
> > - Have created events reflect the current state of pid filtering
> >
> > - Test pid filtering on discard test of recorded logic.
> >   (Also clean up the if statement to be cleaner).  
> 
> I could not parse either of those statements. The second one in
> particular is just a jumble of random words in a random order.

Ouch, rereading that I see what you mean. I guess it was a combination
of Thanksgiving Turkey, then finding out my email isn't going out due
to the updates to mail.kernel.org (which was acting as my smarthost
relay), and my wife wondering where I am (as I wasn't doing the chores
she asked me to), so I was in a bit of a rush to get the pull request
out after it had passed all my tests.

> 
> I tried to make it make sense by looking at the commits themselves,
> but who knows. Maybe I made it worse by turning it from
> incomprehensible to actively wrong.

First change:

 - If pid filtering is enabled, a flag is set to the file descriptor of
   the event for the instance buffer that has pid filtering enabled.
   This flag is checked at the time of triggering to know if the event
   should be traced or not. The problem was when an event was created
   (by module load, kprobe, eprobe, etc), a file descriptor is create
   in each instance, but it wasn't updating the pid filtering flag for
   that event. Hence, those events would start tracing on every
   task and not honor the pid filter.

Second change:

 - Even if an event is filtered out, it could have a "trigger"
   associated it it (triggers do actions based on the event fields,
   like a stack dump into the tracing buffer, or a histogram, or even
   disable tracing). Thus, the event is recorded into a temporary
   buffer so its fields may be used by the trigger. But after the
   trigger has run, it needs to know if that event should be recorded
   into the trace buffer or not, so the filtering is executed again (if
   the event is to be filtered it is discarded). But this second check
   did not take into account pid filtering, and so if an event had a
   trigger attached to it, it did not honor the pid filters, and traced
   for all processes.

Better?

> 
> Please make those explanations more clear in the future, ok?

This was a unique situation. I hope to be better next time.

-- Steve

Re: [GIT PULL] tracing: Two event pid filtering bug fixes

[pr-tracker-bot]

The pull request you sent on Sat, 27 Nov 2021 13:28:22 -0500:

> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git trace-v5.16-rc2-2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/86155d6b43ced7768b70e7b57bcf53a6fb6b1f2d

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] tracing: Two event pid filtering bug fixes

[Steven Rostedt]

On Sat, 27 Nov 2021 13:28:22 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> +	struct trace_pid_list *no_pid_list;
> +	struct trace_pid_list *pid_list;
>  	struct trace_event_file *file;
> +	unsigned int first;
>  
>  	file = kmem_cache_alloc(file_cachep, GFP_TRACE);
>  	if (!file)
>  		return NULL;
>  
> +	pid_list = rcu_dereference_protected(tr->filtered_pids,
> +					     lockdep_is_held(&event_mutex));
> +	no_pid_list = rcu_dereference_protected(tr->filtered_no_pids,
> +					     lockdep_is_held(&event_mutex));
> +
> +	if (!trace_pid_list_first(pid_list, &first) ||
> +	    !trace_pid_list_first(pid_list, &first))
> +		file->flags |= EVENT_FILE_FL_PID_FILTER;

And of course since I only tested "trace only this pid" case, and not
the "trace everything but this pid" case, the above has a bug (which
the ktest bot just told me about), and my tests missed it.

That should have been:

	if (!trace_pid_list_first(pid_list, &first) ||
	    !trace_pid_list_first(no_pid_list, &first))

I'll fix it, run it through my tests, and post another pull request :-/.

/me needs to add that case to his tests, even though it's extremely
rare (I never use it). Which is exactly why I should have a test for it!

-- Steve

Re: [GIT PULL] tracing: Two event pid filtering bug fixes

[Steven Rostedt]

On Sat, 27 Nov 2021 16:41:20 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> And of course since I only tested "trace only this pid" case, and not
> the "trace everything but this pid" case, the above has a bug (which
> the ktest bot just told me about), and my tests missed it.

I'm giving a talk on Friday at Embedded Fest about all these features,
and I would like my examples to work on mainline Linux. ;-)

I found that my talks usually give some "crazy use cases" which tend to
find bugs in the code :-p

-- Steve