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

Define the basic structs and ioctl commands that allow user processes to interact with user_events.

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

---
 include/uapi/linux/user_events.h | 68 ++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 include/uapi/linux/user_events.h

include/uapi/linux/user_events.h new file +68 -0

new file mode 100644
index 000000000000..5bff99418deb
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2021, Microsoft Corporation.
+ *
+ * Authors:
+ *   Beau Belgrave <[email protected]>
+ */
+#ifndef _UAPI_LINUX_USER_EVENTS_H
+#define _UAPI_LINUX_USER_EVENTS_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#ifdef __KERNEL__
+#include <linux/uio.h>
+#else
+#include <sys/uio.h>
+#endif
+
+#define USER_EVENTS_SYSTEM "user_events"
+#define USER_EVENTS_PREFIX "u:"
+
+/* Bits 0-6 are for known probe types, Bit 7 is for unknown probes */
+#define EVENT_BIT_FTRACE 0
+#define EVENT_BIT_PERF 1
+#define EVENT_BIT_OTHER 7
+
+#define EVENT_STATUS_FTRACE (1 << EVENT_BIT_FTRACE)
+#define EVENT_STATUS_PERF (1 << EVENT_BIT_PERF)
+#define EVENT_STATUS_OTHER (1 << EVENT_BIT_OTHER)
+
+/* Use raw iterator for attached BPF program(s), no affect on ftrace/perf */
+#define FLAG_BPF_ITER (1 << 0)
+
+struct user_reg {
+	__u32 size;
+	__u64 name_args;
+	__u32 status_index;
+	__u32 write_index;
+};
+
+#define DIAG_IOC_MAGIC '*'
+#define DIAG_IOCSREG _IOWR(DIAG_IOC_MAGIC, 0, struct user_reg*)
+#define DIAG_IOCSDEL _IOW(DIAG_IOC_MAGIC, 1, char*)
+
+enum {
+	USER_BPF_DATA_KERNEL,
+	USER_BPF_DATA_USER,
+	USER_BPF_DATA_ITER,
+};
+
+struct user_bpf_iter {
+	__u32 iov_offset;
+	__u32 nr_segs;
+	const struct iovec *iov;
+};
+
+struct user_bpf_context {
+	__u32 data_type;
+	__u32 data_len;
+	union {
+		void *kdata;
+		void *udata;
+		struct user_bpf_iter *iter;
+	};
+};
+
+#endif /* _UAPI_LINUX_USER_EVENTS_H */

Signature · 1 line

2.17.1

Quoted 3 lines · On Thu, 9 Dec 2021 14:31:58 -0800 Beau Belgrave <[email protected]>…

Define the basic structs and ioctl commands that allow user processes to interact with user_events.

IMHO, a basic part of this should be integrated with the [2/13] and other parts are incrementaly added with the patch which actually use that data structure or definition, so that it can be bisected cleanly.
(because there is no reason to introduce only this header.)

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

+#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
+
+/*

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

+
+static int user_field_array_size(const char *type)
+{
+	const char *start = strchr(type, '[');
+	char val[8];
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;

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

+
+	if (kstrtouint(val, 0, &size))
+		return -EINVAL;
+
+	if (size > MAX_FIELD_ARRAY_SIZE)
+		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 (str_has_prefix(type, "char["))
+		return user_field_array_size(type);
+	if (str_has_prefix(type, "unsigned char["))
+		return user_field_array_size(type);
+	if (str_has_prefix(type, "__data_loc "))
+		return sizeof(u32);
+	if (str_has_prefix(type, "__rel_loc "))
+		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)

+ */

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

+static int user_event_parse_field(char *field, struct user_event *user,
+				  u32 *offset)
+{
+	char *part, *type, *name;
+	u32 depth = 0, saved_offset = *offset;
+	int len, size = -EINVAL;
+	bool is_struct = false;
+
+	field = skip_spaces(field);
+
+	if (*field == 0)
+		return 0;
+
+	/* Handle types that have a space within */
+	len = str_has_prefix(field, "unsigned ");
+	if (len)
+		goto skip_next;
+
+	len = str_has_prefix(field, "struct ");
+	if (len) {
+		is_struct = true;
+		goto skip_next;
+	}
+
+	len = str_has_prefix(field, "__data_loc unsigned ");
+	if (len)
+		goto skip_next;
+
+	len = str_has_prefix(field, "__data_loc ");
+	if (len)
+		goto skip_next;
+
+	len = str_has_prefix(field, "__rel_loc unsigned ");
+	if (len)
+		goto skip_next;
+
+	len = str_has_prefix(field, "__rel_loc ");
+	if (len)
+		goto skip_next;
+
+	goto parse;
+skip_next:
+	type = field;
+	field = strpbrk(field + len, " ");
+
+	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);
+}

kernel/trace/trace_events_user.c new file +28 -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;

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

+	}
+
+	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;
+}
+

kernel/trace/trace_events_user.c new file +40 -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);
+
+	/*
+	 * The refs->events array is protected by RCU, and new items may be
+	 * added. But the user retrieved from indexing into the events array
+	 * shall be immutable while the file is opened.
+	 */
+	if (likely(refs && idx < refs->count))
+		user = refs->events[idx];
+
+	rcu_read_unlock_sched();
+
+	if (unlikely(user == NULL))
+		return -ENOENT;
+
+	tp = &user->tracepoint;
+
+	/*
+	 * It's possible key.enabled disables after this check, however
+	 * we don't mind if a few events are included in this condition.
+	 */
+	if (likely(atomic_read(&tp->key.enabled) > 0)) {
+		struct tracepoint_func *probe_func_ptr;
+		user_event_func_t probe_func;

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

+
+		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);

kernel/trace/trace_events_user.c line 41

Quoted 4 lines

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

