user_events: Enable user processes to create and write to trace events

Add a documentation file about user_events with example code, etc. explaining how it may be used.

Signed-off-by Beau Belgrave <[email protected]>

---
 Documentation/trace/user_events.rst | 298 ++++++++++++++++++++++++++++
 1 file changed, 298 insertions(+)
 create mode 100644 Documentation/trace/user_events.rst

Documentation/trace/user_events.rst new file +23 -0

new file mode 100644
index 000000000000..d79c9f07d012
@@ -0,0 +1,298 @@
+=========================================
+user_events: User-based Event Tracing
+=========================================
+
+:Author: Beau Belgrave
+
+Overview
+--------
+User based trace events allow user processes to create events and trace data
+that can be viewed via existing tools, such as ftrace, perf and eBPF.
+To enable this feature, build your kernel with CONFIG_USER_EVENTS=y.
+
+Programs can view status of the events via 
+/sys/kernel/debug/tracing/user_events_status and can both register and write
+data out via /sys/kernel/debug/tracing/user_events_data.
+
+Programs can also use /sys/kernel/debug/tracing/dynamic_events to register and
+delete user based events via the u: prefix. The format of the command to
+dynamic_events is the same as the ioctl with the u: prefix applied.
+
+Typically programs will register a set of events that they wish to expose to
+tools that can read trace_events (such as ftrace and perf). The registration
+process gives back two ints to the program for each event. The first int is the

Documentation/trace/user_events.rst new file +12 -0

+Registering
+-----------
+Registering within a user process is done via ioctl() out to the
+/sys/kernel/debug/tracing/user_events_data file. The command to issue is
+DIAG_IOCSREG. This command takes a struct user_reg as an argument.
+
+The struct user_reg requires two values, the first is the size of the structure
+to ensure forward and backward compatibility. The second is the command string
+to issue for registering.
+
+User based events show up under tracefs like any other event under the subsystem
+named "user_events". This means tools that wish to attach to the events need to

Documentation/trace/user_events.rst new file +145 -0

