[v3,2/4] tracing/user_events: Introduce multi-format events

  Currently user_events supports 1 event with the same name and must have
the exact same format when referenced by multiple programs. This opens
an opportunity for malicous or poorly thought through programs to
create events that others use with different formats. Another scenario
is user programs wishing to use the same event name but add more fields
later when the software updates. Various versions of a program may be
running side-by-side, which is prevented by the current single format
requirement.

Add a new register flag (USER_EVENT_REG_MULTI_FORMAT) which indicates
the user program wishes to use the same user_event name, but may have
several different formats of the event in the future. When this flag is
used, create the underlying tracepoint backing the user_event with a
unique name per-version of the format. It's important that existing ABI
users do not get this logic automatically, even if one of the multi
format events matches the format. This ensures existing programs that
create events and assume the tracepoint name will match exactly continue
to work as expected. Add logic to only check multi-format events with
other multi-format events and single-format events to only check
single-format events during find.

Change system name of the multi-format event tracepoint to ensure that
multi-format events are isolated completely from single-format events.
This prevents single-format names from conflicting with multi-format
events if they end with the same suffix as the multi-format events.

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.0
  test.1

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.

Deleting events via "!u:test u64 count" would only delete the first
tracepoint that matched that format. When the delete ABI is used all
events with the same name will be attempted to be deleted. If
per-version deletion is required, user programs should either not use
persistent events or delete them via dynamic_events.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 include/uapi/linux/user_events.h |   6 +-
 kernel/trace/trace_events_user.c | 118 +++++++++++++++++++++++++++----
 2 files changed, 111 insertions(+), 13 deletions(-)
  

On Wed, 14 Feb 2024 17:50:44 +0000
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> +static char *user_event_group_system_multi_name(void)
> +{
> +	char *system_name;
> +	int len = sizeof(USER_EVENTS_MULTI_SYSTEM) + 1;

FYI, the sizeof() will include the "\0" so no need for "+ 1", but I don't
think this matters as for what I mention below.

> +
> +	system_name = kmalloc(len, GFP_KERNEL);
> +
> +	if (!system_name)
> +		return NULL;
> +
> +	snprintf(system_name, len, "%s", USER_EVENTS_MULTI_SYSTEM);
> +
> +	return system_name;

Hmm, the above looks like an open coded version of:

	system_name = kstrdup(USER_EVENTS_MULTI_SYSTEM, GFP_KERNEL);


> +}
> +
>  static struct user_event_group *current_user_event_group(void)
>  {
>  	return init_group;
> @@ -367,6 +390,11 @@ static struct user_event_group *user_event_group_create(void)
>  	if (!group->system_name)
>  		goto error;
>  
> +	group->system_multi_name = user_event_group_system_multi_name();
> +
> +	if (!group->system_multi_name)
> +		goto error;
> +
>  	mutex_init(&group->reg_mutex);
>  	hash_init(group->register_table);
>  
> @@ -1482,6 +1510,11 @@ static int destroy_user_event(struct user_event *user)
>  	hash_del(&user->node);
>  
>  	user_event_destroy_validators(user);
> +
> +	/* If we have different names, both must be freed */
> +	if (EVENT_NAME(user) != EVENT_TP_NAME(user))
> +		kfree(EVENT_TP_NAME(user));
> +
>  	kfree(user->call.print_fmt);
>  	kfree(EVENT_NAME(user));
>  	kfree(user);
> @@ -1504,12 +1537,24 @@ static struct user_event *find_user_event(struct user_event_group *group,
>  	*outkey = key;
>  
>  	hash_for_each_possible(group->register_table, user, node, key) {
> +		/*
> +		 * Single-format events shouldn't return multi-format
> +		 * events. Callers expect the underlying tracepoint to match
> +		 * the name exactly in these cases. Only check like-formats.
> +		 */
> +		if (EVENT_MULTI_FORMAT(flags) != EVENT_MULTI_FORMAT(user->reg_flags))
> +			continue;
> +
>  		if (strcmp(EVENT_NAME(user), name))
>  			continue;
>  
>  		if (user_fields_match(user, argc, argv))
>  			return user_event_get(user);
>  
> +		/* Scan others if this is a multi-format event */
> +		if (EVENT_MULTI_FORMAT(flags))
> +			continue;
> +
>  		return ERR_PTR(-EADDRINUSE);
>  	}
>  
> @@ -1889,8 +1934,12 @@ static bool user_event_match(const char *system, const char *event,
>  	struct user_event *user = container_of(ev, struct user_event, devent);
>  	bool match;
>  
> -	match = strcmp(EVENT_NAME(user), event) == 0 &&
> -		(!system || strcmp(system, USER_EVENTS_SYSTEM) == 0);
> +	match = strcmp(EVENT_NAME(user), event) == 0;
> +
> +	if (match && system) {
> +		match = strcmp(system, user->group->system_name) == 0 ||
> +			strcmp(system, user->group->system_multi_name) == 0;
> +	}
>  
>  	if (match)
>  		match = user_fields_match(user, argc, argv);
> @@ -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);

I believe the above can be replaced with:

		multi_name = kasprintf(GFP_KERNEL_ACCOUNT, "%s.%llx", user->reg_name,
				       user->group->multi_id);
		if (!multi_name)
			return -ENOMEM;

-- Steve

> +
> +		user->call.name = multi_name;
> +		user->tracepoint.name = multi_name;
> +
> +		/* Inc to ensure unique multi-event name next time */
> +		user->group->multi_id++;
> +	} else {
> +		/* Non Multi-format uses register name */
> +		user->call.name = user->reg_name;
> +		user->tracepoint.name = user->reg_name;
> +	}
> +
> +	return 0;
> +}
  