+/* 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)
+#define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)  

I don't recommend to record the event which size is more than a page size... Maybe 256 entries?
It is also better to limit the total size of the event and the number of fields (arguments).

Steve, can we write such a big event data on the trace buffer?

Quoted 10 lines · On Fri, 10 Dec 2021 19:43:58 +0900 Masami Hiramatsu <[email protected]> wrote:

kernel/trace/trace_events_user.c line 41

Quoted 4 lines

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

+/* 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)
+#define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)  

I don't recommend to record the event which size is more than a page size... Maybe 256 entries?
It is also better to limit the total size of the event and the number of fields (arguments).

Quoted 1 line

Steve, can we write such a big event data on the trace buffer?

In the future yes!

https://lore.kernel.org/all/[email protected]/

But it will still require some configuration changes from user space. But that said, if the user wants to add a larger size, then they can do so (in the future).

Quoted 5 lines

Quoted 1 line

Steve, can we write such a big event data on the trace buffer?

In the future yes!

https://lore.kernel.org/all/[email protected]/

Ah, nice!

Quoted 4 lines

But it will still require some configuration changes from user space. But that said, if the user wants to add a larger size, then they can do so (in the future).

Hmm, so, at this moment I recommend to pick the max size of the event smaller than page size but enough large (e.g. 1024, that is finally Beau has chosen).
And after that new ring buffer introduced, expand it.
What would you think?

Hi Beau,

Thanks for updating the patch! I have some comments below.

On Thu, 9 Dec 2021 14:31:59 -0800
Beau Belgrave <[email protected]> wrote:

[..]

Quoted 7 lines

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

+#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
+
+/*

kernel/trace/trace_events_user.c line 41

Quoted 10 lines

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

+ * 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)
+#define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)

I don't recommend to record the event which size is more than a page size... Maybe 256 entries?
It is also better to limit the total size of the event and the number of fields (arguments).

Steve, can we write such a big event data on the trace buffer?

[..]

Quoted 13 lines

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

+
+static int user_field_array_size(const char *type)
+{
+	const char *start = strchr(type, '[');
+	char val[8];
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;
+
+	start++;
+
+	while (*start != ']' && size < (sizeof(val) - 1))

kernel/trace/trace_events_user.c line 144

Quoted 6 lines

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

+		val[size++] = *start++;
+
+	if (*start != ']')
+		return -EINVAL;
+
+	val[size] = 0;

It's '\0', not 0.

If I were you, I just use strlcpy(val, start, sizeof(val)), and strchr(val, ']'). Sometimes using standard libc function will be easer to understand what it does. :)

Quoted 14 lines

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

+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;

kernel/trace/trace_events_user.c line 228

Quoted 5 lines

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

+	field->size = size;
+	field->is_signed = is_signed;
+	field->filter_type = filter_type;
+
+	list_add(&field->link, &user->fields);

I recommend to use list_add_tail() here so that when accessing the list of field without reverse order. (I found this in [4/13])

kernel/trace/trace_events_user.c line 235

Quoted 7 lines

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

+
+	return 0;
+}
+
+/*
+ * Parses the values of a field within the description
+ * Format: type name [size]

Hmm, don't you accept redundant spaces and tabs?
If this accepts the redundant spaces/tabs, I recommend you to use argv_split() instead of strpbrk() etc. e.g.

	int argc, name_idx = 0, size;
	int ret = -EINVAL;
	char **argv;

	argv = argv_split(GFP_KERNEL, field, &argc);
	if (!argv)
		return -ENOMEM;

	if (!strcmp(argv[pos], "__data_loc") ||
	    !strcmp(argv[pos], "__rel_loc")) {
		if (++pos >= argc)
			goto error;
	}
	if (!strcmp(argv[pos], "unsigned")) {
		if (++pos >= argc)
			goto error;
	} else if (!strcmp(argv[pos], "struct")) {
		is_struct = true;
		if (++pos >= argc)
			goto error;
	}
	if (++pos >= argc)
		goto error;
	name_idx = pos++;
	if (pos < argc) {	// size
		if (!is_struct)
			goto error;
		if (kstrtou32(argv[pos++], 10, &size))
			goto error;
	} else
		size = user_field_size(argv[name_idx - 1]);

	if (pos != argc)
		goto error;
	
	// note that type index is always 0 and size must be converted.
	user_event_add_field(user, argv, name_idx, saved_offset, size,
				    type[0] != 'u', FILTER_OTHER);

	ret = 0;
error:
	argv_free(argv);
	return ret;

(This also requires to simplify user_field_size() and remove FIELD_DEPTH_*) What would you think?

Quoted 33 lines

kernel/trace/trace_events_user.c new file +33 -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:

kernel/trace/trace_events_user.c line 511

Quoted 5 lines

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

+	case TRACE_REG_PERF_CLOSE:
+	case TRACE_REG_PERF_ADD:
+	case TRACE_REG_PERF_DEL:
+		break;
+#endif

At this moment (in this patch), you can just add a default case, or just ignore it, because it does nothing.

Quoted 13 lines

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

+	}
+
+	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;
+}
+

[..]

Quoted 44 lines

kernel/trace/trace_events_user.c new file +44 -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);
+
+	/*
+	 * The refs->events array is protected by RCU, and new items may be
+	 * added. But the user retrieved from indexing into the events array
+	 * shall be immutable while the file is opened.
+	 */
+	if (likely(refs && idx < refs->count))
+		user = refs->events[idx];
+
+	rcu_read_unlock_sched();
+
+	if (unlikely(user == NULL))
+		return -ENOENT;
+
+	tp = &user->tracepoint;
+
+	/*
+	 * It's possible key.enabled disables after this check, however
+	 * we don't mind if a few events are included in this condition.
+	 */
+	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;
+

kernel/trace/trace_events_user.c line 780

Quoted 6 lines

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

+		kdata = kmalloc(i->count, GFP_KERNEL);
+
+		if (unlikely(!kdata))
+			return -ENOMEM;
+
+		datalen = copy_from_iter(kdata, i->count, i);

Don't we need to add this datalen to ret?

Quoted 10 lines

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

+
+		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);

kernel/trace/trace_events_user.c line 800

Quoted 10 lines

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

+			} while ((++probe_func_ptr)->func);
+		}
+
+		rcu_read_unlock_sched();
+
+		kfree(kdata);
+	}
+
+	return ret;
+}

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

Quoted 31 lines · Masami Hiramatsu

Hi Beau,

Thanks for updating the patch! I have some comments below.

On Thu, 9 Dec 2021 14:31:59 -0800
Beau Belgrave <[email protected]> wrote:

[..]

Quoted 7 lines

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

