Re: [PATCH] tracing: Add "within" filter for call-stack-based event filtering

[Steven Rostedt <rostedt@goodmis.org>]

From: https://sashiko.dev/#/patchset/20260608145556.94931-1-chenjun102%40huawei.com

> commit dfbe6e67884d4ff73b44c46b51995f1945758a89
> Author: Chen Jun <chenjun102@huawei.com>
> tracing: Add "within" filter for call-stack-based event filtering
> This commit introduces a "within" filter predicate for trace events, allowing
> users to filter events based on whether a specific function appears in the
> current call stack. It uses kallsyms_lookup_name() to resolve the function to
> an address range during setup, and stack_trace_save() to capture the stack at
> runtime to evaluate the filter condition.
>
> > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > index 609325f579427..34e1a7f0b3cd0 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> [ ... ]
> > @@ -1009,6 +1010,22 @@ static int filter_pred_function(struct filter_pred *pred, void *event)
> >  	return pred->op == OP_EQ ? ret : !ret;
> >  }
> >  
> > +/* Filter predicate for within. */
> > +static int filter_pred_within(struct filter_pred *pred, void *event)
> > +{
> > +#ifdef CONFIG_STACKTRACE
> > +	unsigned long entries[16];
> > +	unsigned int nr_entries;
> > +	int i;
> > +
> > +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
>
> Is an array size of 16 entries sufficient here?  Since the skipnr parameter is
> 0, several of these frames will be consumed by internal tracing and filter
> evaluation functions (such as filter_pred_within() and
> trace_event_buffer_commit()).
>
> This leaves very few slots for the actual call chain, meaning higher-level
> functions like "vfs_read" will likely be truncated from the stack trace.
> Could this cause the filter condition to evaluate to false incorrectly,
> missing the events we are trying to capture?
>

This is a current limitation that may be dealt with in the future.

> > +	for (i = 0; i < nr_entries; i++)
> > +		if (pred->val <= entries[i] && entries[i] < pred->val2)
> > +			return !pred->not;
> > +#endif
> > +	return pred->not;
> > +}
>
> [ ... ]
> > @@ -2002,10 +2021,28 @@ static int parse_pred(const char *str, void *data,
> >  
> >  		} else if (field->filter_type == FILTER_DYN_STRING) {
> >  			pred->fn_num = FILTER_PRED_FN_STRLOC;
> > -		} else if (field->filter_type == FILTER_RDYN_STRING)
> > +		} else if (field->filter_type == FILTER_RDYN_STRING) {
> >  			pred->fn_num = FILTER_PRED_FN_STRRELLOC;
> > -		else {
> > +		} else if (field->filter_type == FILTER_WITHIN) {
> > +			unsigned long func;
> > +
> > +			if (op == OP_GLOB)
> > +				goto err_free;
>
> Does this goto need a preceding call to parse_error()?  Other error paths in
> this function initialize the error state before jumping to err_free.
> Without it, users might see an unhelpful generic error message instead of a
> syntax error indicating that the glob operator isn't supported here.

This is a legitimate issue.

Chen, can you send a v2 and add a proper error message here?

Thanks,

-- Steve


> > +
> > +			pred->fn_num = FILTER_PRED_FN_WITHIN;
> > +			func = kallsyms_lookup_name(pred->regex->pattern);