+  name[:FLAG1[,FLAG2...]] [Field1[;Field2...]]
+
+Supported Flags
+^^^^^^^^^^^^^^^
+**BPF_ITER** - EBPF programs attached to this event will get the raw iovec
+struct instead of any data copies for max performance.
+
+Field Format
+^^^^^^^^^^^^
+
+::
+
+  type name [size]
+
+Basic types are supported (__data_loc, u32, u64, int, char, char[20]).
+User programs are encouraged to use clearly sized types like u32.
+
+**NOTE:** *Long is not supported since size can vary between user and kernel.*
+
+The size is only valid for types that start with a struct prefix.
+This allows user programs to describe custom structs out to tools, if required.
+
+For example, a struct in C that looks like this:
+
+::
+
+  struct mytype {
+    char data[20];
+  };
+
+Would be represented by the following field:
+
+::
+
+  struct mytype myname 20
+
+Status
+------
+When tools attach/record user based events the status of the event is updated
+in realtime. This allows user programs to only incur the cost of the write() or
+writev() calls when something is actively attached to the event.
+
+User programs call mmap() on /sys/kernel/debug/tracing/user_events_status to
+check the status for each event that is registered. The byte to check in the
+file is given back after the register ioctl() via user_reg.status_index.
+Currently the size of user_events_status is a single page, however, custom
+kernel configurations can change this size to allow more user based events. In
+all cases the size of the file is a multiple of a page size.
+
+For example, if the register ioctl() gives back a status_index of 3 you would
+check byte 3 of the returned mmap data to see if anything is attached to that
+event.
+
+Administrators can easily check the status of all registered events by reading
+the user_events_status file directly via a terminal. The output is as follows:
+
+::
+
+  Byte:Name [# Comments]
+  ...
+
+  Active: ActiveCount
+  Buisy: BusyCount
+  Max: MaxCount
+
+For example, on a system that has a single event the output looks like this:
+
+::
+
+  1:test
+
+  Active: 1
+  Busy: 0
+  Max: 4096
+
+If a user enables the user event via ftrace, the output would change to this:
+
+:: 
+
+  1:test # Used by ftrace
+
+  Active: 1
+  Busy: 1
+  Max: 4096
+
+**NOTE:** *A status index of 0 will never be returned. This allows user 
+programs to have an index that can be used on error cases.*
+
+Status Bits
+^^^^^^^^^^^
+The byte being checked will be non-zero if anything is attached. Programs can
+check specific bits in the byte to see what mechanism has been attached.
+
+The following values are defined to aid in checking what has been attached:
+**EVENT_STATUS_FTRACE** - Bit set if ftrace has been attached (Bit 0).
+
+**EVENT_STATUS_PERF** - Bit set if perf/eBPF has been attached (Bit 1).
+
+Writing Data
+------------
+After registering an event the same fd that was used to register can be used
+to write an entry for that event. The write_index returned must be at the start
+of the data, then the remaining data is treated as the payload of the event.
+
+For example, if write_index returned was 1 and I wanted to write out an int
+payload of the event. Then the data would have to be 8 bytes (2 ints) long,
+with the first 4 bytes being equal to 1 and the last 4 bytes being equal to the
+value I want as the payload.
+
+In memory this would look like this:
+
+::
+
+  int index;
+  int payload;
+
+User programs might have well known structs that they wish to use to emit out
+as payloads. In those cases writev() can be used, with the first vector being
+the index and the following vector(s) being the actual event payload.
+
+For example, if I have a struct like this:
+
+::
+
+  struct payload {
+        int src;
+        int dst;
+        int flags;
+  };
+
+It's advised for user programs to do the following:
+
+:: 
+
+  struct iovec io[2];
+  struct payload e;
+
+  io[0].iov_base = &write_index;
+  io[0].iov_len = sizeof(write_index);
+  io[1].iov_base = &e;
+  io[1].iov_len = sizeof(e);
+
+  writev(fd, (const struct iovec*)io, 2);
+
+**NOTE:** *The write_index is not emitted out into the trace being recorded.*

Documentation/trace/user_events.rst new file +75 -0

+::
+
+  #include <errno.h>
+  #include <sys/ioctl.h>
+  #include <sys/mman.h>
+  #include <fcntl.h>
+  #include <stdio.h>
+  #include <unistd.h>
+  #include <linux/user_events.h>
+  
+  /* Assumes debugfs is mounted */
+  const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
+  const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
+  
+  static int event_status(char **status)
+  {
+  	int fd = open(status_file, O_RDONLY);
+  
+  	*status = mmap(NULL, sysconf(_SC_PAGESIZE), PROT_READ,
+  		       MAP_SHARED, fd, 0);
+  
+  	close(fd);
+  
+  	if (*status == MAP_FAILED)
+  	      return -1;
+  
+  	return 0;
+  }
+  
+  static int event_reg(int fd, const char *command, int *status, int *write)
+  {
+  	struct user_reg reg = {0};
+  
+  	reg.size = sizeof(reg);
+  	reg.name_args = (__u64)command;
+  
+  	if (ioctl(fd, DIAG_IOCSREG, &reg) == -1)
+  		return -1;
+  
+  	*status = reg.status_index;
+  	*write = reg.write_index;
+  
+  	return 0;
+  }
+  
+  int main(int argc, char **argv)
+  {
+  	int data_fd, status, write;
+  	char *status_page;
+  	struct iovec io[2];
+  	__u32 count = 0;
+  
+  	if (event_status(&status_page) == -1)
+  		return errno;
+  
+  	data_fd = open(data_file, O_RDWR);
+  
+  	if (event_reg(data_fd, "test u32 count", &status, &write) == -1)
+  		return errno;
+  
+  	/* Setup iovec */
+  	io[0].iov_base = &status;
+  	io[0].iov_len = sizeof(status);
+  	io[1].iov_base = &count;
+  	io[1].iov_len = sizeof(count);
+  
+  ask:
+  	printf("Press enter to check status...\n");
+  	getchar();
+  
+  	/* Check if anyone is listening */
+  	if (status_page[status]) {
+  		/* Yep, trace out our data */
+  		writev(data_fd, (const struct iovec*)io, 2);
+  

Quoted 2 lines · Beau Belgrave

Add a documentation file about user_events with example code, etc. explaining how it may be used.

Yay documentation! Some nits for a slow moment...

Quoted 5 lines

Signed-off-by Beau Belgrave <[email protected]>

---
 Documentation/trace/user_events.rst | 298 ++++++++++++++++++++++++++++
 1 file changed, 298 insertions(+)
 create mode 100644 Documentation/trace/user_events.rst

Actually, this isn't really a nit. When you add a new RST file, you need to add it to the index.rst file too so that it gets pulled into the docs build. Otherwise you'll get the warning you doubtless saw when you tried building the docs after adding this file...:)

Quoted 2 lines

+Example Code
+------------

Examples are great, but they are best placed under samples/ (or tools/ if they do something useful) where readers can build and run them.

Quoted 3 lines · Jonathan Corbet

Examples are great, but they are best placed under samples/ (or tools/ if they do something useful) where readers can build and run them.

Ok, sounds good. Is it fine to include user mode samples in there? I quickly checked and most appear to be modules. Maybe there is a better place for user mode examples?

index 000000000000..a68017ad7fdd

kernel/trace/trace_events_user.c new file +113 -0

@@ -0,0 +1,1140 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Microsoft Corporation.
+ *
+ * Authors:
+ *   Beau Belgrave <[email protected]>
+ */
+
+#include <linux/bitmap.h>
+#include <linux/cdev.h>
+#include <linux/hashtable.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/uio.h>
+#include <linux/ioctl.h>
+#include <linux/jhash.h>
+#include <linux/trace_events.h>
+#include <linux/tracefs.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <uapi/linux/user_events.h>
+#include "trace.h"
+#include "trace_dynevent.h"
+
+#define USER_EVENTS_PREFIX_LEN (sizeof(USER_EVENTS_PREFIX)-1)
+
+#define FIELD_DEPTH_TYPE 0
+#define FIELD_DEPTH_NAME 1
+#define FIELD_DEPTH_SIZE 2
+
+/*
+ * Limits how many trace_event calls user processes can create:
+ * Must be multiple of PAGE_SIZE.
+ */
+#define MAX_PAGES 1
+#define MAX_EVENTS (MAX_PAGES * PAGE_SIZE)
+
+/* Limit how long of an event name plus args within the subsystem. */
+#define MAX_EVENT_DESC 512
+#define EVENT_NAME(user_event) ((user_event)->tracepoint.name)
+
+static char *register_page_data;
+
+static DEFINE_MUTEX(reg_mutex);
+static DEFINE_HASHTABLE(register_table, 4);
+static DECLARE_BITMAP(page_bitmap, MAX_EVENTS);
+
+struct user_event {
+	struct tracepoint tracepoint;
+	struct trace_event_call call;
+	struct trace_event_class class;
+	struct dyn_event devent;
+	struct hlist_node node;
+	struct list_head fields;
+	atomic_t refcnt;
+	int index;
+	int flags;
+};
+
+struct user_event_refs {
+	struct rcu_head rcu;
+	int count;
+	struct user_event *events[];
+};
+
+typedef void (*user_event_func_t) (struct user_event *user,
+				   void *data, u32 datalen,
+				   void *tpdata);
+
+static int user_event_parse(char *name, char *args, char *flags,
+			    struct user_event **newuser);
+
+static u32 user_event_key(char *name)
+{
+	return jhash(name, strlen(name), 0);
+}
+
+static struct list_head *user_event_get_fields(struct trace_event_call *call)
+{
+	struct user_event *user = (struct user_event *)call->data;
+
+	return &user->fields;
+}
+
+/*
+ * Parses a register command for user_events
+ * Format: event_name[:FLAG1[,FLAG2...]] [field1[;field2...]]
+ *
+ * Example event named test with a 20 char msg field with a unsigned int after:
+ * test char[20] msg;unsigned int id
+ *
+ * NOTE: Offsets are from the user data perspective, they are not from the
+ * trace_entry/buffer perspective. We automatically add the common properties
+ * sizes to the offset for the user.
+ */
+static int user_event_parse_cmd(char *raw_command, struct user_event **newuser)
+{
+	char *name = raw_command;
+	char *args = strpbrk(name, " ");
+	char *flags;
+
+	if (args)
+		*args++ = 0;
+
+	flags = strpbrk(name, ":");
+
+	if (flags)
+		*flags++ = 0;
+
+	return user_event_parse(name, args, flags, newuser);
+}
+
+static int user_field_array_size(const char *type)

kernel/trace/trace_events_user.c new file +94 -0

+
+	if (*start != ']')
+		return -EINVAL;
+
+	return size;
+}
+
+static int user_field_size(const char *type)
+{
+	/* long is not allowed from a user, since it's ambigious in size */
+	if (strcmp(type, "s64") == 0)
+		return sizeof(s64);
+	if (strcmp(type, "u64") == 0)
+		return sizeof(u64);
+	if (strcmp(type, "s32") == 0)
+		return sizeof(s32);
+	if (strcmp(type, "u32") == 0)
+		return sizeof(u32);
+	if (strcmp(type, "int") == 0)
+		return sizeof(int);
+	if (strcmp(type, "unsigned int") == 0)
+		return sizeof(unsigned int);
+	if (strcmp(type, "s16") == 0)
+		return sizeof(s16);
+	if (strcmp(type, "u16") == 0)
+		return sizeof(u16);
+	if (strcmp(type, "short") == 0)
+		return sizeof(short);
+	if (strcmp(type, "unsigned short") == 0)
+		return sizeof(unsigned short);
+	if (strcmp(type, "s8") == 0)
+		return sizeof(s8);
+	if (strcmp(type, "u8") == 0)
+		return sizeof(u8);
+	if (strcmp(type, "char") == 0)
+		return sizeof(char);
+	if (strcmp(type, "unsigned char") == 0)
+		return sizeof(unsigned char);
+	if (strstr(type, "char[") == type)
+		return user_field_array_size(type);
+	if (strstr(type, "unsigned char[") == type)
+		return user_field_array_size(type);
+	if (strstr(type, "__data_loc ") == type)
+		return sizeof(u32);
+	if (strstr(type, "__rel_loc ") == type)
+		return sizeof(u32);
+
+	/* Uknown basic type, error */
+	return -EINVAL;
+}
+
+static void user_event_destroy_fields(struct user_event *user)
+{
+	struct ftrace_event_field *field, *next;
+	struct list_head *head = &user->fields;
+
+	list_for_each_entry_safe(field, next, head, link) {
+		list_del(&field->link);
+		kfree(field);
+	}
+}
+
+static int user_event_add_field(struct user_event *user, const char *type,
+				const char *name, int offset, int size,
+				int is_signed, int filter_type)
+{
+	struct ftrace_event_field *field;
+
+	field = kmalloc(sizeof(*field), GFP_KERNEL);
+
+	if (!field)
+		return -ENOMEM;
+
+	field->type = type;
+	field->name = name;
+	field->offset = offset;
+	field->size = size;
+	field->is_signed = is_signed;
+	field->filter_type = filter_type;
+
+	list_add(&field->link, &user->fields);
+
+	return 0;
+}
+
+/*
+ * Parses the values of a field within the description
+ * Format: type name [size]
+ */
+static int user_event_parse_field(char *field, struct user_event *user,
+				  u32 *offset)
+{
+	char *part, *type, *name;
+	u32 depth = 0, saved_offset = *offset;

kernel/trace/trace_events_user.c new file +96 -0

+		type = field;
+		field = strpbrk(field + sizeof("unsigned"), " ");
+		goto skip_next;
+	} else if (strstr(field, "struct ") == field) {
+		type = field;
+		field = strpbrk(field + sizeof("struct"), " ");
+		is_struct = true;
+		goto skip_next;
+	} else if (strstr(field, "__data_loc unsigned ") == field) {
+		type = field;
+		field = strpbrk(field + sizeof("__data_loc unsigned"), " ");
+		goto skip_next;
+	} else if (strstr(field, "__data_loc ") == field) {
+		type = field;
+		field = strpbrk(field + sizeof("__data_loc"), " ");
+		goto skip_next;
+	} else if (strstr(field, "__rel_loc unsigned ") == field) {
+		type = field;
+		field = strpbrk(field + sizeof("__rel_loc unsigned"), " ");
+		goto skip_next;
+	} else if (strstr(field, "__rel_loc ") == field) {
+		type = field;
+		field = strpbrk(field + sizeof("__rel_loc"), " ");
+		goto skip_next;
+	}
+	goto parse;
+skip_next:
+	if (field == NULL)
+		return -EINVAL;
+
+	*field++ = 0;
+	depth++;
+parse:
+	while ((part = strsep(&field, " ")) != NULL) {
+		switch (depth++) {
+		case FIELD_DEPTH_TYPE:
+			type = part;
+			break;
+		case FIELD_DEPTH_NAME:
+			name = part;
+			break;
+		case FIELD_DEPTH_SIZE:
+			if (!is_struct)
+				return -EINVAL;
+
+			if (kstrtou32(part, 10, &size))
+				return -EINVAL;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	if (depth < FIELD_DEPTH_SIZE)
+		return -EINVAL;
+
+	if (depth == FIELD_DEPTH_SIZE)
+		size = user_field_size(type);
+
+	if (size == 0)
+		return -EINVAL;
+
+	if (size < 0)
+		return size;
+
+	*offset = saved_offset + size;
+
+	return user_event_add_field(user, type, name, saved_offset, size,
+				    type[0] != 'u', FILTER_OTHER);
+}
+
+static void user_event_parse_flags(struct user_event *user, char *flags)
+{
+	char *flag;
+
+	if (flags == NULL)
+		return;
+
+	while ((flag = strsep(&flags, ",")) != NULL) {
+		if (strcmp(flag, "BPF_ITER") == 0)
+			user->flags |= FLAG_BPF_ITER;
+	}
+}
+
+static int user_event_parse_fields(struct user_event *user, char *args)
+{
+	char *field;
+	u32 offset = sizeof(struct trace_entry);
+	int ret = -EINVAL;
+
+	if (args == NULL)
+		return 0;
+
+	while ((field = strsep(&args, ";")) != NULL) {
+		ret = user_event_parse_field(field, user, &offset);
+

kernel/trace/trace_events_user.c new file +59 -0

+
+	return trace_handle_return(&iter->seq);
+}
+
+static struct trace_event_functions user_event_funcs = {
+	.trace = user_event_print_trace,
+};
+
+static int destroy_user_event(struct user_event *user)
+{
+	int ret = 0;
+
+	/* Must destroy fields before call removal */
+	user_event_destroy_fields(user);
+
+	ret = trace_remove_event_call(&user->call);
+
+	if (ret)
+		return ret;
+
+	dyn_event_remove(&user->devent);
+
+	register_page_data[user->index] = 0;
+	clear_bit(user->index, page_bitmap);
+	hash_del(&user->node);
+
+	kfree(EVENT_NAME(user));
+	kfree(user);
+
+	return ret;
+}
+
+static struct user_event *find_user_event(char *name, u32 *outkey)
+{
+	struct user_event *user;
+	u32 key = user_event_key(name);
+
+	*outkey = key;
+
+	hash_for_each_possible(register_table, user, node, key)
+		if (!strcmp(EVENT_NAME(user), name))
+			return user;
+
+	return NULL;
+}
+
+/*
+ * Writes the user supplied payload out to a trace file.
+ */
+static void user_event_ftrace(struct user_event *user, void *data, u32 datalen,
+			      void *tpdata)
+{
+	struct trace_event_file *file;
+	struct trace_entry *entry;
+	struct trace_event_buffer event_buffer;
+
+	file = (struct trace_event_file *)tpdata;
+
+	if (!file ||

kernel/trace/trace_events_user.c new file +26 -0

+	memcpy(entry + 1, data, datalen);
+
+	trace_event_buffer_commit(&event_buffer);
+}
+
+/*
+ * Update the register page that is shared between user processes.
+ */
+static void update_reg_page_for(struct user_event *user)
+{
+	struct tracepoint *tp = &user->tracepoint;
+	char status = 0;
+
+	if (atomic_read(&tp->key.enabled) > 0) {
+		struct tracepoint_func *probe_func_ptr;
+		user_event_func_t probe_func;
+
+		rcu_read_lock_sched();
+
+		probe_func_ptr = rcu_dereference_sched(tp->funcs);
+
+		if (probe_func_ptr) {
+			do {
+				probe_func = probe_func_ptr->func;
+
+				if (probe_func == user_event_ftrace)

kernel/trace/trace_events_user.c new file +55 -0

+}
+
+/*
+ * Register callback for our events from tracing sub-systems.
+ */
+static int user_event_reg(struct trace_event_call *call,
+			  enum trace_reg type,
+			  void *data)
+{
+	struct user_event *user = (struct user_event *)call->data;
+	int ret = 0;
+
+	if (!user)
+		return -ENOENT;
+
+	switch (type) {
+	case TRACE_REG_REGISTER:
+		ret = tracepoint_probe_register(call->tp,
+						call->class->probe,
+						data);
+		if (!ret)
+			goto inc;
+		break;
+
+	case TRACE_REG_UNREGISTER:
+		tracepoint_probe_unregister(call->tp,
+					    call->class->probe,
+					    data);
+		goto dec;
+
+#ifdef CONFIG_PERF_EVENTS
+	case TRACE_REG_PERF_REGISTER:
+	case TRACE_REG_PERF_UNREGISTER:
+	case TRACE_REG_PERF_OPEN:
+	case TRACE_REG_PERF_CLOSE:
+	case TRACE_REG_PERF_ADD:
+	case TRACE_REG_PERF_DEL:
+		break;
+#endif
+	}
+
+	return ret;
+inc:
+	atomic_inc(&user->refcnt);
+	update_reg_page_for(user);
+	return 0;
+dec:
+	update_reg_page_for(user);
+	atomic_dec(&user->refcnt);
+	return 0;
+}
+
+static int user_event_create(const char *raw_command)
+{
+	struct user_event *user;

kernel/trace/trace_events_user.c new file +37 -0

+
+	if (!name)
+		return -ENOMEM;
+
+	mutex_lock(&reg_mutex);
+	ret = user_event_parse_cmd(name, &user);
+	mutex_unlock(&reg_mutex);
+
+	return ret;
+}
+
+static int user_event_show(struct seq_file *m, struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+	struct ftrace_event_field *field, *next;
+	struct list_head *head;
+	int depth = 0;
+
+	seq_printf(m, "%s%s", USER_EVENTS_PREFIX, EVENT_NAME(user));
+
+	head = trace_get_fields(&user->call);
+
+	list_for_each_entry_safe_reverse(field, next, head, link) {
+		if (depth == 0)
+			seq_puts(m, " ");
+		else
+			seq_puts(m, "; ");
+		seq_printf(m, "%s %s", field->type, field->name);
+		depth++;
+	}
+
+	seq_puts(m, "\n");
+
+	return 0;
+}
+
+static bool user_event_is_busy(struct dyn_event *ev)

kernel/trace/trace_events_user.c new file +158 -0

+	return destroy_user_event(user);
+}
+
+static bool user_event_match(const char *system, const char *event,
+			     int argc, const char **argv, struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+
+	return strcmp(EVENT_NAME(user), event) == 0 &&
+		(!system || strcmp(system, USER_EVENTS_SYSTEM) == 0);
+}
+
+static struct dyn_event_operations user_event_dops = {
+	.create = user_event_create,
+	.show = user_event_show,
+	.is_busy = user_event_is_busy,
+	.free = user_event_free,
+	.match = user_event_match,
+};
+
+static int user_event_trace_register(struct user_event *user)
+{
+	int ret;
+
+	ret = register_trace_event(&user->call.event);
+
+	if (!ret)
+		return -ENODEV;
+
+	ret = trace_add_event_call(&user->call);
+
+	if (ret)
+		unregister_trace_event(&user->call.event);
+
+	return ret;
+}
+
+/*
+ * Parses the event name, arguments and flags then registers if successful.
+ */
+static int user_event_parse(char *name, char *args, char *flags,
+			    struct user_event **newuser)
+{
+	int ret;
+	int index;
+	u32 key;
+	struct user_event *user = find_user_event(name, &key);
+
+	if (user) {
+		*newuser = user;
+		ret = 0;
+		goto put_name;
+	}
+
+	index = find_first_zero_bit(page_bitmap, MAX_EVENTS);
+
+	if (index == MAX_EVENTS) {
+		ret = -EMFILE;
+		goto put_name;
+	}
+
+	user = kzalloc(sizeof(*user), GFP_KERNEL);
+
+	if (!user) {
+		ret = -ENOMEM;
+		goto put_name;
+	}
+
+	INIT_LIST_HEAD(&user->class.fields);
+	INIT_LIST_HEAD(&user->fields);
+
+	user->tracepoint.name = name;
+
+	user_event_parse_flags(user, flags);
+
+	ret = user_event_parse_fields(user, args);
+
+	if (ret)
+		goto put_user;
+
+	/* Minimal print format */
+	user->call.print_fmt = "\"\"";
+
+	user->call.data = user;
+	user->call.class = &user->class;
+	user->call.name = name;
+	user->call.flags = TRACE_EVENT_FL_TRACEPOINT;
+	user->call.tp = &user->tracepoint;
+	user->call.event.funcs = &user_event_funcs;
+
+	user->class.system = USER_EVENTS_SYSTEM;
+	user->class.fields_array = user_event_fields_array;
+	user->class.get_fields = user_event_get_fields;
+	user->class.reg = user_event_reg;
+	user->class.probe = user_event_ftrace;
+
+	mutex_lock(&event_mutex);
+	ret = user_event_trace_register(user);
+	mutex_unlock(&event_mutex);
+
+	if (ret)
+		goto put_user;
+
+	user->index = index;
+	dyn_event_init(&user->devent, &user_event_dops);
+	dyn_event_add(&user->devent);
+	set_bit(user->index, page_bitmap);
+	hash_add(register_table, &user->node, key);
+
+	*newuser = user;
+	return 0;
+put_user:
+	user_event_destroy_fields(user);
+	kfree(user);
+put_name:
+	kfree(name);
+	return ret;
+}
+
+/*
+ * Deletes a previously created event if it is no longer being used.
+ */
+static int delete_user_event(char *name)
+{
+	u32 key;
+	int ret;
+	struct user_event *user = find_user_event(name, &key);
+
+	if (!user)
+		return -ENOENT;
+
+	if (atomic_read(&user->refcnt) != 0)
+		return -EBUSY;
+
+	mutex_lock(&event_mutex);
+	ret = destroy_user_event(user);
+	mutex_unlock(&event_mutex);
+
+	return ret;
+}
+
+/*
+ * Validates the user payload and writes via iterator.
+ */
+static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
+{
+	struct user_event_refs *refs;
+	struct user_event *user = NULL;
+	struct tracepoint *tp;
+	ssize_t ret = i->count;
+	int idx;
+
+	if (unlikely(copy_from_iter(&idx, sizeof(idx), i) != sizeof(idx)))
+		return -EFAULT;
+
+	rcu_read_lock_sched();
+
+	refs = rcu_dereference_sched(file->private_data);

kernel/trace/trace_events_user.c new file +42 -0

+
+		if (unlikely(!kdata))
+			return -ENOMEM;
+
+		datalen = copy_from_iter(kdata, i->count, i);
+
+		rcu_read_lock_sched();
+
+		probe_func_ptr = rcu_dereference_sched(tp->funcs);
+
+		if (probe_func_ptr) {
+			do {
+				probe_func = probe_func_ptr->func;
+				tpdata = probe_func_ptr->data;
+				probe_func(user, kdata, datalen, tpdata);
+			} while ((++probe_func_ptr)->func);
+		}
+
+		rcu_read_unlock_sched();
+
+		kfree(kdata);
+	}
+
+	return ret;
+}
+
+static ssize_t user_events_write(struct file *file, const char __user *ubuf,
+				 size_t count, loff_t *ppos)
+{
+	struct iovec iov;
+	struct iov_iter i;
+
+	if (unlikely(*ppos != 0))
+		return -EFAULT;
+
+	if (unlikely(import_single_range(READ, (char *)ubuf, count, &iov, &i)))
+		return -EFAULT;
+
+	return user_events_write_core(file, &i);
+}
+
+static ssize_t user_events_write_iter(struct kiocb *kp, struct iov_iter *i)

kernel/trace/trace_events_user.c new file +24 -0

+	refs = rcu_dereference_sched(file->private_data);
+	rcu_read_unlock_sched();
+
+	if (refs) {
+		count = refs->count;
+
+		for (i = 0; i < count; ++i)
+			if (refs->events[i] == user)
+				return i;
+	}
+
+	size = sizeof(*refs) + (sizeof(struct user_event *) * (count + 1));
+
+	new_refs = kzalloc(size, GFP_KERNEL);
+
+	if (!new_refs)
+		return -ENOMEM;
+
+	new_refs->count = count + 1;
+
+	for (i = 0; i < count; ++i)
+		new_refs->events[i] = refs->events[i];
+
+	new_refs->events[i] = user;

Hi Beau,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rostedt-trace/for-next]
[also build test ERROR on shuah-kselftest/next linux/master linus/master v5.15 next-20211104] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Beau-Belgrave/user_events-Enable-user-processes-to-create-and-write-to-trace-events/20211105-010650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git for-next
config: powerpc-allmodconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 11.2.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/da0961ad45aa1192b47b8a80de6b17437434ae4a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Beau-Belgrave/user_events-Enable-user-processes-to-create-and-write-to-trace-events/20211105-010650 git checkout da0961ad45aa1192b47b8a80de6b17437434ae4a
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash kernel/trace/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

kernel/trace/trace_events_user.c: In function 'user_event_parse':

Quoted 1 line

Quoted 1 line

kernel/trace/trace_events_user.c:665:9: error: too few arguments to function 'dyn_event_add'

     665 |         dyn_event_add(&user->devent);
         |         ^~~~~~~~~~~~~

In file included from kernel/trace/trace_events_user.c:23:
kernel/trace/trace_dynevent.h:79:19: note: declared here
79 | static inline int dyn_event_add(struct dyn_event *ev,
| ^~~~~~~~~~~~~

Quoted 30 lines · On Fri, 5 Nov 2021 05:34:31 +0800 kernel test robot <[email protected]> wrote:

Hi Beau,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rostedt-trace/for-next]
[also build test ERROR on shuah-kselftest/next linux/master linus/master v5.15 next-20211104] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Beau-Belgrave/user_events-Enable-user-processes-to-create-and-write-to-trace-events/20211105-010650
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git for-next
config: powerpc-allmodconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 11.2.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/da0961ad45aa1192b47b8a80de6b17437434ae4a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Beau-Belgrave/user_events-Enable-user-processes-to-create-and-write-to-trace-events/20211105-010650 git checkout da0961ad45aa1192b47b8a80de6b17437434ae4a
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash kernel/trace/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

Quoted 8 lines

kernel/trace/trace_events_user.c: In function 'user_event_parse':

Quoted 1 line

Quoted 1 line

kernel/trace/trace_events_user.c:665:9: error: too few arguments to function 'dyn_event_add'

     665 |         dyn_event_add(&user->devent);
         |         ^~~~~~~~~~~~~

In file included from kernel/trace/trace_events_user.c:23:
kernel/trace/trace_dynevent.h:79:19: note: declared here
79 | static inline int dyn_event_add(struct dyn_event *ev,
| ^~~~~~~~~~~~~

You need to pass &user->call too :)

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

Hi Beau,

At first, thanks for breaking down your patch into this series!

Now I found that a suspicious security design issue in this patch.

kernel/trace/trace_events_user.c line 342

Quoted 8 lines · On Thu, 4 Nov 2021 10:04:25 -0700 Beau Belgrave <[email protected]>…

kernel/trace/trace_events_user.c new file +8 -0

+
+static enum print_line_t user_event_print_trace(struct trace_iterator *iter,
+						int flags,
+						struct trace_event *event)
+{
+	/* Unsafe to try to decode user provided print_fmt, use hex */
+	trace_print_hex_dump_seq(&iter->seq, "", DUMP_PREFIX_OFFSET, 16,
+				 1, iter->ent, iter->ent_size, true);

You said this is "Unsafe to try to decode user provided" here, because this doesn't check the event data sanity, especially non-fixed size data.

However, it is not enough that you don't decode it here. Because synthetic events (histograms) and event filters need to decode this recorded data entry using the event format information.

This means this can cause a buffer overrun issue on the ring buffer, because __data_loc (and __rel_loc too) can be compromised by the user.

If you want to just trace the user events with digit parameters, there is a way to close this issue - support only the fixed size types (IOW, drop __data_loc/rel_loc support) and always checks the 'length' of the written data size. This ensures that those filters/synthetic events can access those parameters as 'values'. Maybe eprobes still has to reject the user events but the other parts will work well.

If you want to log some "string", it is hard. Maybe it needs a special check when writing the event (address, length, and null termination.), but it will increase the recording overhead.

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

Does that mean the decoders in eprobes/histogram don't check event record sizes before accessing the data? Shouldn't that get fix centrally? That would mean a loaded module could do the same thing (user_events only works if the user has access to tracefs, so it's not like it's open to all users).

kernel/trace/trace_events_user.c line 123

Quoted 11 lines · On Thu, 4 Nov 2021 10:04:25 -0700 Beau Belgrave <[email protected]>…

kernel/trace/trace_events_user.c new file +11 -0

+static int user_field_array_size(const char *type)
+{
+	const char *start = strchr(type, '[');
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;
+
+	start++;
+
+	while (*start >= '0' && *start <= '9')

The kernel has include/linux/ctype.h

while (isdigit(*start))

kernel/trace/trace_events_user.c line 124

Quoted 1 line

kernel/trace/trace_events_user.c new file +1 -0

+		size = (size * 10) + (*start++ - '0');

So you only allow decimal digits? No hex?

Also, is there anything to check if the size overflows?

I'm assuming that other patches will add checking if the size is greater than some amount?

Quoted 9 lines

kernel/trace/trace_events_user.c new file +9 -0

+/*
+ * Parses the values of a field within the description
+ * Format: type name [size]
+ */
+static int user_event_parse_field(char *field, struct user_event *user,
+				  u32 *offset)
+{
+	char *part, *type, *name;
+	u32 depth = 0, saved_offset = *offset;

kernel/trace/trace_events_user.c line 228

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	int size = -EINVAL;
+	bool is_struct = false;
+
+	field = skip_spaces(field);
+
+	if (*field == 0)
+		return 0;
+
+	/* Handle types that have a space within */
+	if (strstr(field, "unsigned ") == field) {

These should use str_has_prefix(field, "unsigned ") etc.

It also returns the length of the prefix so you don't need to add sizeof() of the string you checked for.

kernel/trace/trace_events_user.c line 334

Quoted 4 lines

kernel/trace/trace_events_user.c new file +4 -0

+
+static struct trace_event_fields user_event_fields_array[] = {
+	{}
+};

Isn't the above just a fancy way of writing:

static struct trace_event_fields user_event_fields_array[1];

?

Quoted 13 lines

kernel/trace/trace_events_user.c new file +13 -0

+/*
+ * Writes the user supplied payload out to a trace file.
+ */
+static void user_event_ftrace(struct user_event *user, void *data, u32 datalen,
+			      void *tpdata)
+{
+	struct trace_event_file *file;
+	struct trace_entry *entry;
+	struct trace_event_buffer event_buffer;
+
+	file = (struct trace_event_file *)tpdata;
+
+	if (!file ||

kernel/trace/trace_events_user.c line 411

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	    !(file->flags & EVENT_FILE_FL_ENABLED) ||
+	    trace_trigger_soft_disabled(file))
+		return;
+
+	entry = trace_event_buffer_reserve(&event_buffer, file,
+					   sizeof(*entry) + datalen);
+
+	if (unlikely(!entry))
+		return;
+

Might want to add a comment here that the trace_event_buffer_reserve() will fill in the struct trace_entry, which explains the "entry+1" below.

I also need to add comments to trace_event_buffer_reserve() that it does so :-p

Quoted 27 lines

kernel/trace/trace_events_user.c new file +27 -0

+/*
+ * Update the register page that is shared between user processes.
+ */
+static void update_reg_page_for(struct user_event *user)
+{
+	struct tracepoint *tp = &user->tracepoint;
+	char status = 0;
+
+	if (atomic_read(&tp->key.enabled) > 0) {
+		struct tracepoint_func *probe_func_ptr;
+		user_event_func_t probe_func;
+
+		rcu_read_lock_sched();
+
+		probe_func_ptr = rcu_dereference_sched(tp->funcs);
+
+		if (probe_func_ptr) {
+			do {
+				probe_func = probe_func_ptr->func;
+
+				if (probe_func == user_event_ftrace)
+					status |= EVENT_STATUS_FTRACE;
+				else
+					status |= EVENT_STATUS_OTHER;
+			} while ((++probe_func_ptr)->func);
+		}
+

kernel/trace/trace_events_user.c line 447

Quoted 4 lines

kernel/trace/trace_events_user.c new file +4 -0

+		rcu_read_unlock_sched();
+	}
+
+	register_page_data[user->index] = status;

Should there be some kind of memory barriers here? That is, isn't this the page that user space sees? The user space code should probably have some kind of read memory barrier as well.

kernel/trace/trace_events_user.c line 512

Quoted 13 lines

kernel/trace/trace_events_user.c new file +13 -0

+static int user_event_create(const char *raw_command)
+{
+	struct user_event *user;
+	char *name;
+	int ret;
+
+	if (strstr(raw_command, USER_EVENTS_PREFIX) != raw_command)
+		return -ECANCELED;
+
+	raw_command += USER_EVENTS_PREFIX_LEN;
+	raw_command = skip_spaces(raw_command);
+
+	name = kstrdup(raw_command, GFP_KERNEL);

name is allocated here, it really needs to be freed in this function as well. I see that user_event_parse() will free it, but that is extremely error prone to have a dependency like that. If name needs to be saved by user_event_parse_cmd() then that should be shown in the return value of that function. And if it fails the freeing of name should be in this function. Also, if it is saved, then there should be a comment in this function stating that.

Quoted 5 lines

kernel/trace/trace_events_user.c new file +5 -0

+static bool user_event_is_busy(struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+
+	return atomic_read(&user->refcnt) != 0;

kernel/trace/trace_events_user.c line 559

Quoted 6 lines

kernel/trace/trace_events_user.c new file +6 -0

+}
+
+static int user_event_free(struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+

Shouldn't this check if the event is busy first?

Quoted 21 lines

kernel/trace/trace_events_user.c new file +21 -0

+/*
+ * Validates the user payload and writes via iterator.
+ */
+static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
+{
+	struct user_event_refs *refs;
+	struct user_event *user = NULL;
+	struct tracepoint *tp;
+	ssize_t ret = i->count;
+	int idx;
+
+	if (unlikely(copy_from_iter(&idx, sizeof(idx), i) != sizeof(idx)))
+		return -EFAULT;
+
+	rcu_read_lock_sched();
+
+	refs = rcu_dereference_sched(file->private_data);
+
+	if (likely(refs && idx < refs->count))
+		user = refs->events[idx];
+

kernel/trace/trace_events_user.c line 727

Quoted 6 lines

kernel/trace/trace_events_user.c new file +6 -0

+	rcu_read_unlock_sched();
+
+	if (unlikely(user == NULL))
+		return -ENOENT;
+
+	tp = &user->tracepoint;

What protects user here? You released the rcu lock.

kernel/trace/trace_events_user.c line 736

Quoted 10 lines

kernel/trace/trace_events_user.c new file +9 -0

+
+	if (likely(atomic_read(&tp->key.enabled) > 0)) {
+		struct tracepoint_func *probe_func_ptr;
+		user_event_func_t probe_func;
+		void *tpdata;
+		void *kdata;
+		u32 datalen;
+
+		kdata = kmalloc(i->count, GFP_KERNEL);

The allocation would need to be done first, before grabbing the rcu lock, and all this would need to be done within the rcu lock, because "tp" may not even exist anymore.

kernel/trace/trace_events_user.c line 788

Quoted 6 lines

kernel/trace/trace_events_user.c new file +6 -0

+static int user_events_ref_add(struct file *file, struct user_event *user)
+{
+	struct user_event_refs *refs, *new_refs;
+	int i, size, count = 0;
+
+	rcu_read_lock_sched();

rcu lock is not needed, but you may want to use:

rcu_dereference_protected()

and list the lock that protects the modifications here.

I just skimmed the rest, and didn't see anything that stuck out.

But maybe I'll get time to look deeper at it later.

-- Steve

Quoted 5 lines · On Mon, 8 Nov 2021 09:13:36 -0800 Beau Belgrave <[email protected]>…

Does that mean the decoders in eprobes/histogram don't check event record sizes before accessing the data? Shouldn't that get fix centrally? That would mean a loaded module could do the same thing (user_events only works if the user has access to tracefs, so it's not like it's open to all users).

There's checks to make sure everything fits in eprobes and kprobes. If it doesn't then the event is simply dropped.

For example, if you look at __eprobe_trace_func() in trace_eprobe.c, you'll see that it calls get_eprobe_size(), which goes through and just reads what it is about to accept. Then it reserves the amount of data on the ring buffer, and then calls store_trace_args() which also passes in the size that it found, in case things change. If it's too big, it only records what it originally intended.

-- Steve

Quoted 16 lines · Steven Rostedt

kernel/trace/trace_events_user.c line 123

Quoted 11 lines · On Thu, 4 Nov 2021 10:04:25 -0700 Beau Belgrave <[email protected]>…

kernel/trace/trace_events_user.c new file +11 -0

+static int user_field_array_size(const char *type)
+{
+	const char *start = strchr(type, '[');
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;
+
+	start++;
+
+	while (*start >= '0' && *start <= '9')  

The kernel has include/linux/ctype.h

Quoted 6 lines

while (isdigit(*start))

kernel/trace/trace_events_user.c line 124

Quoted 1 line

kernel/trace/trace_events_user.c new file +1 -0

+		size = (size * 10) + (*start++ - '0');  

So you only allow decimal digits? No hex?

Happy to change it, I only expected decimal to be allowed.

Is there a strong need for hex? (IE: kstrtouint(start, 0, ...)?

Quoted 2 lines

Also, is there anything to check if the size overflows?

Quoted 3 lines

I'm assuming that other patches will add checking if the size is greater than some amount?

I can switch to kstrtouint and use a max check, however:
The max checks are weird here because eBPF has no limits, while ftrace and perf both do (and I believe they are different max sizes?)

If someone really wanted a large array of characters and it can fit within perf but not ftrace, is it correct to block it here? My thinking was to allow each trace buffer reserve call to either work or not based on what the user requested depending on what was hooked.

No strong opinion here though, just thoughts about what is a reasonable max given the 3 technologies.

Quoted 23 lines

Quoted 10 lines

kernel/trace/trace_events_user.c new file +9 -0

+/*
+ * Parses the values of a field within the description
+ * Format: type name [size]
+ */
+static int user_event_parse_field(char *field, struct user_event *user,
+				  u32 *offset)
+{
+	char *part, *type, *name;
+	u32 depth = 0, saved_offset = *offset;

kernel/trace/trace_events_user.c line 228

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	int size = -EINVAL;
+	bool is_struct = false;
+
+	field = skip_spaces(field);
+
+	if (*field == 0)
+		return 0;
+
+	/* Handle types that have a space within */
+	if (strstr(field, "unsigned ") == field) {  

These should use str_has_prefix(field, "unsigned ") etc.

Quoted 3 lines

It also returns the length of the prefix so you don't need to add sizeof() of the string you checked for.

Nice, will use that.

Quoted 7 lines

kernel/trace/trace_events_user.c line 334

Quoted 4 lines

kernel/trace/trace_events_user.c new file +4 -0

+
+static struct trace_event_fields user_event_fields_array[] = {
+	{}
+};  

Isn't the above just a fancy way of writing:

Quoted 4 lines

static struct trace_event_fields user_event_fields_array[1];

?

Yes, as long as it gets init'd to zero. My understanding was that {} would force zeroing, but I could totally be wrong.

Quoted 28 lines

Quoted 14 lines

kernel/trace/trace_events_user.c new file +13 -0

+/*
+ * Writes the user supplied payload out to a trace file.
+ */
+static void user_event_ftrace(struct user_event *user, void *data, u32 datalen,
+			      void *tpdata)
+{
+	struct trace_event_file *file;
+	struct trace_entry *entry;
+	struct trace_event_buffer event_buffer;
+
+	file = (struct trace_event_file *)tpdata;
+
+	if (!file ||

kernel/trace/trace_events_user.c line 411

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	    !(file->flags & EVENT_FILE_FL_ENABLED) ||
+	    trace_trigger_soft_disabled(file))
+		return;
+
+	entry = trace_event_buffer_reserve(&event_buffer, file,
+					   sizeof(*entry) + datalen);
+
+	if (unlikely(!entry))
+		return;
+  

Might want to add a comment here that the trace_event_buffer_reserve() will fill in the struct trace_entry, which explains the "entry+1" below.

Quoted 2 lines

I also need to add comments to trace_event_buffer_reserve() that it does so :-p

Will do.

Quoted 27 lines

Quoted 27 lines

kernel/trace/trace_events_user.c new file +27 -0

+/*
+ * Update the register page that is shared between user processes.
+ */
+static void update_reg_page_for(struct user_event *user)
+{
+	struct tracepoint *tp = &user->tracepoint;
+	char status = 0;
+
+	if (atomic_read(&tp->key.enabled) > 0) {
+		struct tracepoint_func *probe_func_ptr;
+		user_event_func_t probe_func;
+
+		rcu_read_lock_sched();
+
+		probe_func_ptr = rcu_dereference_sched(tp->funcs);
+
+		if (probe_func_ptr) {
+			do {
+				probe_func = probe_func_ptr->func;
+
+				if (probe_func == user_event_ftrace)
+					status |= EVENT_STATUS_FTRACE;
+				else
+					status |= EVENT_STATUS_OTHER;
+			} while ((++probe_func_ptr)->func);
+		}
+

Quoted 9 lines

kernel/trace/trace_events_user.c line 447

Quoted 4 lines

kernel/trace/trace_events_user.c new file +4 -0

+		rcu_read_unlock_sched();
+	}
+
+	register_page_data[user->index] = status;  

Should there be some kind of memory barriers here? That is, isn't this the page that user space sees? The user space code should probably have some kind of read memory barrier as well.

I'm glad you brought this up. I wanted to ensure a balance between eventual enablement of the event in the user mode process vs the cost of simultaneous enablement of the event (stalls, etc).

We haven't seen this become an issue for our teams in our other telemetry sources (with no barriers), which seems to indicate eventual agreement of the page data works well as a tradeoff.

Quoted 15 lines

Quoted 4 lines

kernel/trace/trace_events_user.c new file +3 -0

+static int user_event_create(const char *raw_command)
+{
+	struct user_event *user;

kernel/trace/trace_events_user.c line 512

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	char *name;
+	int ret;
+
+	if (strstr(raw_command, USER_EVENTS_PREFIX) != raw_command)
+		return -ECANCELED;
+
+	raw_command += USER_EVENTS_PREFIX_LEN;
+	raw_command = skip_spaces(raw_command);
+
+	name = kstrdup(raw_command, GFP_KERNEL);  

Quoted 8 lines

name is allocated here, it really needs to be freed in this function as well. I see that user_event_parse() will free it, but that is extremely error prone to have a dependency like that. If name needs to be saved by user_event_parse_cmd() then that should be shown in the return value of that function. And if it fails the freeing of name should be in this function. Also, if it is saved, then there should be a comment in this function stating that.

It's a bit tricky, because if the event already exists, the name is freed. If the function fails, the name is freed. If the event has never been seen before then it is saved.

I'll try to make this more clear, my thought is to add an explicit argument that gets set back to the caller if the string should be freed or not to make this clear while reading the code.

Quoted 5 lines

Quoted 5 lines

kernel/trace/trace_events_user.c new file +5 -0

+static bool user_event_is_busy(struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+
+	return atomic_read(&user->refcnt) != 0;

Quoted 8 lines

kernel/trace/trace_events_user.c line 559

Quoted 6 lines

kernel/trace/trace_events_user.c new file +6 -0

+}
+
+static int user_event_free(struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+  

Shouldn't this check if the event is busy first?

Yes, you are right. In the release all case busy is checked by dyn_events for me. However, in the individual case it is not. I'll fix this.

Quoted 21 lines

Quoted 21 lines

kernel/trace/trace_events_user.c new file +21 -0

+/*
+ * Validates the user payload and writes via iterator.
+ */
+static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
+{
+	struct user_event_refs *refs;
+	struct user_event *user = NULL;
+	struct tracepoint *tp;
+	ssize_t ret = i->count;
+	int idx;
+
+	if (unlikely(copy_from_iter(&idx, sizeof(idx), i) != sizeof(idx)))
+		return -EFAULT;
+
+	rcu_read_lock_sched();
+
+	refs = rcu_dereference_sched(file->private_data);
+
+	if (likely(refs && idx < refs->count))
+		user = refs->events[idx];
+

Quoted 8 lines

kernel/trace/trace_events_user.c line 727

Quoted 6 lines

kernel/trace/trace_events_user.c new file +6 -0

+	rcu_read_unlock_sched();
+
+	if (unlikely(user == NULL))
+		return -ENOENT;
+
+	tp = &user->tracepoint;  

What protects user here? You released the rcu lock.

user is ref counted by the file, so before the final close all events linked to the file via the reg ioctl are ref'd and cannot go away.

Quoted 11 lines

Quoted 11 lines

kernel/trace/trace_events_user.c new file +9 -0

+
+	if (likely(atomic_read(&tp->key.enabled) > 0)) {
+		struct tracepoint_func *probe_func_ptr;
+		user_event_func_t probe_func;
+		void *tpdata;
+		void *kdata;
+		u32 datalen;
+
+		kdata = kmalloc(i->count, GFP_KERNEL);  

It seems there are 2 concerns:
1. If data comes in and it's not in the size that is specified, it's suspicious and should either be truncated or ignored. Maybe under ignore, over truncate.

2. If the data is more than specified, it must be checked to see if there are __data_loc / __rel_loc entries and they must be validated as within range of accepted limits. If there are no __data_loc / __rel_loc it should either be truncated or ignored.

Is there more that I may have missed?

I'd like to know if I do fix them that the features like filtering will still be available to user_events or if it's better to just add flags to disable kernel filtering?

I'm still unsure this is limited to just user_events.

For example, why doesn't filter_pred_strloc and filter_pred_pchar in trace_events_filter.c check the boundary it will be accessing?

It seems like tracepoints from kernel modules, while more trusted, can also cause this kind of thing due to bugs, etc.

Quoted 24 lines · On Mon, 8 Nov 2021 11:56:42 -0800 Beau Belgrave <[email protected]>…

Quoted 22 lines · Steven Rostedt

kernel/trace/trace_events_user.c line 123

Quoted 11 lines · On Thu, 4 Nov 2021 10:04:25 -0700 Beau Belgrave <[email protected]>…

kernel/trace/trace_events_user.c new file +11 -0

+static int user_field_array_size(const char *type)
+{
+	const char *start = strchr(type, '[');
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;
+
+	start++;
+
+	while (*start >= '0' && *start <= '9')  

The kernel has include/linux/ctype.h

while (isdigit(*start))

kernel/trace/trace_events_user.c line 124

Quoted 1 line

kernel/trace/trace_events_user.c new file +1 -0

+		size = (size * 10) + (*start++ - '0');  

So you only allow decimal digits? No hex?

Quoted 1 line

Happy to change it, I only expected decimal to be allowed.

I'm more worried that if the output of the "registered events" may have a hex number, and someone uses that output to recreate the event.

Quoted 9 lines

Is there a strong need for hex? (IE: kstrtouint(start, 0, ...)?

Quoted 5 lines

Also, is there anything to check if the size overflows?

I'm assuming that other patches will add checking if the size is greater than some amount?

Quoted 3 lines

I can switch to kstrtouint and use a max check, however:
The max checks are weird here because eBPF has no limits, while ftrace and perf both do (and I believe they are different max sizes?)

I'm not concerned about taking up more than ftrace or perf, but having some kind of DOS that is caused by hugh allocations. If you make a huge event and try to record it in ftrace, then ftrace will simply drop the event. No harm done (except you won't see the event).

Quoted 6 lines

If someone really wanted a large array of characters and it can fit within perf but not ftrace, is it correct to block it here? My thinking was to allow each trace buffer reserve call to either work or not based on what the user requested depending on what was hooked.

Quoted 2 lines

No strong opinion here though, just thoughts about what is a reasonable max given the 3 technologies.

Again, I'm more worried about just a general 'this is way too big' thing. If anything, it could be simply to flag a bug where the event in created via some logic that goes crazy.

Quoted 36 lines

Quoted 23 lines

Quoted 10 lines

kernel/trace/trace_events_user.c new file +9 -0

+/*
+ * Parses the values of a field within the description
+ * Format: type name [size]
+ */
+static int user_event_parse_field(char *field, struct user_event *user,
+				  u32 *offset)
+{
+	char *part, *type, *name;
+	u32 depth = 0, saved_offset = *offset;

kernel/trace/trace_events_user.c line 228

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	int size = -EINVAL;
+	bool is_struct = false;
+
+	field = skip_spaces(field);
+
+	if (*field == 0)
+		return 0;
+
+	/* Handle types that have a space within */
+	if (strstr(field, "unsigned ") == field) {  

These should use str_has_prefix(field, "unsigned ") etc.

Quoted 3 lines

It also returns the length of the prefix so you don't need to add sizeof() of the string you checked for.

Nice, will use that.

Quoted 7 lines

kernel/trace/trace_events_user.c line 334

Quoted 4 lines

kernel/trace/trace_events_user.c new file +4 -0

+
+static struct trace_event_fields user_event_fields_array[] = {
+	{}
+};  

Isn't the above just a fancy way of writing:

Quoted 7 lines

Quoted 4 lines

static struct trace_event_fields user_event_fields_array[1];

?

Yes, as long as it gets init'd to zero. My understanding was that {} would force zeroing, but I could totally be wrong.

All static and global variables that are not assigned will be initialized to zero. No need to initialize anything that is zero by default.

Quoted 70 lines

Quoted 28 lines

Quoted 14 lines

kernel/trace/trace_events_user.c new file +13 -0

+/*
+ * Writes the user supplied payload out to a trace file.
+ */
+static void user_event_ftrace(struct user_event *user, void *data, u32 datalen,
+			      void *tpdata)
+{
+	struct trace_event_file *file;
+	struct trace_entry *entry;
+	struct trace_event_buffer event_buffer;
+
+	file = (struct trace_event_file *)tpdata;
+
+	if (!file ||

kernel/trace/trace_events_user.c line 411

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	    !(file->flags & EVENT_FILE_FL_ENABLED) ||
+	    trace_trigger_soft_disabled(file))
+		return;
+
+	entry = trace_event_buffer_reserve(&event_buffer, file,
+					   sizeof(*entry) + datalen);
+
+	if (unlikely(!entry))
+		return;
+  

Might want to add a comment here that the trace_event_buffer_reserve() will fill in the struct trace_entry, which explains the "entry+1" below.

Quoted 2 lines

I also need to add comments to trace_event_buffer_reserve() that it does so :-p

Will do.

Quoted 27 lines

Quoted 27 lines

kernel/trace/trace_events_user.c new file +27 -0

+/*
+ * Update the register page that is shared between user processes.
+ */
+static void update_reg_page_for(struct user_event *user)
+{
+	struct tracepoint *tp = &user->tracepoint;
+	char status = 0;
+
+	if (atomic_read(&tp->key.enabled) > 0) {
+		struct tracepoint_func *probe_func_ptr;
+		user_event_func_t probe_func;
+
+		rcu_read_lock_sched();
+
+		probe_func_ptr = rcu_dereference_sched(tp->funcs);
+
+		if (probe_func_ptr) {
+			do {
+				probe_func = probe_func_ptr->func;
+
+				if (probe_func == user_event_ftrace)
+					status |= EVENT_STATUS_FTRACE;
+				else
+					status |= EVENT_STATUS_OTHER;
+			} while ((++probe_func_ptr)->func);
+		}
+

Quoted 9 lines

kernel/trace/trace_events_user.c line 447

Quoted 4 lines

kernel/trace/trace_events_user.c new file +4 -0

+		rcu_read_unlock_sched();
+	}
+
+	register_page_data[user->index] = status;  

Should there be some kind of memory barriers here? That is, isn't this the page that user space sees? The user space code should probably have some kind of read memory barrier as well.

Quoted 7 lines

I'm glad you brought this up. I wanted to ensure a balance between eventual enablement of the event in the user mode process vs the cost of simultaneous enablement of the event (stalls, etc).

We haven't seen this become an issue for our teams in our other telemetry sources (with no barriers), which seems to indicate eventual agreement of the page data works well as a tradeoff.

Another approach is if you know what tasks these events come from (who registered them), then you could simply send an IPI to the task if it happen to be running. An IPI will force a memory barrier, and since you only need to do this on changes (enable / disable event) it's not like it will happen often.

Quoted 24 lines

Quoted 15 lines

Quoted 4 lines

kernel/trace/trace_events_user.c new file +3 -0

+static int user_event_create(const char *raw_command)
+{
+	struct user_event *user;

kernel/trace/trace_events_user.c line 512

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+	char *name;
+	int ret;
+
+	if (strstr(raw_command, USER_EVENTS_PREFIX) != raw_command)
+		return -ECANCELED;
+
+	raw_command += USER_EVENTS_PREFIX_LEN;
+	raw_command = skip_spaces(raw_command);
+
+	name = kstrdup(raw_command, GFP_KERNEL);  

Quoted 8 lines

name is allocated here, it really needs to be freed in this function as well. I see that user_event_parse() will free it, but that is extremely error prone to have a dependency like that. If name needs to be saved by user_event_parse_cmd() then that should be shown in the return value of that function. And if it fails the freeing of name should be in this function. Also, if it is saved, then there should be a comment in this function stating that.

Quoted 3 lines

It's a bit tricky, because if the event already exists, the name is freed. If the function fails, the name is freed. If the event has never been seen before then it is saved.

I would then suggest to free the name from the calling function only if it succeeds and the name already exists. With a comment stating that.

But I would have the caller free it on failure. That is, you could change user_event_parse() to have:

	struct user_event *user = find_user_event(name, &key);

	if (user) {
		*newuser = user;
		/*
		 * The name is allocated by the caller, but since it
		 * already exists in user, simply free it here.
		 */
		kfree(name);
		return 0;
	}

And remove all the "put_name" jumps.

Quoted 53 lines

I'll try to make this more clear, my thought is to add an explicit argument that gets set back to the caller if the string should be freed or not to make this clear while reading the code.

Quoted 5 lines

Quoted 5 lines

kernel/trace/trace_events_user.c new file +5 -0

+static bool user_event_is_busy(struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+
+	return atomic_read(&user->refcnt) != 0;

Quoted 8 lines

kernel/trace/trace_events_user.c line 559

Quoted 6 lines

kernel/trace/trace_events_user.c new file +6 -0

+}
+
+static int user_event_free(struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+  

Shouldn't this check if the event is busy first?

Yes, you are right. In the release all case busy is checked by dyn_events for me. However, in the individual case it is not. I'll fix this.

Quoted 29 lines

Quoted 17 lines

kernel/trace/trace_events_user.c new file +17 -0

+/*
+ * Validates the user payload and writes via iterator.
+ */
+static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
+{
+	struct user_event_refs *refs;
+	struct user_event *user = NULL;
+	struct tracepoint *tp;
+	ssize_t ret = i->count;
+	int idx;
+
+	if (unlikely(copy_from_iter(&idx, sizeof(idx), i) != sizeof(idx)))
+		return -EFAULT;
+
+	rcu_read_lock_sched();
+
+	refs = rcu_dereference_sched(file->private_data);

kernel/trace/trace_events_user.c line 727

Quoted 10 lines

kernel/trace/trace_events_user.c new file +10 -0

+
+	if (likely(refs && idx < refs->count))
+		user = refs->events[idx];
+
+	rcu_read_unlock_sched();
+
+	if (unlikely(user == NULL))
+		return -ENOENT;
+
+	tp = &user->tracepoint;  

What protects user here? You released the rcu lock.

Quoted 2 lines

user is ref counted by the file, so before the final close all events linked to the file via the reg ioctl are ref'd and cannot go away.

If that's the case, then why the rcu_read_lock_sched() around the assigment of user?

-- Steve

Addes print_fmt format generation for basic types that are supported for user processes. Only supports sizes that are the same on 32 and 64 bit.

Signed-off-by Beau Belgrave <[email protected]>

---
 kernel/trace/trace_events_user.c | 108 ++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 2 deletions(-)

kernel/trace/trace_events_user.c +0 -0

index a68017ad7fdd..479a9ced3281 100644

kernel/trace/trace_events_user.c +30 -0

+{
+	if (strcmp(type, "s64") == 0)
+		return "%lld";
+	if (strcmp(type, "u64") == 0)
+		return "%llu";
+	if (strcmp(type, "s32") == 0)
+		return "%d";
+	if (strcmp(type, "u32") == 0)
+		return "%u";
+	if (strcmp(type, "int") == 0)
+		return "%d";
+	if (strcmp(type, "unsigned int") == 0)
+		return "%u";
+	if (strcmp(type, "s16") == 0)
+		return "%d";
+	if (strcmp(type, "u16") == 0)
+		return "%u";
+	if (strcmp(type, "short") == 0)
+		return "%d";
+	if (strcmp(type, "unsigned short") == 0)
+		return "%u";
+	if (strcmp(type, "s8") == 0)
+		return "%d";
+	if (strcmp(type, "u8") == 0)
+		return "%u";
+	if (strcmp(type, "char") == 0)
+		return "%d";
+	if (strcmp(type, "unsigned char") == 0)
+		return "%u";
+	if (strstr(type, "char[") != 0)

kernel/trace/trace_events_user.c +65 -2

+		if (strstr(type, "char[") != 0)
+			return true;
+	}
+
+	return false;
+}
+
+#define LEN_OR_ZERO (len ? len - pos : 0)
+static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
+{
+	struct ftrace_event_field *field, *next;
+	struct list_head *head = &user->fields;
+	int pos = 0, depth = 0;
+
+	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
+
+	list_for_each_entry_safe_reverse(field, next, head, link) {
+		if (depth != 0)
+			pos += snprintf(buf + pos, LEN_OR_ZERO, " ");
+
+		pos += snprintf(buf + pos, LEN_OR_ZERO, "%s=%s",
+				field->name, user_field_format(field->type));
+
+		depth++;
+	}
+
+	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
+
+	list_for_each_entry_safe_reverse(field, next, head, link) {
+		if (user_field_is_dyn_string(field->type))
+			pos += snprintf(buf + pos, LEN_OR_ZERO,
+					", __get_str(%s)", field->name);
+		else
+			pos += snprintf(buf + pos, LEN_OR_ZERO,
+					", REC->%s", field->name);
+	}
+
+	return pos + 1;
+}
+#undef LEN_OR_ZERO
+
+static int user_event_create_print_fmt(struct user_event *user)
+{
+	char *print_fmt;
+	int len;
+
+	len = user_event_set_print_fmt(user, NULL, 0);
+
+	print_fmt = kmalloc(len, GFP_KERNEL);
+
+	if (!print_fmt)
+		return -ENOMEM;
+
+	user_event_set_print_fmt(user, print_fmt, len);
+
+	user->call.print_fmt = print_fmt;
+
+	return 0;
+}
+
 static struct trace_event_fields user_event_fields_array[] = {
 	{}
 };

@@ -366,6 +467,7 @@ static int destroy_user_event(struct user_event *user)
 	clear_bit(user->index, page_bitmap);
 	hash_del(&user->node);
 
+	kfree(user->call.print_fmt);
 	kfree(EVENT_NAME(user));
 	kfree(user);
 

@@ -637,8 +739,10 @@ static int user_event_parse(char *name, char *args, char *flags,
 	if (ret)
 		goto put_user;
 
-	/* Minimal print format */
-	user->call.print_fmt = "\"\"";
+	ret = user_event_create_print_fmt(user);
+
+	if (ret)
+		goto put_user;
 
 	user->call.data = user;
 	user->call.class = &user->class;