+#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
+
+/*

kernel/trace/trace_events_user.c line 41

Quoted 10 lines

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

+ * 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)
+#define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)

I don't recommend to record the event which size is more than a page size... Maybe 256 entries?
It is also better to limit the total size of the event and the number of fields (arguments).

Quoted 2 lines

Steve, can we write such a big event data on the trace buffer?

This moved to 1024 in part 12 when validation was added.

Quoted 15 lines

[..]

Quoted 13 lines

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

+
+static int user_field_array_size(const char *type)
+{
+	const char *start = strchr(type, '[');
+	char val[8];
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;
+
+	start++;
+
+	while (*start != ']' && size < (sizeof(val) - 1))

Quoted 8 lines

kernel/trace/trace_events_user.c line 144

Quoted 6 lines

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

+		val[size++] = *start++;
+
+	if (*start != ']')
+		return -EINVAL;
+
+	val[size] = 0;

It's '\0', not 0.

Both evaluate to 0, is this a style thing?

For example, argv_split does this same thing ;)

Quoted 5 lines

If I were you, I just use strlcpy(val, start, sizeof(val)), and strchr(val, ']'). Sometimes using standard libc function will be easer to understand what it does. :)

Sure good idea.

[..]

Quoted 14 lines

Quoted 14 lines

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

+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;

Quoted 9 lines

kernel/trace/trace_events_user.c line 228

Quoted 5 lines

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

+	field->size = size;
+	field->is_signed = is_signed;
+	field->filter_type = filter_type;
+
+	list_add(&field->link, &user->fields);

I recommend to use list_add_tail() here so that when accessing the list of field without reverse order. (I found this in [4/13])

If I did that, wouldn't that mean the format file in tracefs now has the arguments printed in reverse order they were added?

Quoted 57 lines

kernel/trace/trace_events_user.c line 235

Quoted 8 lines

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

+
+	return 0;
+}
+
+/*
+ * Parses the values of a field within the description
+ * Format: type name [size]

Hmm, don't you accept redundant spaces and tabs?
If this accepts the redundant spaces/tabs, I recommend you to use argv_split() instead of strpbrk() etc. e.g.

	int argc, name_idx = 0, size;
	int ret = -EINVAL;
	char **argv;

	argv = argv_split(GFP_KERNEL, field, &argc);
	if (!argv)
		return -ENOMEM;

	if (!strcmp(argv[pos], "__data_loc") ||
	    !strcmp(argv[pos], "__rel_loc")) {
		if (++pos >= argc)
			goto error;
	}
	if (!strcmp(argv[pos], "unsigned")) {
		if (++pos >= argc)
			goto error;
	} else if (!strcmp(argv[pos], "struct")) {
		is_struct = true;
		if (++pos >= argc)
			goto error;
	}
	if (++pos >= argc)
		goto error;
	name_idx = pos++;
	if (pos < argc) {	// size
		if (!is_struct)
			goto error;
		if (kstrtou32(argv[pos++], 10, &size))
			goto error;
	} else
		size = user_field_size(argv[name_idx - 1]);

	if (pos != argc)
		goto error;
	
	// note that type index is always 0 and size must be converted.
	user_event_add_field(user, argv, name_idx, saved_offset, size,
				    type[0] != 'u', FILTER_OTHER);

	ret = 0;
error:
	argv_free(argv);
	return ret;

Quoted 3 lines

(This also requires to simplify user_field_size() and remove FIELD_DEPTH_*) What would you think?

The code currently does not support duplicate spaces after the first non-whitespace.

We do copy the string before this, so how this is written would do a double allocation. If the argv_split was moved higher in the callchain then I could move to this.

If you feel strongly about this, I don't have a problem moving to this pattern. Let me know if you feel strongly about it.

Quoted 33 lines

Quoted 33 lines

kernel/trace/trace_events_user.c new file +33 -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:

Quoted 9 lines

kernel/trace/trace_events_user.c line 511

Quoted 5 lines

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

+	case TRACE_REG_PERF_CLOSE:
+	case TRACE_REG_PERF_ADD:
+	case TRACE_REG_PERF_DEL:
+		break;
+#endif

At this moment (in this patch), you can just add a default case, or just ignore it, because it does nothing.

Yeah, I was trying to avoid the warning that resulted if I just ignored them.

Quoted 60 lines

Quoted 14 lines

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

+	}
+
+	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;
+}
+

[..]

Quoted 44 lines

kernel/trace/trace_events_user.c new file +44 -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);
+
+	/*
+	 * The refs->events array is protected by RCU, and new items may be
+	 * added. But the user retrieved from indexing into the events array
+	 * shall be immutable while the file is opened.
+	 */
+	if (likely(refs && idx < refs->count))
+		user = refs->events[idx];
+
+	rcu_read_unlock_sched();
+
+	if (unlikely(user == NULL))
+		return -ENOENT;
+
+	tp = &user->tracepoint;
+
+	/*
+	 * It's possible key.enabled disables after this check, however
+	 * we don't mind if a few events are included in this condition.
+	 */
+	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;
+

Quoted 9 lines

kernel/trace/trace_events_user.c line 780

Quoted 6 lines

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

+		kdata = kmalloc(i->count, GFP_KERNEL);
+
+		if (unlikely(!kdata))
+			return -ENOMEM;
+
+		datalen = copy_from_iter(kdata, i->count, i);

Don't we need to add this datalen to ret?

ret is set to the bytes that were given by the user to avoid multiple writes from occuring for the same data if the data was paged out (or if the event isn't enabled at that time for whatever reason).

Since seek/partial writes are not supported, I don't believe we want to do that.

Quoted 28 lines

Quoted 11 lines

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

+
+		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);

kernel/trace/trace_events_user.c line 800

Quoted 10 lines

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

+			} while ((++probe_func_ptr)->func);
+		}
+
+		rcu_read_unlock_sched();
+
+		kfree(kdata);
+	}
+
+	return ret;
+}

Thank you,

Signature · 1 line

Masami Hiramatsu <[email protected]>

Hi Beau,

Quoted 32 lines · On Fri, 10 Dec 2021 10:03:54 -0800 Beau Belgrave <[email protected]>…

Quoted 31 lines · Masami Hiramatsu

Hi Beau,

Thanks for updating the patch! I have some comments below.

On Thu, 9 Dec 2021 14:31:59 -0800
Beau Belgrave <[email protected]> wrote:

[..]

Quoted 7 lines

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

