Re: [PATCH 2/4] tracing/user_events: Introduce multi-format events

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

On Sat, Jan 27, 2024 at 12:01:04AM +0900, Masami Hiramatsu wrote:
> On Tue, 23 Jan 2024 22:08:42 +0000
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > Add a register_name (reg_name) to the user_event struct which allows for
> > split naming of events. We now have the name that was used to register
> > within user_events as well as the unique name for the tracepoint. Upon
> > registering events ensure matches based on first the reg_name, followed
> > by the fields and format of the event. This allows for multiple events
> > with the same registered name to have different formats. The underlying
> > tracepoint will have a unique name in the format of {reg_name}:[unique_id].
> > 
> > For example, if both "test u32 value" and "test u64 value" are used with
> > the USER_EVENT_REG_MULTI_FORMAT the system would have 2 unique
> > tracepoints. The dynamic_events file would then show the following:
> >   u:test u64 count
> >   u:test u32 count
> > 
> > The actual tracepoint names look like this:
> >   test:[d5874fdac44]
> >   test:[d5914662cd4]
> > 
> > Both would be under the new user_events_multi system name to prevent the
> > older ABI from being used to squat on multi-formatted events and block
> > their use.
> [...]
> > @@ -1923,6 +1972,39 @@ static int user_event_trace_register(struct user_event *user)
> >  	return ret;
> >  }
> >  
> > +static int user_event_set_tp_name(struct user_event *user)
> > +{
> > +	lockdep_assert_held(&user->group->reg_mutex);
> > +
> > +	if (EVENT_MULTI_FORMAT(user->reg_flags)) {
> > +		char *multi_name;
> > +		int len;
> > +
> > +		len = snprintf(NULL, 0, "%s:[%llx]", user->reg_name,
> > +			       user->group->multi_id) + 1;
> > +
> > +		multi_name = kzalloc(len, GFP_KERNEL_ACCOUNT);
> > +
> > +		if (!multi_name)
> > +			return -ENOMEM;
> > +
> > +		snprintf(multi_name, len, "%s:[%llx]", user->reg_name,
> > +			 user->group->multi_id);
> 
> OK, so the each different event has suffixed name. But this will
> introduce non C-variable name.
> 
> Steve, do you think your library can handle these symbols? It will
> be something like "event:[1]" as the event name.
> Personally I like "event.1" style. (of course we need to ensure the
> user given event name is NOT including such suffix numbers)
> 

Just to clarify around events including a suffix number. This is why
multi-events use "user_events_multi" system name and the single-events
using just "user_events".

Even if a user program did include a suffix, the suffix would still get
appended. An example is "test" vs "test:[0]" using multi-format would
result in two tracepoints ("test:[0]" and "test:[0]:[1]" respectively
(assuming these are the first multi-events on the system).

I'm with you, we really don't want any spoofing or squatting possible.
By using different system names and always appending the suffix I
believe covers this.

Looking forward to hearing Steven's thoughts on this as well.

Thanks,
-Beau

> Thank you.
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>