On Wed, 14 Feb 2024 17:50:44 +0000
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> Currently user_events supports 1 event with the same name and must have
> the exact same format when referenced by multiple programs. This opens
> an opportunity for malicous or poorly thought through programs to
> create events that others use with different formats. Another scenario
> is user programs wishing to use the same event name but add more fields
> later when the software updates. Various versions of a program may be
> running side-by-side, which is prevented by the current single format
> requirement.
> 
> Add a new register flag (USER_EVENT_REG_MULTI_FORMAT) which indicates
> the user program wishes to use the same user_event name, but may have
> several different formats of the event in the future. When this flag is

"of the event in the future." Does it have to be in the future? Is there
use case where an application might legitimately want the same event name
with different formats?

-- Steve

> used, create the underlying tracepoint backing the user_event with a
> unique name per-version of the format. It's important that existing ABI
> users do not get this logic automatically, even if one of the multi
> format events matches the format. This ensures existing programs that
> create events and assume the tracepoint name will match exactly continue
> to work as expected. Add logic to only check multi-format events with
> other multi-format events and single-format events to only check
> single-format events during find.
>
  

On Wed, Feb 21, 2024 at 10:21:04AM -0500, Steven Rostedt wrote:
> On Wed, 14 Feb 2024 17:50:44 +0000
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > Currently user_events supports 1 event with the same name and must have
> > the exact same format when referenced by multiple programs. This opens
> > an opportunity for malicous or poorly thought through programs to
> > create events that others use with different formats. Another scenario
> > is user programs wishing to use the same event name but add more fields
> > later when the software updates. Various versions of a program may be
> > running side-by-side, which is prevented by the current single format
> > requirement.
> > 
> > Add a new register flag (USER_EVENT_REG_MULTI_FORMAT) which indicates
> > the user program wishes to use the same user_event name, but may have
> > several different formats of the event in the future. When this flag is
> 
> "of the event in the future." Does it have to be in the future? Is there
> use case where an application might legitimately want the same event name
> with different formats?
> 

You're right, our use cases are mostly around future facing/compat.
There are valid cases where you just want several different formats with
the same name.

I'll drop the "in the future", so it'll just be "several different
formats".

Thanks,
-Beau

> -- Steve
> 
> > used, create the underlying tracepoint backing the user_event with a
> > unique name per-version of the format. It's important that existing ABI
> > users do not get this logic automatically, even if one of the multi
> > format events matches the format. This ensures existing programs that
> > create events and assume the tracepoint name will match exactly continue
> > to work as expected. Add logic to only check multi-format events with
> > other multi-format events and single-format events to only check
> > single-format events during find.
> >
  