+#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
+
+/*

kernel/trace/trace_events_user.c line 41

Quoted 10 lines

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

+ * 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)
+#define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)

I don't recommend to record the event which size is more than a page size... Maybe 256 entries?
It is also better to limit the total size of the event and the number of fields (arguments).

Quoted 4 lines

Quoted 2 lines

Steve, can we write such a big event data on the trace buffer?

This moved to 1024 in part 12 when validation was added.

OK, then it should be done in this patch.

BTW, real maximum limitation is defined in the kernel/trace/ring_buffer.c (I'm not sure why this is not defined in the header...)

-----
#define BUF_PAGE_SIZE (PAGE_SIZE - BUF_PAGE_HDR_SIZE)

/* Max payload is BUF_PAGE_SIZE - header (8bytes) */ #define BUF_MAX_DATA_SIZE (BUF_PAGE_SIZE - (sizeof(u32) * 2))
-----

Quoted 24 lines

Quoted 23 lines

[..]

Quoted 9 lines

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

+
+static int user_field_array_size(const char *type)
+{
+	const char *start = strchr(type, '[');
+	char val[8];
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;

kernel/trace/trace_events_user.c line 144

Quoted 10 lines

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

+
+	start++;
+
+	while (*start != ']' && size < (sizeof(val) - 1))
+		val[size++] = *start++;
+
+	if (*start != ']')
+		return -EINVAL;
+
+	val[size] = 0;

It's '\0', not 0.

Quoted 3 lines

Both evaluate to 0, is this a style thing?

For example, argv_split does this same thing ;)

Oops, OK. That is the style thing for clarify what you are doing. (not initializing the element, but terminating the string)

Quoted 34 lines

Quoted 5 lines

If I were you, I just use strlcpy(val, start, sizeof(val)), and strchr(val, ']'). Sometimes using standard libc function will be easer to understand what it does. :)

Sure good idea.

[..]

Quoted 14 lines

Quoted 14 lines

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

+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;

Quoted 9 lines

kernel/trace/trace_events_user.c line 228

Quoted 5 lines

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

+	field->size = size;
+	field->is_signed = is_signed;
+	field->filter_type = filter_type;
+
+	list_add(&field->link, &user->fields);

I recommend to use list_add_tail() here so that when accessing the list of field without reverse order. (I found this in [4/13])

Quoted 2 lines

If I did that, wouldn't that mean the format file in tracefs now has the arguments printed in reverse order they were added?

Ah, sorry. It was my misunderstanding. I found that the trace_event expects the fields are chained in the reverse order. (e.g. trace_event_get_offsets())

BTW, I think the current implementation is confusing. For example, trace_event_get_offsets() needs a redundant explanation;
---

        /*
         * head->next points to the last field with the largest offset,
         * since it was added last by trace_define_field()
         */
        tail = list_first_entry(head, struct ftrace_event_field, link);

---
If the list is sorted in normal order, it doesn't need such explanation, just do "tail = list_last_entry(...)"

Quoted 64 lines

Quoted 57 lines

kernel/trace/trace_events_user.c line 235

Quoted 8 lines

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

+
+	return 0;
+}
+
+/*
+ * Parses the values of a field within the description
+ * Format: type name [size]

Hmm, don't you accept redundant spaces and tabs?
If this accepts the redundant spaces/tabs, I recommend you to use argv_split() instead of strpbrk() etc. e.g.

	int argc, name_idx = 0, size;
	int ret = -EINVAL;
	char **argv;

	argv = argv_split(GFP_KERNEL, field, &argc);
	if (!argv)
		return -ENOMEM;

	if (!strcmp(argv[pos], "__data_loc") ||
	    !strcmp(argv[pos], "__rel_loc")) {
		if (++pos >= argc)
			goto error;
	}
	if (!strcmp(argv[pos], "unsigned")) {
		if (++pos >= argc)
			goto error;
	} else if (!strcmp(argv[pos], "struct")) {
		is_struct = true;
		if (++pos >= argc)
			goto error;
	}
	if (++pos >= argc)
		goto error;
	name_idx = pos++;
	if (pos < argc) {	// size
		if (!is_struct)
			goto error;
		if (kstrtou32(argv[pos++], 10, &size))
			goto error;
	} else
		size = user_field_size(argv[name_idx - 1]);

	if (pos != argc)
		goto error;
	
	// note that type index is always 0 and size must be converted.
	user_event_add_field(user, argv, name_idx, saved_offset, size,
				    type[0] != 'u', FILTER_OTHER);

	ret = 0;
error:
	argv_free(argv);
	return ret;

Quoted 3 lines

(This also requires to simplify user_field_size() and remove FIELD_DEPTH_*) What would you think?

The code currently does not support duplicate spaces after the first non-whitespace.

Quoted 3 lines

We do copy the string before this, so how this is written would do a double allocation. If the argv_split was moved higher in the callchain then I could move to this.

If it works and simplifies, I'm OK. But I thought the syntax required to split a user string by ';' at first, and split each field by spaces. So I put the argv_split() here. And anyway, this is not a hot path. I think avoiding allocation is not such a big matter.

Quoted 3 lines

If you feel strongly about this, I don't have a problem moving to this pattern. Let me know if you feel strongly about it.

I just hope to support duplicate spaces/tabs, since I guess that users may want to write the field definition with indentation.

(Recently I hit a similar issue on another software. No one duplicates visible separators, but spaces/tabs. :( )

Quoted 43 lines

Quoted 33 lines

Quoted 33 lines

kernel/trace/trace_events_user.c new file +33 -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:

Quoted 9 lines

kernel/trace/trace_events_user.c line 511

Quoted 5 lines

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

+	case TRACE_REG_PERF_CLOSE:
+	case TRACE_REG_PERF_ADD:
+	case TRACE_REG_PERF_DEL:
+		break;
+#endif

At this moment (in this patch), you can just add a default case, or just ignore it, because it does nothing.

Quoted 2 lines

Yeah, I was trying to avoid the warning that resulted if I just ignored them.

Ah, then that's OK.

Quoted 70 lines

Quoted 60 lines

Quoted 14 lines

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

+	}
+
+	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;
+}
+

[..]

Quoted 44 lines

kernel/trace/trace_events_user.c new file +44 -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);
+
+	/*
+	 * The refs->events array is protected by RCU, and new items may be
+	 * added. But the user retrieved from indexing into the events array
+	 * shall be immutable while the file is opened.
+	 */
+	if (likely(refs && idx < refs->count))
+		user = refs->events[idx];
+
+	rcu_read_unlock_sched();
+
+	if (unlikely(user == NULL))
+		return -ENOENT;
+
+	tp = &user->tracepoint;
+
+	/*
+	 * It's possible key.enabled disables after this check, however
+	 * we don't mind if a few events are included in this condition.
+	 */
+	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;
+

Quoted 9 lines

kernel/trace/trace_events_user.c line 780

Quoted 6 lines

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

+		kdata = kmalloc(i->count, GFP_KERNEL);
+
+		if (unlikely(!kdata))
+			return -ENOMEM;
+
+		datalen = copy_from_iter(kdata, i->count, i);

Don't we need to add this datalen to ret?

Quoted 6 lines

ret is set to the bytes that were given by the user to avoid multiple writes from occuring for the same data if the data was paged out (or if the event isn't enabled at that time for whatever reason).

Since seek/partial writes are not supported, I don't believe we want to do that.

OK, got it.

Thank you,

Quoted 26 lines

Quoted 26 lines

Quoted 11 lines

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

+
+		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);

kernel/trace/trace_events_user.c line 800

Quoted 10 lines

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

+			} while ((++probe_func_ptr)->func);
+		}
+
+		rcu_read_unlock_sched();
+
+		kfree(kdata);
+	}
+
+	return ret;
+}

Thank you,

Quoted 2 lines

Quoted 2 lines

Signature · 1 line

Masami Hiramatsu <[email protected]>

Signature · 1 line

Masami Hiramatsu <[email protected]>

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 | 107 ++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 2 deletions(-)

kernel/trace/trace_events_user.c +105 -2

index 1d96d1c85147..bd8ac46fddb1 100644
@@ -357,6 +357,106 @@ static int user_event_parse_fields(struct user_event *user, char *args)
 
 static struct trace_event_fields user_event_fields_array[1];
 
+static const char *user_field_format(const char *type)
+{
+	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)
+		return "%s";
+
+	/* Unknown, likely struct, allowed treat as 64-bit */
+	return "%llu";
+}
+
+static bool user_field_is_dyn_string(const char *type)
+{
+	if (str_has_prefix(type, "__data_loc ") ||
+	    str_has_prefix(type, "__rel_loc "))
+		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 enum print_line_t user_event_print_trace(struct trace_iterator *iter,
 						int flags,
 						struct trace_event *event)

@@ -390,6 +490,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);
 

@@ -669,8 +770,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;

Signature · 1 line

2.17.1

Pass iterator through to probes to allow copying data directly to the probe buffers instead of taking multiple copies. Enables eBPF user and raw iterator types out to programs for no-copy scenarios.

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

---
 kernel/trace/trace_events_user.c | 102 ++++++++++++++++++++++---------
 1 file changed, 74 insertions(+), 28 deletions(-)

kernel/trace/trace_events_user.c +68 -20

index 807db0af74fb..1d29f6ec907d 100644
@@ -41,6 +41,10 @@
 #define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)
 #define MAX_FIELD_ARG_NAME 256
 
+#define MAX_BPF_COPY_SIZE PAGE_SIZE
+#define MAX_STACK_BPF_DATA 512
+#define copy_nofault copy_from_iter_nocache
+
 static char *register_page_data;
 
 static DEFINE_MUTEX(reg_mutex);

@@ -78,8 +82,7 @@ struct user_event_refs {
 	struct user_event *events[];
 };
 