On Wed, Feb 21, 2024 at 10:08:33AM -0500, Steven Rostedt wrote:
> On Wed, 14 Feb 2024 17:50:44 +0000
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > +static char *user_event_group_system_multi_name(void)
> > +{
> > +	char *system_name;
> > +	int len = sizeof(USER_EVENTS_MULTI_SYSTEM) + 1;
> 
> FYI, the sizeof() will include the "\0" so no need for "+ 1", but I don't
> think this matters as for what I mention below.
> 
> > +
> > +	system_name = kmalloc(len, GFP_KERNEL);
> > +
> > +	if (!system_name)
> > +		return NULL;
> > +
> > +	snprintf(system_name, len, "%s", USER_EVENTS_MULTI_SYSTEM);
> > +
> > +	return system_name;
> 
> Hmm, the above looks like an open coded version of:
> 
> 	system_name = kstrdup(USER_EVENTS_MULTI_SYSTEM, GFP_KERNEL);
> 

That's much cleaner, I'll move to that.

> 
> > +}
> > +
> >  static struct user_event_group *current_user_event_group(void)
> >  {
> >  	return init_group;
> > @@ -367,6 +390,11 @@ static struct user_event_group *user_event_group_create(void)
> >  	if (!group->system_name)
> >  		goto error;
> >  
> > +	group->system_multi_name = user_event_group_system_multi_name();
> > +
> > +	if (!group->system_multi_name)
> > +		goto error;
> > +
> >  	mutex_init(&group->reg_mutex);
> >  	hash_init(group->register_table);
> >  
> > @@ -1482,6 +1510,11 @@ static int destroy_user_event(struct user_event *user)
> >  	hash_del(&user->node);
> >  
> >  	user_event_destroy_validators(user);
> > +
> > +	/* If we have different names, both must be freed */
> > +	if (EVENT_NAME(user) != EVENT_TP_NAME(user))
> > +		kfree(EVENT_TP_NAME(user));
> > +
> >  	kfree(user->call.print_fmt);
> >  	kfree(EVENT_NAME(user));
> >  	kfree(user);
> > @@ -1504,12 +1537,24 @@ static struct user_event *find_user_event(struct user_event_group *group,
> >  	*outkey = key;
> >  
> >  	hash_for_each_possible(group->register_table, user, node, key) {
> > +		/*
> > +		 * Single-format events shouldn't return multi-format
> > +		 * events. Callers expect the underlying tracepoint to match
> > +		 * the name exactly in these cases. Only check like-formats.
> > +		 */
> > +		if (EVENT_MULTI_FORMAT(flags) != EVENT_MULTI_FORMAT(user->reg_flags))
> > +			continue;
> > +
> >  		if (strcmp(EVENT_NAME(user), name))
> >  			continue;
> >  
> >  		if (user_fields_match(user, argc, argv))
> >  			return user_event_get(user);
> >  
> > +		/* Scan others if this is a multi-format event */
> > +		if (EVENT_MULTI_FORMAT(flags))
> > +			continue;
> > +
> >  		return ERR_PTR(-EADDRINUSE);
> >  	}
> >  
> > @@ -1889,8 +1934,12 @@ static bool user_event_match(const char *system, const char *event,
> >  	struct user_event *user = container_of(ev, struct user_event, devent);
> >  	bool match;
> >  
> > -	match = strcmp(EVENT_NAME(user), event) == 0 &&
> > -		(!system || strcmp(system, USER_EVENTS_SYSTEM) == 0);
> > +	match = strcmp(EVENT_NAME(user), event) == 0;
> > +
> > +	if (match && system) {
> > +		match = strcmp(system, user->group->system_name) == 0 ||
> > +			strcmp(system, user->group->system_multi_name) == 0;
> > +	}
> >  
> >  	if (match)
> >  		match = user_fields_match(user, argc, argv);
> > @@ -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);
> 
> I believe the above can be replaced with:
> 
> 		multi_name = kasprintf(GFP_KERNEL_ACCOUNT, "%s.%llx", user->reg_name,
> 				       user->group->multi_id);
> 		if (!multi_name)
> 			return -ENOMEM;
> 

Great, I'll move to that as well and validate.

Thanks,
-Beau

> -- Steve
> 
> > +
> > +		user->call.name = multi_name;
> > +		user->tracepoint.name = multi_name;
> > +
> > +		/* Inc to ensure unique multi-event name next time */
> > +		user->group->multi_id++;
> > +	} else {
> > +		/* Non Multi-format uses register name */
> > +		user->call.name = user->reg_name;
> > +		user->tracepoint.name = user->reg_name;
> > +	}
> > +
> > +	return 0;
> > +}