-typedef void (*user_event_func_t) (struct user_event *user,
-				   void *data, u32 datalen,
+typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i,
 				   void *tpdata);
 
 static int user_event_parse(char *name, char *args, char *flags,

@@ -515,7 +518,7 @@ static struct user_event *find_user_event(char *name, u32 *outkey)
 /*
  * Writes the user supplied payload out to a trace file.
  */
-static void user_event_ftrace(struct user_event *user, void *data, u32 datalen,
+static void user_event_ftrace(struct user_event *user, struct iov_iter *i,
 			      void *tpdata)
 {
 	struct trace_event_file *file;

@@ -531,41 +534,85 @@ static void user_event_ftrace(struct user_event *user, void *data, u32 datalen,
 
 	/* Allocates and fills trace_entry, + 1 of this is data payload */
 	entry = trace_event_buffer_reserve(&event_buffer, file,
-					   sizeof(*entry) + datalen);
+					   sizeof(*entry) + i->count);
 
 	if (unlikely(!entry))
 		return;
 
-	memcpy(entry + 1, data, datalen);
+	if (unlikely(!copy_nofault(entry + 1, i->count, i))) {
+		__trace_event_discard_commit(event_buffer.buffer,
+					     event_buffer.event);
+		return;
+	}
 
 	trace_event_buffer_commit(&event_buffer);
 }
 
 #ifdef CONFIG_PERF_EVENTS
+static void user_event_bpf(struct user_event *user, struct iov_iter *i)
+{
+	struct user_bpf_context context;
+	struct user_bpf_iter bpf_i;
+	char fast_data[MAX_STACK_BPF_DATA];
+	void *temp = NULL;
+
+	if ((user->flags & FLAG_BPF_ITER) && iter_is_iovec(i)) {
+		/* Raw iterator */
+		context.data_type = USER_BPF_DATA_ITER;
+		context.data_len = i->count;
+		context.iter = &bpf_i;
+
+		bpf_i.iov_offset = i->iov_offset;
+		bpf_i.iov = i->iov;
+		bpf_i.nr_segs = i->nr_segs;
+	} else if (i->nr_segs == 1 && iter_is_iovec(i)) {
+		/* Single buffer from user */
+		context.data_type = USER_BPF_DATA_USER;
+		context.data_len = i->count;
+		context.udata = i->iov->iov_base + i->iov_offset;
+	} else {
+		/* Multi buffer from user */
+		struct iov_iter copy = *i;
+		size_t copy_size = min(i->count, (size_t)MAX_BPF_COPY_SIZE);
+
+		context.data_type = USER_BPF_DATA_KERNEL;
+		context.kdata = fast_data;
+
+		if (unlikely(copy_size > sizeof(fast_data))) {
+			temp = kmalloc(copy_size, GFP_NOWAIT);
+
+			if (temp)
+				context.kdata = temp;
+			else
+				copy_size = sizeof(fast_data);
+		}
+
+		context.data_len = copy_nofault(context.kdata,
+						copy_size, &copy);
+	}
+
+	trace_call_bpf(&user->call, &context);
+
+	kfree(temp);
+}
+
 /*
  * Writes the user supplied payload out to perf ring buffer or eBPF program.
  */
-static void user_event_perf(struct user_event *user, void *data, u32 datalen,
+static void user_event_perf(struct user_event *user, struct iov_iter *i,
 			    void *tpdata)
 {
 	struct hlist_head *perf_head;
 
-	if (bpf_prog_array_valid(&user->call)) {
-		struct user_bpf_context context = {0};
-
-		context.data_len = datalen;
-		context.data_type = USER_BPF_DATA_KERNEL;
-		context.kdata = data;
-
-		trace_call_bpf(&user->call, &context);
-	}
+	if (bpf_prog_array_valid(&user->call))
+		user_event_bpf(user, i);
 
 	perf_head = this_cpu_ptr(user->call.perf_events);
 
 	if (perf_head && !hlist_empty(perf_head)) {
 		struct trace_entry *perf_entry;
 		struct pt_regs *regs;
-		size_t size = sizeof(*perf_entry) + datalen;
+		size_t size = sizeof(*perf_entry) + i->count;
 		int context;
 
 		perf_entry = perf_trace_buf_alloc(ALIGN(size, 8),

@@ -576,7 +623,10 @@ static void user_event_perf(struct user_event *user, void *data, u32 datalen,
 
 		perf_fetch_caller_regs(regs);
 
-		memcpy(perf_entry + 1, data, datalen);
+		if (unlikely(!copy_nofault(perf_entry + 1, i->count, i))) {
+			perf_swevent_put_recursion_context(context);
+			return;
+		}
 
 		perf_trace_buf_submit(perf_entry, size, context,
 				      user->call.event.type, 1, regs,

@@ -1009,32 +1059,28 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
 	if (likely(atomic_read(&tp->key.enabled) > 0)) {
 		struct tracepoint_func *probe_func_ptr;
 		user_event_func_t probe_func;
+		struct iov_iter copy;
 		void *tpdata;
-		void *kdata;
-		u32 datalen;
 
-		kdata = kmalloc(i->count, GFP_KERNEL);

kernel/trace/trace_events_user.c +3 -3

 		probe_func_ptr = rcu_dereference_sched(tp->funcs);
 
 		if (probe_func_ptr) {
 			do {
+				copy = *i;
 				probe_func = probe_func_ptr->func;
 				tpdata = probe_func_ptr->data;
-				probe_func(user, kdata, datalen, tpdata);
+				probe_func(user, &copy, tpdata);
 			} while ((++probe_func_ptr)->func);
 		}
 
+		pagefault_enable();
 		rcu_read_unlock_sched();
-
-		kfree(kdata);
 	}
 
 	return ret;

Signature · 1 line

2.17.1

Hi Beau,

Quoted 6 lines · On Thu, 9 Dec 2021 14:32:06 -0800 Beau Belgrave <[email protected]>…

Pass iterator through to probes to allow copying data directly to the probe buffers instead of taking multiple copies. Enables eBPF user and raw iterator types out to programs for no-copy scenarios.

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

---

Quoted 18 lines

kernel/trace/trace_events_user.c +3 -8

@@ -1009,32 +1059,28 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
 	if (likely(atomic_read(&tp->key.enabled) > 0)) {
 		struct tracepoint_func *probe_func_ptr;
 		user_event_func_t probe_func;
+		struct iov_iter copy;
 		void *tpdata;
-		void *kdata;
-		u32 datalen;
 
-		kdata = kmalloc(i->count, GFP_KERNEL);
-
-		if (unlikely(!kdata))
-			return -ENOMEM;
-
-		datalen = copy_from_iter(kdata, i->count, i);
+		if (unlikely(iov_iter_fault_in_readable(i, i->count)))
+			return -EFAULT;
 

kernel/trace/trace_events_user.c static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) · line 1069

Quoted 2 lines

kernel/trace/trace_events_user.c +1 -0

 		rcu_read_lock_sched();
+		pagefault_disable();

Since the pagefault_disable() may have unexpected side effect, I think it should be used really limited area, e.g. around actual memory access function.
Can we move this around the copy_nofault()?

Thank you,

Quoted 19 lines

kernel/trace/trace_events_user.c +3 -3

 		probe_func_ptr = rcu_dereference_sched(tp->funcs);
 
 		if (probe_func_ptr) {
 			do {
+				copy = *i;
 				probe_func = probe_func_ptr->func;
 				tpdata = probe_func_ptr->data;
-				probe_func(user, kdata, datalen, tpdata);
+				probe_func(user, &copy, tpdata);
 			} while ((++probe_func_ptr)->func);
 		}
 
+		pagefault_enable();
 		rcu_read_unlock_sched();
-
-		kfree(kdata);
 	}
 

Quoted 4 lines

kernel/trace/trace_events_user.c +0 -0

 	return ret;

Signature · 1 line

2.17.1

Signature · 1 line

Masami Hiramatsu <[email protected]>

Add validation to ensure data is at or greater than the min size for the fields of the event. If a dynamic array is used and is a type of char, ensure null termination of the array exists. Add unit test cases for the above scenarios.

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

---
 include/uapi/linux/user_events.h              |   3 +
 kernel/trace/trace_events_user.c              | 151 ++++++++++++++++--
 .../selftests/user_events/ftrace_test.c       |  83 ++++++++++
 3 files changed, 220 insertions(+), 17 deletions(-)

include/uapi/linux/user_events.h +3 -0

index 5bff99418deb..f97db05e00c9 100644
@@ -29,6 +29,9 @@
 #define EVENT_STATUS_PERF (1 << EVENT_BIT_PERF)
 #define EVENT_STATUS_OTHER (1 << EVENT_BIT_OTHER)
 
+/* Create dynamic location entry within a 32-bit value */
+#define DYN_LOC(offset, size) ((size) << 16 | (offset))
+
 /* Use raw iterator for attached BPF program(s), no affect on ftrace/perf */
 #define FLAG_BPF_ITER (1 << 0)
 

kernel/trace/trace_events_user.c +134 -17

index 1d29f6ec907d..56eb58ddb4cf 100644
@@ -38,7 +38,7 @@
 /* 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)
-#define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)
+#define MAX_FIELD_ARRAY_SIZE 1024
 #define MAX_FIELD_ARG_NAME 256
 
 #define MAX_BPF_COPY_SIZE PAGE_SIZE

@@ -65,9 +65,11 @@ struct user_event {
 	struct dyn_event devent;
 	struct hlist_node node;
 	struct list_head fields;
+	struct list_head validators;
 	atomic_t refcnt;
 	int index;
 	int flags;
+	int min_size;
 };
 
 /*

@@ -82,8 +84,17 @@ struct user_event_refs {
 	struct user_event *events[];
 };
 
+#define VALIDATOR_ENSURE_NULL (1 << 0)
+#define VALIDATOR_REL (1 << 1)
+
+struct user_event_validator {
+	struct list_head link;
+	int offset;
+	int flags;
+};
+
 typedef void (*user_event_func_t) (struct user_event *user, struct iov_iter *i,
-				   void *tpdata);
+				   void *tpdata, bool *faulted);
 
 static int user_event_parse(char *name, char *args, char *flags,
 			    struct user_event **newuser);

@@ -200,6 +211,17 @@ static int user_field_size(const char *type)
 	return -EINVAL;
 }
 
+static void user_event_destroy_validators(struct user_event *user)
+{
+	struct user_event_validator *validator, *next;
+	struct list_head *head = &user->validators;
+
+	list_for_each_entry_safe(validator, next, head, link) {
+		list_del(&validator->link);
+		kfree(validator);
+	}
+}
+
 static void user_event_destroy_fields(struct user_event *user)
 {
 	struct ftrace_event_field *field, *next;

@@ -215,13 +237,43 @@ 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 user_event_validator *validator;
 	struct ftrace_event_field *field;
+	int validator_flags = 0;
 
 	field = kmalloc(sizeof(*field), GFP_KERNEL);
 
 	if (!field)
 		return -ENOMEM;
 
+	if (str_has_prefix(type, "__data_loc "))
+		goto add_validator;
+
+	if (str_has_prefix(type, "__rel_loc ")) {
+		validator_flags |= VALIDATOR_REL;
+		goto add_validator;
+	}
+
+	goto add_field;
+
+add_validator:
+	if (strstr(type, "char[") != 0)
+		validator_flags |= VALIDATOR_ENSURE_NULL;
+
+	validator = kmalloc(sizeof(*validator), GFP_KERNEL);
+
+	if (!validator) {
+		kfree(field);
+		return -ENOMEM;
+	}
+
+	validator->flags = validator_flags;
+	validator->offset = offset;
+
+	/* Want sequential access when validating */
+	list_add_tail(&validator->link, &user->validators);
+
+add_field:
 	field->type = type;
 	field->name = name;
 	field->offset = offset;

@@ -231,6 +283,12 @@ static int user_event_add_field(struct user_event *user, const char *type,
 
 	list_add(&field->link, &user->fields);
 
+	/*
+	 * Min size from user writes that are required, this does not include
+	 * the size of trace_entry (common fields).
+	 */
+	user->min_size = (offset + size) - sizeof(struct trace_entry);
+
 	return 0;
 }
 

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

@@ -515,15 +574,49 @@ static struct user_event *find_user_event(char *name, u32 *outkey)
 	return NULL;
 }
 
+static int user_event_validate(struct user_event *user, void *data, int len)
+{
+	struct list_head *head = &user->validators;
+	struct user_event_validator *validator;
+	void *pos, *end = data + len;
+	u32 loc, offset, size;
+
+	list_for_each_entry(validator, head, link) {
+		pos = data + validator->offset;
+
+		/* Already done min_size check, no bounds check here */
+		loc = *(u32 *)pos;
+		offset = loc & 0xffff;
+		size = loc >> 16;
+
+		if (likely(validator->flags & VALIDATOR_REL))
+			pos += offset + sizeof(loc);
+		else
+			pos = data + offset;
+
+		pos += size;
+
+		if (unlikely(pos > end))
+			return -EFAULT;
+
+		if (likely(validator->flags & VALIDATOR_ENSURE_NULL))
+			if (unlikely(*(char *)(pos - 1) != 0))
+				return -EFAULT;
+	}
+
+	return 0;
+}
+
 /*
  * Writes the user supplied payload out to a trace file.
  */
 static void user_event_ftrace(struct user_event *user, struct iov_iter *i,
-			      void *tpdata)
+			      void *tpdata, bool *faulted)
 {
 	struct trace_event_file *file;
 	struct trace_entry *entry;
 	struct trace_event_buffer event_buffer;
+	size_t size = sizeof(*entry) + i->count;
 
 	file = (struct trace_event_file *)tpdata;
 

@@ -533,19 +626,25 @@ static void user_event_ftrace(struct user_event *user, struct iov_iter *i,
 		return;
 
 	/* Allocates and fills trace_entry, + 1 of this is data payload */
-	entry = trace_event_buffer_reserve(&event_buffer, file,
-					   sizeof(*entry) + i->count);
+	entry = trace_event_buffer_reserve(&event_buffer, file, size);
 
 	if (unlikely(!entry))
 		return;
 
-	if (unlikely(!copy_nofault(entry + 1, i->count, i))) {
-		__trace_event_discard_commit(event_buffer.buffer,
-					     event_buffer.event);
-		return;
-	}
+	if (unlikely(!copy_nofault(entry + 1, i->count, i)))
+		goto discard;
+
+	if (!list_empty(&user->validators) &&
+	    unlikely(user_event_validate(user, entry, size)))
+		goto discard;
 
 	trace_event_buffer_commit(&event_buffer);
+
+	return;
+discard:
+	*faulted = true;
+	__trace_event_discard_commit(event_buffer.buffer,
+				     event_buffer.event);
 }
 
 #ifdef CONFIG_PERF_EVENTS

@@ -573,7 +672,7 @@ static void user_event_bpf(struct user_event *user, struct iov_iter *i)
 	} else {
 		/* Multi buffer from user */
 		struct iov_iter copy = *i;
-		size_t copy_size = min(i->count, (size_t)MAX_BPF_COPY_SIZE);
+		size_t copy_size = min_t(size_t, i->count, MAX_BPF_COPY_SIZE);
 
 		context.data_type = USER_BPF_DATA_KERNEL;
 		context.kdata = fast_data;

@@ -600,7 +699,7 @@ static void user_event_bpf(struct user_event *user, struct iov_iter *i)
  * Writes the user supplied payload out to perf ring buffer or eBPF program.
  */
 static void user_event_perf(struct user_event *user, struct iov_iter *i,
-			    void *tpdata)
+			    void *tpdata, bool *faulted)
 {
 	struct hlist_head *perf_head;
 

@@ -623,14 +722,21 @@ static void user_event_perf(struct user_event *user, struct iov_iter *i,
 
 		perf_fetch_caller_regs(regs);
 
-		if (unlikely(!copy_nofault(perf_entry + 1, i->count, i))) {
-			perf_swevent_put_recursion_context(context);
-			return;
-		}
+		if (unlikely(!copy_nofault(perf_entry + 1, i->count, i)))
+			goto discard;
+
+		if (!list_empty(&user->validators) &&
+		    unlikely(user_event_validate(user, perf_entry, size)))
+			goto discard;
 
 		perf_trace_buf_submit(perf_entry, size, context,
 				      user->call.event.type, 1, regs,
 				      perf_head, NULL);
+
+		return;
+discard:
+		*faulted = true;
+		perf_swevent_put_recursion_context(context);
 	}
 }
 #endif

@@ -945,6 +1051,7 @@ static int user_event_parse(char *name, char *args, char *flags,
 
 	INIT_LIST_HEAD(&user->class.fields);
 	INIT_LIST_HEAD(&user->fields);
+	INIT_LIST_HEAD(&user->validators);
 
 	user->tracepoint.name = name;
 

@@ -993,6 +1100,7 @@ static int user_event_parse(char *name, char *args, char *flags,
 	return 0;
 put_user:
 	user_event_destroy_fields(user);
+	user_event_destroy_validators(user);
 	kfree(user);
 	return ret;
 }

@@ -1050,6 +1158,9 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
 	if (unlikely(user == NULL))
 		return -ENOENT;
 
+	if (unlikely(i->count < user->min_size))
+		return -EINVAL;
+
 	tp = &user->tracepoint;
 
 	/*

@@ -1061,10 +1172,13 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
 		user_event_func_t probe_func;
 		struct iov_iter copy;
 		void *tpdata;
+		bool faulted;
 
 		if (unlikely(iov_iter_fault_in_readable(i, i->count)))
 			return -EFAULT;
 
+		faulted = false;
+
 		rcu_read_lock_sched();
 		pagefault_disable();
 

@@ -1075,12 +1189,15 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
 				copy = *i;
 				probe_func = probe_func_ptr->func;
 				tpdata = probe_func_ptr->data;
-				probe_func(user, &copy, tpdata);
+				probe_func(user, &copy, tpdata, &faulted);
 			} while ((++probe_func_ptr)->func);
 		}
 
 		pagefault_enable();
 		rcu_read_unlock_sched();
+
+		if (unlikely(faulted))
+			return -EFAULT;
 	}
 
 	return ret;

tools/testing/selftests/user_events/ftrace_test.c +83 -0

index 9d53717139e6..16aff1fb295a 100644
@@ -47,6 +47,22 @@ static int trace_bytes(void)
 	return bytes;
 }
 
+static int clear(void)
+{
+	int fd = open(data_file, O_RDWR);
+
+	if (fd == -1)
+		return -1;
+
+	if (ioctl(fd, DIAG_IOCSDEL, "__test_event") == -1)
+		if (errno != ENOENT)
+			return -1;
+
+	close(fd);
+
+	return 0;
+}
+
 FIXTURE(user) {
 	int status_fd;
 	int data_fd;

@@ -71,6 +87,8 @@ FIXTURE_TEARDOWN(user) {
 		write(self->enable_fd, "0", sizeof("0"));
 		close(self->enable_fd);
 	}
+
+	ASSERT_EQ(0, clear());
 }
 
 TEST_F(user, register_events) {

@@ -199,6 +217,71 @@ TEST_F(user, write_fault) {
 	ASSERT_EQ(0, munmap(anon, l));
 }
 
+TEST_F(user, write_validator) {
+	struct user_reg reg = {0};
+	struct iovec io[3];
+	int loc, bytes;
+	char data[8];
+	int before = 0, after = 0;
+
+	reg.size = sizeof(reg);
+	reg.name_args = (__u64)"__test_event __rel_loc char[] data";
+
+	/* Register should work */
+	ASSERT_EQ(0, ioctl(self->data_fd, DIAG_IOCSREG, &reg));
+	ASSERT_EQ(0, reg.write_index);
+	ASSERT_NE(0, reg.status_index);
+
+	io[0].iov_base = &reg.write_index;
+	io[0].iov_len = sizeof(reg.write_index);
+	io[1].iov_base = &loc;
+	io[1].iov_len = sizeof(loc);
+	io[2].iov_base = data;
+	bytes = snprintf(data, sizeof(data), "Test") + 1;
+	io[2].iov_len = bytes;
+
+	/* Undersized write should fail */
+	ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 1));
+	ASSERT_EQ(EINVAL, errno);
+
+	/* Enable event */
+	self->enable_fd = open(enable_file, O_RDWR);
+	ASSERT_NE(-1, write(self->enable_fd, "1", sizeof("1")))
+
+	/* Full in-bounds write should work */
+	before = trace_bytes();
+	loc = DYN_LOC(0, bytes);
+	ASSERT_NE(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	after = trace_bytes();
+	ASSERT_GT(after, before);
+
+	/* Out of bounds write should fault (offset way out) */
+	loc = DYN_LOC(1024, bytes);
+	ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	ASSERT_EQ(EFAULT, errno);
+
+	/* Out of bounds write should fault (offset 1 byte out) */
+	loc = DYN_LOC(1, bytes);
+	ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	ASSERT_EQ(EFAULT, errno);
+
+	/* Out of bounds write should fault (size way out) */
+	loc = DYN_LOC(0, bytes + 1024);
+	ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	ASSERT_EQ(EFAULT, errno);
+
+	/* Out of bounds write should fault (size 1 byte out) */
+	loc = DYN_LOC(0, bytes + 1);
+	ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	ASSERT_EQ(EFAULT, errno);
+
+	/* Non-Null should fault */
+	memset(data, 'A', sizeof(data));
+	loc = DYN_LOC(0, bytes);
+	ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	ASSERT_EQ(EFAULT, errno);
+}
+
 int main(int argc, char **argv)
 {
 	return test_harness_run(argc, argv);

Signature · 1 line

2.17.1