[PATCH v7 00/13] user_events: Enable user processes to create and write to trace events

[PATCH v7 00/13] user_events: Enable user processes to create and write to trace events

[Beau Belgrave]

User mode processes that wish to use trace events to get data into
ftrace, perf, eBPF, etc are limited to uprobes today. The user events
features enables an ABI for user mode processes to create and write to
trace events that are isolated from kernel level trace events. This
enables a faster path for tracing from user mode data as well as opens
managed code to participate in trace events, where stub locations are
dynamic.

User processes often want to trace only when it's useful. To enable this
a set of pages are mapped into the user process space that indicate the
current state of the user events that have been registered. User
processes can check if their event is hooked to a trace/probe, and if it
is, emit the event data out via the write() syscall.

Two new files are introduced into tracefs to accomplish this:
user_events_status - This file is mmap'd into participating user mode
processes to indicate event status.

user_events_data - This file is opened and register/delete ioctl's are
issued to create/open/delete trace events that can be used for tracing.

The typical scenario is on process start to mmap user_events_status. Processes
then register the events they plan to use via the REG ioctl. The ioctl reads
and updates the passed in user_reg struct. The status_index of the struct is
used to know the byte in the status page to check for that event. The
write_index of the struct is used to describe that event when writing out to
the fd that was used for the ioctl call. The data must always include this
index first when writing out data for an event. Data can be written either by
write() or by writev().

For example, in memory:
int index;
char data[];

Psuedo code example of typical usage:
struct user_reg reg;

int page_fd = open("user_events_status", O_RDWR);
char *page_data = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, page_fd, 0);
close(page_fd);

int data_fd = open("user_events_data", O_RDWR);

reg.size = sizeof(reg);
reg.name_args = (__u64)"test";

ioctl(data_fd, DIAG_IOCSREG, &reg);
int status_id = reg.status_index;
int write_id = reg.write_index;

struct iovec io[2];
io[0].iov_base = &write_id;
io[0].iov_len = sizeof(write_id);
io[1].iov_base = payload;
io[1].iov_len = sizeof(payload);

if (page_data[status_id])
	writev(data_fd, io, 2);

User events are also exposed via the dynamic_events tracefs file for
both create and delete. Current status is exposed via the user_events_status
tracefs file.

Simple example to register a user event via dynamic_events:
	echo u:test >> dynamic_events
	cat dynamic_events
	u:test

If an event is hooked to a probe, the probe hooked shows up:
	echo 1 > events/user_events/test/enable
	cat user_events_status
	1:test # Used by ftrace

	Active: 1
	Busy: 1
	Max: 4096

If an event is not hooked to a probe, no probe status shows up:
	echo 0 > events/user_events/test/enable
	cat user_events_status
	1:test

	Active: 1
	Busy: 0
	Max: 4096

Users can describe the trace event format via the following format:
	name[:FLAG1[,FLAG2...] [field1[;field2...]]

Each field has the following format:
	type name

Example for char array with a size of 20 named msg:
	echo 'u:detailed char[20] msg' >> dynamic_events
	cat dynamic_events
	u:detailed char[20] msg

Data offsets are based on the data written out via write() and will be
updated to reflect the correct offset in the trace_event fields. For dynamic
data it is recommended to use the new __rel_loc data type. This type will be
the same as __data_loc, but the offset is relative to this entry. This allows
user_events to not worry about what common fields are being inserted before
the data.

The above format is valid for both the ioctl and the dynamic_events file.

V2:
Fixed kmalloc vs kzalloc for register_page.
Renamed user_event_mmap to user_event_status.
Renamed user_event prefix from ue to u.
Added seq_* operations to user_event_status to enable cat output.
Aligned field parsing to synth_events format (+ size specifier for
custom/user types).
Added uapi header user_events.h to align kernel and user ABI definitions.

V3:
Updated ABI to handle single FD into many events via an int header.
Added iovec/writev support to enable int header without payload changes.
Updated bpf context to describe if data is coming from user, kernel or
raw iovec.
Added flag support for registering event, allows forcing BPF to always
recieve the direct iovecs for sensitive code paths that do not want
copies.

V4:
Moved to struct user_reg for registering events via ioctl.
Added unit tests for ftrace, dyn_events and perf integration.
Added print_fmt generation and proper dyn_events matching statements.
Reduced time in preemption disabled paths.
Added documentation file.
Pre-fault in data when preemption is enabled and use no-fault copy in probes.
Fixed MIPs missing PAGE_READONLY define.

V5:
Rebase to linux-trace for-next branch.
Added sample code into samples/user_events.
Switched to str_has_prefix in various locations.
Allow hex in array sizes and ensure reasonable sizes are used.
Moved lifetime of name buffer when parsing to the caller for failure paths.
Fixed documentation nits and index.
Ensure event isn't busy before freeing through dyn_events.
Properly handle failure case for ftrace and perf in fault cases for buffers.
Ensure write data is over min size and null terminated for dynamic arrays.

V6:
Fixed endian issue with dyn loc decoding (use u32).
Fixed size_t conversion warning on hexagon arch (min vs min_t).
Handle cases for __get_str vs __get_rel_str in print_fmt generation.
Add additional comments around various event member lifetimes.
Reduced max field array size to 1K.

V7:
Acquire reg_mutex during release, ensure refs cannot change under any situation.
Remove default n from Kconfig.
Move from static 0644 mode to TRACE_MODE_WRITE.

Beau Belgrave (13):
  user_events: Add UABI header for user access to user_events
  user_events: Add minimal support for trace_event into ftrace
  user_events: Add print_fmt generation support for basic types
  user_events: Handle matching arguments from dyn_events
  user_events: Add basic perf and eBPF support
  user_events: Add self-test for ftrace integration
  user_events: Add self-test for dynamic_events integration
  user_events: Add self-test for perf_event integration
  user_events: Optimize writing events by only copying data once
  user_events: Add documentation file
  user_events: Add sample code for typical usage
  user_events: Validate user payloads for size and null termination
  user_events: Use __get_rel_str for relative string fields

 Documentation/trace/index.rst                 |    1 +
 Documentation/trace/user_events.rst           |  195 ++
 include/uapi/linux/user_events.h              |   71 +
 kernel/trace/Kconfig                          |   14 +
 kernel/trace/Makefile                         |    1 +
 kernel/trace/trace_events_user.c              | 1606 +++++++++++++++++
 samples/user_events/Makefile                  |    5 +
 samples/user_events/example.c                 |   91 +
 tools/testing/selftests/user_events/Makefile  |    9 +
 .../testing/selftests/user_events/dyn_test.c  |  130 ++
 .../selftests/user_events/ftrace_test.c       |  454 +++++
 .../testing/selftests/user_events/perf_test.c |  168 ++
 tools/testing/selftests/user_events/settings  |    1 +
 13 files changed, 2746 insertions(+)
 create mode 100644 Documentation/trace/user_events.rst
 create mode 100644 include/uapi/linux/user_events.h
 create mode 100644 kernel/trace/trace_events_user.c
 create mode 100644 samples/user_events/Makefile
 create mode 100644 samples/user_events/example.c
 create mode 100644 tools/testing/selftests/user_events/Makefile
 create mode 100644 tools/testing/selftests/user_events/dyn_test.c
 create mode 100644 tools/testing/selftests/user_events/ftrace_test.c
 create mode 100644 tools/testing/selftests/user_events/perf_test.c
 create mode 100644 tools/testing/selftests/user_events/settings


base-commit: 67d4f6e3bf5dddced226fbf19704cdbbb0c98847
-- 
2.17.1

[PATCH v7 01/13] user_events: Add UABI header for user access to user_events

[Beau Belgrave]

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

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 include/uapi/linux/user_events.h | 68 ++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 include/uapi/linux/user_events.h

diff --git a/include/uapi/linux/user_events.h b/include/uapi/linux/user_events.h
new file mode 100644
index 000000000000..5bff99418deb
--- /dev/null
+++ b/include/uapi/linux/user_events.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2021, Microsoft Corporation.
+ *
+ * Authors:
+ *   Beau Belgrave <beaub@linux.microsoft.com>
+ */
+#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 */
-- 
2.17.1

[PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[Beau Belgrave]

Minimal support for interacting with dynamic events, trace_event and
ftrace. Core outline of flow between user process, ioctl and trace_event
APIs.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 kernel/trace/Kconfig             |   14 +
 kernel/trace/Makefile            |    1 +
 kernel/trace/trace_events_user.c | 1194 ++++++++++++++++++++++++++++++
 3 files changed, 1209 insertions(+)
 create mode 100644 kernel/trace/trace_events_user.c

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 420ff4bc67fd..a0ae2640f391 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -724,6 +724,20 @@ config SYNTH_EVENTS
 
 	  If in doubt, say N.
 
+config USER_EVENTS
+	bool "User trace events"
+	select TRACING
+	select DYNAMIC_EVENTS
+	help
+	  User trace events are user-defined trace events that
+	  can be used like an existing kernel trace event.  User trace
+	  events are generated by writing to a tracefs file.  User
+	  processes can determine if their tracing events should be
+	  generated by memory mapping a tracefs file and checking for
+	  an associated byte being non-zero.
+
+	  If in doubt, say N.
+
 config HIST_TRIGGERS
 	bool "Histogram triggers"
 	depends on ARCH_HAVE_NMI_SAFE_CMPXCHG
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index bedc5caceec7..19ef3758da95 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_PROBE_EVENTS) += trace_eprobe.o
 obj-$(CONFIG_TRACE_EVENT_INJECT) += trace_events_inject.o
 obj-$(CONFIG_SYNTH_EVENTS) += trace_events_synth.o
 obj-$(CONFIG_HIST_TRIGGERS) += trace_events_hist.o
+obj-$(CONFIG_USER_EVENTS) += trace_events_user.o
 obj-$(CONFIG_BPF_EVENTS) += bpf_trace.o
 obj-$(CONFIG_KPROBE_EVENTS) += trace_kprobe.o
 obj-$(CONFIG_TRACEPOINTS) += error_report-traces.o
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
new file mode 100644
index 000000000000..1d96d1c85147
--- /dev/null
+++ b/kernel/trace/trace_events_user.c
@@ -0,0 +1,1194 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Microsoft Corporation.
+ *
+ * Authors:
+ *   Beau Belgrave <beaub@linux.microsoft.com>
+ */
+
+#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)
+#define MAX_FIELD_ARRAY_SIZE (2 * PAGE_SIZE)
+
+static char *register_page_data;
+
+static DEFINE_MUTEX(reg_mutex);
+static DEFINE_HASHTABLE(register_table, 4);
+static DECLARE_BITMAP(page_bitmap, MAX_EVENTS);
+
+/*
+ * Stores per-event properties, as users register events
+ * within a file a user_event might be created if it does not
+ * already exist. These are globally used and their lifetime
+ * is tied to the refcnt member. These cannot go away until the
+ * refcnt reaches zero.
+ */
+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;
+};
+
+/*
+ * Stores per-file events references, as users register events
+ * within a file this structure is modified and freed via RCU.
+ * The lifetime of this struct is tied to the lifetime of the file.
+ * These are not shared and only accessible by the file that created it.
+ */
+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)
+{
+	const char *start = strchr(type, '[');
+	char val[8];
+	int size = 0;
+
+	if (start == NULL)
+		return -EINVAL;
+
+	start++;
+
+	while (*start != ']' && size < (sizeof(val) - 1))
+		val[size++] = *start++;
+
+	if (*start != ']')
+		return -EINVAL;
+
+	val[size] = 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)
+		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;
+	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);
+}
+
+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);
+
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static struct trace_event_fields user_event_fields_array[1];
+
+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);
+
+	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 ||
+	    !(file->flags & EVENT_FILE_FL_ENABLED) ||
+	    trace_trigger_soft_disabled(file))
+		return;
+
+	/* Allocates and fills trace_entry, + 1 of this is data payload */
+	entry = trace_event_buffer_reserve(&event_buffer, file,
+					   sizeof(*entry) + datalen);
+
+	if (unlikely(!entry))
+		return;
+
+	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)
+					status |= EVENT_STATUS_FTRACE;
+				else
+					status |= EVENT_STATUS_OTHER;
+			} while ((++probe_func_ptr)->func);
+		}
+
+		rcu_read_unlock_sched();
+	}
+
+	register_page_data[user->index] = status;
+}
+
+/*
+ * 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;
+	char *name;
+	int ret;
+
+	if (!str_has_prefix(raw_command, USER_EVENTS_PREFIX))
+		return -ECANCELED;
+
+	raw_command += USER_EVENTS_PREFIX_LEN;
+	raw_command = skip_spaces(raw_command);
+
+	name = kstrdup(raw_command, GFP_KERNEL);
+
+	if (!name)
+		return -ENOMEM;
+
+	mutex_lock(&reg_mutex);
+	ret = user_event_parse_cmd(name, &user);
+	mutex_unlock(&reg_mutex);
+
+	if (ret)
+		kfree(name);
+
+	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)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+
+	return atomic_read(&user->refcnt) != 0;
+}
+
+static int user_event_free(struct dyn_event *ev)
+{
+	struct user_event *user = container_of(ev, struct user_event, devent);
+
+	if (atomic_read(&user->refcnt) != 0)
+		return -EBUSY;
+
+	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.
+ * The name buffer lifetime is owned by this method for success cases only.
+ */
+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;
+		/*
+		 * Name is allocated by caller, free it since it already exists.
+		 * Caller only worries about failure cases for freeing.
+		 */
+		kfree(name);
+		return 0;
+	}
+
+	index = find_first_zero_bit(page_bitmap, MAX_EVENTS);
+
+	if (index == MAX_EVENTS)
+		return -EMFILE;
+
+	user = kzalloc(sizeof(*user), GFP_KERNEL);
+
+	if (!user)
+		return -ENOMEM;
+
+	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, &user->call);
+	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);
+	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);
+
+	/*
+	 * 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;
+
+		kdata = kmalloc(i->count, GFP_KERNEL);
+
+		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)
+{
+	return user_events_write_core(kp->ki_filp, i);
+}
+
+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;
+
+	refs = rcu_dereference_protected(file->private_data,
+					 lockdep_is_held(&reg_mutex));
+
+	if (refs) {
+		count = refs->count;
+
+		for (i = 0; i < count; ++i)
+			if (refs->events[i] == user)
+				return i;
+	}
+
+	size = struct_size(refs, events, 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;
+
+	atomic_inc(&user->refcnt);
+
+	rcu_assign_pointer(file->private_data, new_refs);
+
+	if (refs)
+		kfree_rcu(refs, rcu);
+
+	return i;
+}
+
+static long user_reg_get(struct user_reg __user *ureg, struct user_reg *kreg)
+{
+	u32 size;
+	long ret;
+
+	ret = get_user(size, &ureg->size);
+
+	if (ret)
+		return ret;
+
+	if (size > PAGE_SIZE)
+		return -E2BIG;
+
+	return copy_struct_from_user(kreg, sizeof(*kreg), ureg, size);
+}
+
+/*
+ * Registers a user_event on behalf of a user process.
+ */
+static long user_events_ioctl_reg(struct file *file, unsigned long uarg)
+{
+	struct user_reg __user *ureg = (struct user_reg __user *)uarg;
+	struct user_reg reg;
+	struct user_event *user;
+	char *name;
+	long ret;
+
+	ret = user_reg_get(ureg, &reg);
+
+	if (ret)
+		return ret;
+
+	name = strndup_user((const char __user *)(uintptr_t)reg.name_args,
+			    MAX_EVENT_DESC);
+
+	if (IS_ERR(name)) {
+		ret = PTR_ERR(name);
+		return ret;
+	}
+
+	ret = user_event_parse_cmd(name, &user);
+
+	if (ret) {
+		kfree(name);
+		return ret;
+	}
+
+	ret = user_events_ref_add(file, user);
+
+	/* Positive number is index and valid */
+	if (ret < 0)
+		return ret;
+
+	put_user((u32)ret, &ureg->write_index);
+	put_user(user->index, &ureg->status_index);
+
+	return 0;
+}
+
+/*
+ * Deletes a user_event on behalf of a user process.
+ */
+static long user_events_ioctl_del(struct file *file, unsigned long uarg)
+{
+	void __user *ubuf = (void __user *)uarg;
+	char *name;
+	long ret;
+
+	name = strndup_user(ubuf, MAX_EVENT_DESC);
+
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
+	ret = delete_user_event(name);
+
+	kfree(name);
+
+	return ret;
+}
+
+/*
+ * Handles the ioctl from user mode to register or alter operations.
+ */
+static long user_events_ioctl(struct file *file, unsigned int cmd,
+			      unsigned long uarg)
+{
+	long ret = -ENOTTY;
+
+	switch (cmd) {
+	case DIAG_IOCSREG:
+		mutex_lock(&reg_mutex);
+		ret = user_events_ioctl_reg(file, uarg);
+		mutex_unlock(&reg_mutex);
+		break;
+
+	case DIAG_IOCSDEL:
+		mutex_lock(&reg_mutex);
+		ret = user_events_ioctl_del(file, uarg);
+		mutex_unlock(&reg_mutex);
+		break;
+	}
+
+	return ret;
+}
+
+/*
+ * Handles the final close of the file from user mode.
+ */
+static int user_events_release(struct inode *node, struct file *file)
+{
+	struct user_event_refs *refs;
+	struct user_event *user;
+	int i;
+
+	/*
+	 * Ensure refs cannot change under any situation by taking the
+	 * register mutex during the final freeing of the references.
+	 */
+	mutex_lock(&reg_mutex);
+
+	refs = file->private_data;
+
+	if (!refs)
+		goto out;
+
+	/*
+	 * The lifetime of refs has reached an end, it's tied to this file.
+	 * The underlying user_events are ref counted, and cannot be freed.
+	 * After this decrement, the user_events may be freed elsewhere.
+	 */
+	for (i = 0; i < refs->count; ++i) {
+		user = refs->events[i];
+
+		if (user)
+			atomic_dec(&user->refcnt);
+	}
+out:
+	file->private_data = NULL;
+
+	mutex_unlock(&reg_mutex);
+
+	kfree(refs);
+
+	return 0;
+}
+
+static const struct file_operations user_data_fops = {
+	.write = user_events_write,
+	.write_iter = user_events_write_iter,
+	.unlocked_ioctl	= user_events_ioctl,
+	.release = user_events_release,
+};
+
+/*
+ * Maps the shared page into the user process for checking if event is enabled.
+ */
+static int user_status_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	unsigned long size = vma->vm_end - vma->vm_start;
+
+	if (size != MAX_EVENTS)
+		return -EINVAL;
+
+	return remap_pfn_range(vma, vma->vm_start,
+			       virt_to_phys(register_page_data) >> PAGE_SHIFT,
+			       size, vm_get_page_prot(VM_READ));
+}
+
+static int user_status_show(struct seq_file *m, void *p)
+{
+	struct user_event *user;
+	char status;
+	int i, active = 0, busy = 0, flags;
+
+	mutex_lock(&reg_mutex);
+
+	hash_for_each(register_table, i, user, node) {
+		status = register_page_data[user->index];
+		flags = user->flags;
+
+		seq_printf(m, "%d:%s", user->index, EVENT_NAME(user));
+
+		if (flags != 0 || status != 0)
+			seq_puts(m, " #");
+
+		if (status != 0) {
+			seq_puts(m, " Used by");
+			if (status & EVENT_STATUS_FTRACE)
+				seq_puts(m, " ftrace");
+			if (status & EVENT_STATUS_PERF)
+				seq_puts(m, " perf");
+			if (status & EVENT_STATUS_OTHER)
+				seq_puts(m, " other");
+			busy++;
+		}
+
+		if (flags & FLAG_BPF_ITER)
+			seq_puts(m, " FLAG:BPF_ITER");
+
+		seq_puts(m, "\n");
+		active++;
+	}
+
+	mutex_unlock(&reg_mutex);
+
+	seq_puts(m, "\n");
+	seq_printf(m, "Active: %d\n", active);
+	seq_printf(m, "Busy: %d\n", busy);
+	seq_printf(m, "Max: %ld\n", MAX_EVENTS);
+
+	return 0;
+}
+
+static ssize_t user_status_read(struct file *file, char __user *ubuf,
+				size_t count, loff_t *ppos)
+{
+	/*
+	 * Delay allocation of seq data until requested, most callers
+	 * will never read the status file. They will only mmap.
+	 */
+	if (file->private_data == NULL) {
+		int ret;
+
+		if (*ppos != 0)
+			return -EINVAL;
+
+		ret = single_open(file, user_status_show, NULL);
+
+		if (ret)
+			return ret;
+	}
+
+	return seq_read(file, ubuf, count, ppos);
+}
+
+static loff_t user_status_seek(struct file *file, loff_t offset, int whence)
+{
+	if (file->private_data == NULL)
+		return 0;
+
+	return seq_lseek(file, offset, whence);
+}
+
+static int user_status_release(struct inode *node, struct file *file)
+{
+	if (file->private_data == NULL)
+		return 0;
+
+	return single_release(node, file);
+}
+
+static const struct file_operations user_status_fops = {
+	.mmap = user_status_mmap,
+	.read = user_status_read,
+	.llseek  = user_status_seek,
+	.release = user_status_release,
+};
+
+/*
+ * Creates a set of tracefs files to allow user mode interactions.
+ */
+static int create_user_tracefs(void)
+{
+	struct dentry *edata, *emmap;
+
+	edata = tracefs_create_file("user_events_data", TRACE_MODE_WRITE,
+				    NULL, NULL, &user_data_fops);
+
+	if (!edata) {
+		pr_warn("Could not create tracefs 'user_events_data' entry\n");
+		goto err;
+	}
+
+	/* mmap with MAP_SHARED requires writable fd */
+	emmap = tracefs_create_file("user_events_status", TRACE_MODE_WRITE,
+				    NULL, NULL, &user_status_fops);
+
+	if (!emmap) {
+		tracefs_remove(edata);
+		pr_warn("Could not create tracefs 'user_events_mmap' entry\n");
+		goto err;
+	}
+
+	return 0;
+err:
+	return -ENODEV;
+}
+
+static void set_page_reservations(bool set)
+{
+	int page;
+
+	for (page = 0; page < MAX_PAGES; ++page) {
+		void *addr = register_page_data + (PAGE_SIZE * page);
+
+		if (set)
+			SetPageReserved(virt_to_page(addr));
+		else
+			ClearPageReserved(virt_to_page(addr));
+	}
+}
+
+static int __init trace_events_user_init(void)
+{
+	int ret;
+
+	/* Zero all bits beside 0 (which is reserved for failures) */
+	bitmap_zero(page_bitmap, MAX_EVENTS);
+	set_bit(0, page_bitmap);
+
+	register_page_data = kzalloc(MAX_EVENTS, GFP_KERNEL);
+
+	if (!register_page_data)
+		return -ENOMEM;
+
+	set_page_reservations(true);
+
+	ret = create_user_tracefs();
+
+	if (ret) {
+		pr_warn("user_events could not register with tracefs\n");
+		set_page_reservations(false);
+		kfree(register_page_data);
+		return ret;
+	}
+
+	if (dyn_event_register(&user_event_dops))
+		pr_warn("user_events could not register with dyn_events\n");
+
+	return 0;
+}
+
+fs_initcall(trace_events_user_init);
-- 
2.17.1

[PATCH v7 03/13] user_events: Add print_fmt generation support for basic types

[Beau Belgrave]

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 <beaub@linux.microsoft.com>
---
 kernel/trace/trace_events_user.c | 107 ++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 1d96d1c85147..bd8ac46fddb1 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -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;
-- 
2.17.1

[PATCH v7 04/13] user_events: Handle matching arguments from dyn_events

[Beau Belgrave]

Ensures that when dynamic events requests a match with arguments that
they match what is in the user_event.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 kernel/trace/trace_events_user.c | 77 +++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index bd8ac46fddb1..ae3ceaaa9098 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -39,6 +39,7 @@
 #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_ARG_NAME 256
 
 static char *register_page_data;
 
@@ -692,13 +693,87 @@ static int user_event_free(struct dyn_event *ev)
 	return destroy_user_event(user);
 }
 
+static bool user_field_match(struct ftrace_event_field *field, int argc,
+			     const char **argv, int *iout)
+{
+	char *field_name, *arg_name;
+	int len, pos, i = *iout;
+	bool colon = false, match = false;
+
+	if (i >= argc)
+		return false;
+
+	len = MAX_FIELD_ARG_NAME;
+	field_name = kmalloc(len, GFP_KERNEL);
+	arg_name = kmalloc(len, GFP_KERNEL);
+
+	if (!arg_name || !field_name)
+		goto out;
+
+	pos = 0;
+
+	for (; i < argc; ++i) {
+		if (i != *iout)
+			pos += snprintf(arg_name + pos, len - pos, " ");
+
+		pos += snprintf(arg_name + pos, len - pos, argv[i]);
+
+		if (strchr(argv[i], ';')) {
+			++i;
+			colon = true;
+			break;
+		}
+	}
+
+	pos = 0;
+
+	pos += snprintf(field_name + pos, len - pos, field->type);
+	pos += snprintf(field_name + pos, len - pos, " ");
+	pos += snprintf(field_name + pos, len - pos, field->name);
+
+	if (colon)
+		pos += snprintf(field_name + pos, len - pos, ";");
+
+	*iout = i;
+
+	match = strcmp(arg_name, field_name) == 0;
+out:
+	kfree(arg_name);
+	kfree(field_name);
+
+	return match;
+}
+
+static bool user_fields_match(struct user_event *user, int argc,
+			      const char **argv)
+{
+	struct ftrace_event_field *field, *next;
+	struct list_head *head = &user->fields;
+	int i = 0;
+
+	list_for_each_entry_safe_reverse(field, next, head, link)
+		if (!user_field_match(field, argc, argv, &i))
+			return false;
+
+	if (i != argc)
+		return false;
+
+	return true;
+}
+
 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);
+	bool match;
 
-	return strcmp(EVENT_NAME(user), event) == 0 &&
+	match = strcmp(EVENT_NAME(user), event) == 0 &&
 		(!system || strcmp(system, USER_EVENTS_SYSTEM) == 0);
+
+	if (match && argc > 0)
+		match = user_fields_match(user, argc, argv);
+
+	return match;
 }
 
 static struct dyn_event_operations user_event_dops = {
-- 
2.17.1

[PATCH v7 05/13] user_events: Add basic perf and eBPF support

[Beau Belgrave]

Adds support to write out user_event data to perf_probe/perf files as
well as to any attached eBPF program.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 kernel/trace/trace_events_user.c | 63 ++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index ae3ceaaa9098..807db0af74fb 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -541,6 +541,50 @@ static void user_event_ftrace(struct user_event *user, void *data, u32 datalen,
 	trace_event_buffer_commit(&event_buffer);
 }
 
+#ifdef CONFIG_PERF_EVENTS
+/*
+ * 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,
+			    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);
+	}
+
+	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;
+		int context;
+
+		perf_entry = perf_trace_buf_alloc(ALIGN(size, 8),
+						  &regs, &context);
+
+		if (unlikely(!perf_entry))
+			return;
+
+		perf_fetch_caller_regs(regs);
+
+		memcpy(perf_entry + 1, data, datalen);
+
+		perf_trace_buf_submit(perf_entry, size, context,
+				      user->call.event.type, 1, regs,
+				      perf_head, NULL);
+	}
+}
+#endif
+
 /*
  * Update the register page that is shared between user processes.
  */
@@ -563,6 +607,10 @@ static void update_reg_page_for(struct user_event *user)
 
 				if (probe_func == user_event_ftrace)
 					status |= EVENT_STATUS_FTRACE;
+#ifdef CONFIG_PERF_EVENTS
+				else if (probe_func == user_event_perf)
+					status |= EVENT_STATUS_PERF;
+#endif
 				else
 					status |= EVENT_STATUS_OTHER;
 			} while ((++probe_func_ptr)->func);
@@ -604,7 +652,19 @@ static int user_event_reg(struct trace_event_call *call,
 
 #ifdef CONFIG_PERF_EVENTS
 	case TRACE_REG_PERF_REGISTER:
+		ret = tracepoint_probe_register(call->tp,
+						call->class->perf_probe,
+						data);
+		if (!ret)
+			goto inc;
+		break;
+
 	case TRACE_REG_PERF_UNREGISTER:
+		tracepoint_probe_unregister(call->tp,
+					    call->class->perf_probe,
+					    data);
+		goto dec;
+
 	case TRACE_REG_PERF_OPEN:
 	case TRACE_REG_PERF_CLOSE:
 	case TRACE_REG_PERF_ADD:
@@ -862,6 +922,9 @@ static int user_event_parse(char *name, char *args, char *flags,
 	user->class.get_fields = user_event_get_fields;
 	user->class.reg = user_event_reg;
 	user->class.probe = user_event_ftrace;
+#ifdef CONFIG_PERF_EVENTS
+	user->class.perf_probe = user_event_perf;
+#endif
 
 	mutex_lock(&event_mutex);
 	ret = user_event_trace_register(user);
-- 
2.17.1

[PATCH v7 06/13] user_events: Add self-test for ftrace integration

[Beau Belgrave]

Tests basic functionality of registering/deregistering, status and
writing data out via ftrace mechanisms within user_events.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 tools/testing/selftests/user_events/Makefile  |   9 +
 .../selftests/user_events/ftrace_test.c       | 205 ++++++++++++++++++
 tools/testing/selftests/user_events/settings  |   1 +
 3 files changed, 215 insertions(+)
 create mode 100644 tools/testing/selftests/user_events/Makefile
 create mode 100644 tools/testing/selftests/user_events/ftrace_test.c
 create mode 100644 tools/testing/selftests/user_events/settings

diff --git a/tools/testing/selftests/user_events/Makefile b/tools/testing/selftests/user_events/Makefile
new file mode 100644
index 000000000000..d66c551a6fe3
--- /dev/null
+++ b/tools/testing/selftests/user_events/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+CFLAGS += -Wl,-no-as-needed -Wall -I../../../../usr/include
+LDLIBS += -lrt -lpthread -lm
+
+TEST_GEN_PROGS = ftrace_test
+
+TEST_FILES := settings
+
+include ../lib.mk
diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
new file mode 100644
index 000000000000..9d53717139e6
--- /dev/null
+++ b/tools/testing/selftests/user_events/ftrace_test.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * User Events FTrace Test Program
+ *
+ * Copyright (c) 2021 Beau Belgrave <beaub@linux.microsoft.com>
+ */
+
+#include <errno.h>
+#include <linux/user_events.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "../kselftest_harness.h"
+
+const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
+const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
+const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable";
+const char *trace_file = "/sys/kernel/debug/tracing/trace";
+
+static int trace_bytes(void)
+{
+	int fd = open(trace_file, O_RDONLY);
+	char buf[256];
+	int bytes = 0, got;
+
+	if (fd == -1)
+		return -1;
+
+	while (true) {
+		got = read(fd, buf, sizeof(buf));
+
+		if (got == -1)
+			return -1;
+
+		if (got == 0)
+			break;
+
+		bytes += got;
+	}
+
+	close(fd);
+
+	return bytes;
+}
+
+FIXTURE(user) {
+	int status_fd;
+	int data_fd;
+	int enable_fd;
+};
+
+FIXTURE_SETUP(user) {
+	self->status_fd = open(status_file, O_RDONLY);
+	ASSERT_NE(-1, self->status_fd);
+
+	self->data_fd = open(data_file, O_RDWR);
+	ASSERT_NE(-1, self->data_fd);
+
+	self->enable_fd = -1;
+}
+
+FIXTURE_TEARDOWN(user) {
+	close(self->status_fd);
+	close(self->data_fd);
+
+	if (self->enable_fd != -1) {
+		write(self->enable_fd, "0", sizeof("0"));
+		close(self->enable_fd);
+	}
+}
+
+TEST_F(user, register_events) {
+	struct user_reg reg = {0};
+	int page_size = sysconf(_SC_PAGESIZE);
+	char *status_page;
+
+	reg.size = sizeof(reg);
+	reg.name_args = (__u64)"__test_event u32 field1; u32 field2";
+
+	status_page = mmap(NULL, page_size, PROT_READ, MAP_SHARED,
+			   self->status_fd, 0);
+
+	/* 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);
+
+	/* Multiple registers should result in same index */
+	ASSERT_EQ(0, ioctl(self->data_fd, DIAG_IOCSREG, &reg));
+	ASSERT_EQ(0, reg.write_index);
+	ASSERT_NE(0, reg.status_index);
+
+	/* Ensure disabled */
+	self->enable_fd = open(enable_file, O_RDWR);
+	ASSERT_NE(-1, self->enable_fd);
+	ASSERT_NE(-1, write(self->enable_fd, "0", sizeof("0")))
+
+	/* MMAP should work and be zero'd */
+	ASSERT_NE(MAP_FAILED, status_page);
+	ASSERT_NE(NULL, status_page);
+	ASSERT_EQ(0, status_page[reg.status_index]);
+
+	/* Enable event and ensure bits updated in status */
+	ASSERT_NE(-1, write(self->enable_fd, "1", sizeof("1")))
+	ASSERT_EQ(EVENT_STATUS_FTRACE, status_page[reg.status_index]);
+
+	/* Disable event and ensure bits updated in status */
+	ASSERT_NE(-1, write(self->enable_fd, "0", sizeof("0")))
+	ASSERT_EQ(0, status_page[reg.status_index]);
+
+	/* File still open should return -EBUSY for delete */
+	ASSERT_EQ(-1, ioctl(self->data_fd, DIAG_IOCSDEL, "__test_event"));
+	ASSERT_EQ(EBUSY, errno);
+
+	/* Delete should work only after close */
+	close(self->data_fd);
+	self->data_fd = open(data_file, O_RDWR);
+	ASSERT_EQ(0, ioctl(self->data_fd, DIAG_IOCSDEL, "__test_event"));
+
+	/* Unmap should work */
+	ASSERT_EQ(0, munmap(status_page, page_size));
+}
+
+TEST_F(user, write_events) {
+	struct user_reg reg = {0};
+	struct iovec io[3];
+	__u32 field1, field2;
+	int before = 0, after = 0;
+
+	reg.size = sizeof(reg);
+	reg.name_args = (__u64)"__test_event u32 field1; u32 field2";
+
+	field1 = 1;
+	field2 = 2;
+
+	io[0].iov_base = &reg.write_index;
+	io[0].iov_len = sizeof(reg.write_index);
+	io[1].iov_base = &field1;
+	io[1].iov_len = sizeof(field1);
+	io[2].iov_base = &field2;
+	io[2].iov_len = sizeof(field2);
+
+	/* 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);
+
+	/* Write should fail on invalid slot with ENOENT */
+	io[0].iov_base = &field2;
+	io[0].iov_len = sizeof(field2);
+	ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	ASSERT_EQ(ENOENT, errno);
+	io[0].iov_base = &reg.write_index;
+	io[0].iov_len = sizeof(reg.write_index);
+
+	/* Enable event */
+	self->enable_fd = open(enable_file, O_RDWR);
+	ASSERT_NE(-1, write(self->enable_fd, "1", sizeof("1")))
+
+	/* Write should make it out to ftrace buffers */
+	before = trace_bytes();
+	ASSERT_NE(-1, writev(self->data_fd, (const struct iovec *)io, 3));
+	after = trace_bytes();
+	ASSERT_GT(after, before);
+}
+
+TEST_F(user, write_fault) {
+	struct user_reg reg = {0};
+	struct iovec io[2];
+	int l = sizeof(__u64);
+	void *anon;
+
+	reg.size = sizeof(reg);
+	reg.name_args = (__u64)"__test_event u64 anon";
+
+	anon = mmap(NULL, l, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	ASSERT_NE(MAP_FAILED, anon);
+
+	io[0].iov_base = &reg.write_index;
+	io[0].iov_len = sizeof(reg.write_index);
+	io[1].iov_base = anon;
+	io[1].iov_len = l;
+
+	/* 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);
+
+	/* Write should work normally */
+	ASSERT_NE(-1, writev(self->data_fd, (const struct iovec *)io, 2));
+
+	/* Faulted data should zero fill and work */
+	ASSERT_EQ(0, madvise(anon, l, MADV_DONTNEED));
+	ASSERT_NE(-1, writev(self->data_fd, (const struct iovec *)io, 2));
+	ASSERT_EQ(0, munmap(anon, l));
+}
+
+int main(int argc, char **argv)
+{
+	return test_harness_run(argc, argv);
+}
diff --git a/tools/testing/selftests/user_events/settings b/tools/testing/selftests/user_events/settings
new file mode 100644
index 000000000000..ba4d85f74cd6
--- /dev/null
+++ b/tools/testing/selftests/user_events/settings
@@ -0,0 +1 @@
+timeout=90
-- 
2.17.1

[PATCH v7 07/13] user_events: Add self-test for dynamic_events integration

[Beau Belgrave]

Tests matching deletes, creation of basic and complex types. Ensures
common patterns work correctly when interacting with dynamic_events
file.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 tools/testing/selftests/user_events/Makefile  |   2 +-
 .../testing/selftests/user_events/dyn_test.c  | 130 ++++++++++++++++++
 2 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/user_events/dyn_test.c

diff --git a/tools/testing/selftests/user_events/Makefile b/tools/testing/selftests/user_events/Makefile
index d66c551a6fe3..e824b9c2cae7 100644
--- a/tools/testing/selftests/user_events/Makefile
+++ b/tools/testing/selftests/user_events/Makefile
@@ -2,7 +2,7 @@
 CFLAGS += -Wl,-no-as-needed -Wall -I../../../../usr/include
 LDLIBS += -lrt -lpthread -lm
 
-TEST_GEN_PROGS = ftrace_test
+TEST_GEN_PROGS = ftrace_test dyn_test
 
 TEST_FILES := settings
 
diff --git a/tools/testing/selftests/user_events/dyn_test.c b/tools/testing/selftests/user_events/dyn_test.c
new file mode 100644
index 000000000000..d6265d14cd51
--- /dev/null
+++ b/tools/testing/selftests/user_events/dyn_test.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * User Events Dyn Events Test Program
+ *
+ * Copyright (c) 2021 Beau Belgrave <beaub@linux.microsoft.com>
+ */
+
+#include <errno.h>
+#include <linux/user_events.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "../kselftest_harness.h"
+
+const char *dyn_file = "/sys/kernel/debug/tracing/dynamic_events";
+const char *clear = "!u:__test_event";
+
+static int Append(const char *value)
+{
+	int fd = open(dyn_file, O_RDWR | O_APPEND);
+	int ret = write(fd, value, strlen(value));
+
+	close(fd);
+	return ret;
+}
+
+#define CLEAR() \
+do { \
+	int ret = Append(clear); \
+	if (ret == -1) \
+		ASSERT_EQ(ENOENT, errno); \
+} while (0)
+
+#define TEST_PARSE(x) \
+do { \
+	ASSERT_NE(-1, Append(x)); \
+	CLEAR(); \
+} while (0)
+
+#define TEST_NPARSE(x) ASSERT_EQ(-1, Append(x))
+
+FIXTURE(user) {
+};
+
+FIXTURE_SETUP(user) {
+	CLEAR();
+}
+
+FIXTURE_TEARDOWN(user) {
+	CLEAR();
+}
+
+TEST_F(user, basic_types) {
+	/* All should work */
+	TEST_PARSE("u:__test_event u64 a");
+	TEST_PARSE("u:__test_event u32 a");
+	TEST_PARSE("u:__test_event u16 a");
+	TEST_PARSE("u:__test_event u8 a");
+	TEST_PARSE("u:__test_event char a");
+	TEST_PARSE("u:__test_event unsigned char a");
+	TEST_PARSE("u:__test_event int a");
+	TEST_PARSE("u:__test_event unsigned int a");
+	TEST_PARSE("u:__test_event short a");
+	TEST_PARSE("u:__test_event unsigned short a");
+	TEST_PARSE("u:__test_event char[20] a");
+	TEST_PARSE("u:__test_event unsigned char[20] a");
+	TEST_PARSE("u:__test_event char[0x14] a");
+	TEST_PARSE("u:__test_event unsigned char[0x14] a");
+	/* Bad size format should fail */
+	TEST_NPARSE("u:__test_event char[aa] a");
+	/* Large size should fail */
+	TEST_NPARSE("u:__test_event char[9999] a");
+	/* Long size string should fail */
+	TEST_NPARSE("u:__test_event char[0x0000000000001] a");
+}
+
+TEST_F(user, loc_types) {
+	/* All should work */
+	TEST_PARSE("u:__test_event __data_loc char[] a");
+	TEST_PARSE("u:__test_event __data_loc unsigned char[] a");
+	TEST_PARSE("u:__test_event __rel_loc char[] a");
+	TEST_PARSE("u:__test_event __rel_loc unsigned char[] a");
+}
+
+TEST_F(user, size_types) {
+	/* Should work */
+	TEST_PARSE("u:__test_event struct custom a 20");
+	/* Size not specified on struct should fail */
+	TEST_NPARSE("u:__test_event struct custom a");
+	/* Size specified on non-struct should fail */
+	TEST_NPARSE("u:__test_event char a 20");
+}
+
+TEST_F(user, flags) {
+	/* Should work */
+	TEST_PARSE("u:__test_event:BPF_ITER u32 a");
+	/* Forward compat */
+	TEST_PARSE("u:__test_event:BPF_ITER,FLAG_FUTURE u32 a");
+}
+
+TEST_F(user, matching) {
+	/* Register */
+	ASSERT_NE(-1, Append("u:__test_event struct custom a 20"));
+	/* Should not match */
+	TEST_NPARSE("!u:__test_event struct custom b");
+	/* Should match */
+	TEST_PARSE("!u:__test_event struct custom a");
+	/* Multi field reg */
+	ASSERT_NE(-1, Append("u:__test_event u32 a; u32 b"));
+	/* Non matching cases */
+	TEST_NPARSE("!u:__test_event u32 a");
+	TEST_NPARSE("!u:__test_event u32 b");
+	TEST_NPARSE("!u:__test_event u32 a; u32 ");
+	TEST_NPARSE("!u:__test_event u32 a; u32 a");
+	/* Matching case */
+	TEST_PARSE("!u:__test_event u32 a; u32 b");
+	/* Register */
+	ASSERT_NE(-1, Append("u:__test_event u32 a; u32 b"));
+	/* Ensure trailing semi-colon case */
+	TEST_PARSE("!u:__test_event u32 a; u32 b;");
+}
+
+int main(int argc, char **argv)
+{
+	return test_harness_run(argc, argv);
+}
-- 
2.17.1

[PATCH v7 08/13] user_events: Add self-test for perf_event integration

[Beau Belgrave]

Tests perf can be attached to and written out correctly. Ensures attach
updates status bits in user programs.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 tools/testing/selftests/user_events/Makefile  |   2 +-
 .../testing/selftests/user_events/perf_test.c | 168 ++++++++++++++++++
 2 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/user_events/perf_test.c

diff --git a/tools/testing/selftests/user_events/Makefile b/tools/testing/selftests/user_events/Makefile
index e824b9c2cae7..c765d8635d9a 100644
--- a/tools/testing/selftests/user_events/Makefile
+++ b/tools/testing/selftests/user_events/Makefile
@@ -2,7 +2,7 @@
 CFLAGS += -Wl,-no-as-needed -Wall -I../../../../usr/include
 LDLIBS += -lrt -lpthread -lm
 
-TEST_GEN_PROGS = ftrace_test dyn_test
+TEST_GEN_PROGS = ftrace_test dyn_test perf_test
 
 TEST_FILES := settings
 
diff --git a/tools/testing/selftests/user_events/perf_test.c b/tools/testing/selftests/user_events/perf_test.c
new file mode 100644
index 000000000000..26851d51d6bb
--- /dev/null
+++ b/tools/testing/selftests/user_events/perf_test.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * User Events Perf Events Test Program
+ *
+ * Copyright (c) 2021 Beau Belgrave <beaub@linux.microsoft.com>
+ */
+
+#include <errno.h>
+#include <linux/user_events.h>
+#include <linux/perf_event.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <asm/unistd.h>
+
+#include "../kselftest_harness.h"
+
+const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
+const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
+const char *id_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/id";
+const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format";
+
+struct event {
+	__u32 index;
+	__u32 field1;
+	__u32 field2;
+};
+
+static long perf_event_open(struct perf_event_attr *pe, pid_t pid,
+			    int cpu, int group_fd, unsigned long flags)
+{
+	return syscall(__NR_perf_event_open, pe, pid, cpu, group_fd, flags);
+}
+
+static int get_id(void)
+{
+	FILE *fp = fopen(id_file, "r");
+	int ret, id = 0;
+
+	if (!fp)
+		return -1;
+
+	ret = fscanf(fp, "%d", &id);
+	fclose(fp);
+
+	if (ret != 1)
+		return -1;
+
+	return id;
+}
+
+static int get_offset(void)
+{
+	FILE *fp = fopen(fmt_file, "r");
+	int ret, c, last = 0, offset = 0;
+
+	if (!fp)
+		return -1;
+
+	/* Read until empty line */
+	while (true) {
+		c = getc(fp);
+
+		if (c == EOF)
+			break;
+
+		if (last == '\n' && c == '\n')
+			break;
+
+		last = c;
+	}
+
+	ret = fscanf(fp, "\tfield:u32 field1;\toffset:%d;", &offset);
+	fclose(fp);
+
+	if (ret != 1)
+		return -1;
+
+	return offset;
+}
+
+FIXTURE(user) {
+	int status_fd;
+	int data_fd;
+};
+
+FIXTURE_SETUP(user) {
+	self->status_fd = open(status_file, O_RDONLY);
+	ASSERT_NE(-1, self->status_fd);
+
+	self->data_fd = open(data_file, O_RDWR);
+	ASSERT_NE(-1, self->data_fd);
+}
+
+FIXTURE_TEARDOWN(user) {
+	close(self->status_fd);
+	close(self->data_fd);
+}
+
+TEST_F(user, perf_write) {
+	struct perf_event_attr pe = {0};
+	struct user_reg reg = {0};
+	int page_size = sysconf(_SC_PAGESIZE);
+	char *status_page;
+	struct event event;
+	struct perf_event_mmap_page *perf_page;
+	int id, fd, offset;
+	__u32 *val;
+
+	reg.size = sizeof(reg);
+	reg.name_args = (__u64)"__test_event u32 field1; u32 field2";
+
+	status_page = mmap(NULL, page_size, PROT_READ, MAP_SHARED,
+			   self->status_fd, 0);
+	ASSERT_NE(MAP_FAILED, status_page);
+
+	/* 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);
+	ASSERT_EQ(0, status_page[reg.status_index]);
+
+	/* Id should be there */
+	id = get_id();
+	ASSERT_NE(-1, id);
+	offset = get_offset();
+	ASSERT_NE(-1, offset);
+
+	pe.type = PERF_TYPE_TRACEPOINT;
+	pe.size = sizeof(pe);
+	pe.config = id;
+	pe.sample_type = PERF_SAMPLE_RAW;
+	pe.sample_period = 1;
+	pe.wakeup_events = 1;
+
+	/* Tracepoint attach should work */
+	fd = perf_event_open(&pe, 0, -1, -1, 0);
+	ASSERT_NE(-1, fd);
+
+	perf_page = mmap(NULL, page_size * 2, PROT_READ, MAP_SHARED, fd, 0);
+	ASSERT_NE(MAP_FAILED, perf_page);
+
+	/* Status should be updated */
+	ASSERT_EQ(EVENT_STATUS_PERF, status_page[reg.status_index]);
+
+	event.index = reg.write_index;
+	event.field1 = 0xc001;
+	event.field2 = 0xc01a;
+
+	/* Ensure write shows up at correct offset */
+	ASSERT_NE(-1, write(self->data_fd, &event, sizeof(event)));
+	val = (void *)(((char *)perf_page) + perf_page->data_offset);
+	ASSERT_EQ(PERF_RECORD_SAMPLE, *val);
+	/* Skip over header and size, move to offset */
+	val += 3;
+	val = (void *)((char *)val) + offset;
+	/* Ensure correct */
+	ASSERT_EQ(event.field1, *val++);
+	ASSERT_EQ(event.field2, *val++);
+}
+
+int main(int argc, char **argv)
+{
+	return test_harness_run(argc, argv);
+}
-- 
2.17.1

[PATCH v7 09/13] user_events: Optimize writing events by only copying data once

[Beau Belgrave]

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 <beaub@linux.microsoft.com>
---
 kernel/trace/trace_events_user.c | 102 ++++++++++++++++++++++---------
 1 file changed, 74 insertions(+), 28 deletions(-)

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 807db0af74fb..1d29f6ec907d 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -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);
-
-		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;
 
 		rcu_read_lock_sched();
+		pagefault_disable();
 
 		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;
-- 
2.17.1

[PATCH v7 10/13] user_events: Add documentation file

[Beau Belgrave]

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

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 Documentation/trace/index.rst       |   1 +
 Documentation/trace/user_events.rst | 195 ++++++++++++++++++++++++++++
 2 files changed, 196 insertions(+)
 create mode 100644 Documentation/trace/user_events.rst

diff --git a/Documentation/trace/index.rst b/Documentation/trace/index.rst
index 3769b9b7aed8..3a47aa8341c6 100644
--- a/Documentation/trace/index.rst
+++ b/Documentation/trace/index.rst
@@ -30,3 +30,4 @@ Linux Tracing Technologies
    stm
    sys-t
    coresight/index
+   user_events
diff --git a/Documentation/trace/user_events.rst b/Documentation/trace/user_events.rst
new file mode 100644
index 000000000000..e77e71b2fe9b
--- /dev/null
+++ b/Documentation/trace/user_events.rst
@@ -0,0 +1,195 @@
+=========================================
+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
+status index. This index describes which byte in the
+/sys/kernel/debug/tracing/user_events_status file represents this event. The
+second int is the write index. This index describes the data when a write() or
+writev() is called on the /sys/kernel/debug/tracing/user_events_data file.
+
+The structures referenced in this document are contained with the
+/include/uap/linux/user_events.h file in the source tree.
+
+**NOTE:** *Both user_events_status and user_events_data are under the tracefs
+filesystem and may be mounted at different paths than above.*
+
+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 use /sys/kernel/debug/tracing/events/user_events/[name]/enable
+or perf record -e user_events:[name] when attaching/recording.
+
+**NOTE:** *The write_index returned is only valid for the FD that was used*
+
+Command Format
+^^^^^^^^^^^^^^
+The command string format is as follows::
+
+  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], etc).
+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) in size,
+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.*
+
+EBPF
+----
+EBPF programs that attach to a user-based event tracepoint are given a pointer
+to a struct user_bpf_context. The bpf context contains the data type (which can
+be a user or kernel buffer, or can be a pointer to the iovec) and the data
+length that was emitted (minus the write_index).
+
+Example Code
+------------
+See sample code in samples/user_events.
-- 
2.17.1

[PATCH v7 11/13] user_events: Add sample code for typical usage

[Beau Belgrave]

Add sample code for user_events typical usage to show how to register
and monitor status, as well as to write out data.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 samples/user_events/Makefile  |  5 ++
 samples/user_events/example.c | 91 +++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)
 create mode 100644 samples/user_events/Makefile
 create mode 100644 samples/user_events/example.c

diff --git a/samples/user_events/Makefile b/samples/user_events/Makefile
new file mode 100644
index 000000000000..7252b589db57
--- /dev/null
+++ b/samples/user_events/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+CFLAGS += -Wl,-no-as-needed -Wall -I../../usr/include
+
+example: example.o
+example.o: example.c
diff --git a/samples/user_events/example.c b/samples/user_events/example.c
new file mode 100644
index 000000000000..4f5778e441c0
--- /dev/null
+++ b/samples/user_events/example.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Microsoft Corporation.
+ *
+ * Authors:
+ *   Beau Belgrave <beaub@linux.microsoft.com>
+ */
+
+#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 = &write;
+	io[0].iov_len = sizeof(write);
+	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);
+
+		/* Increase the count */
+		count++;
+
+		printf("Something was attached, wrote data\n");
+	}
+
+	goto ask;
+
+	return 0;
+}
-- 
2.17.1

[PATCH v7 12/13] user_events: Validate user payloads for size and null termination

[Beau Belgrave]

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 <beaub@linux.microsoft.com>
---
 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(-)

diff --git a/include/uapi/linux/user_events.h b/include/uapi/linux/user_events.h
index 5bff99418deb..f97db05e00c9 100644
--- a/include/uapi/linux/user_events.h
+++ b/include/uapi/linux/user_events.h
@@ -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)
 
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 1d29f6ec907d..56eb58ddb4cf 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -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;
diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
index 9d53717139e6..16aff1fb295a 100644
--- a/tools/testing/selftests/user_events/ftrace_test.c
+++ b/tools/testing/selftests/user_events/ftrace_test.c
@@ -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);
-- 
2.17.1

[PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields

[Beau Belgrave]

Switch between __get_str and __get_rel_str within the print_fmt of
user_events. Add unit test to ensure print_fmt is correct on known
types.

Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
---
 kernel/trace/trace_events_user.c              |  24 ++-
 .../selftests/user_events/ftrace_test.c       | 166 ++++++++++++++++++
 2 files changed, 182 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 56eb58ddb4cf..3779fa2ca14a 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
 	goto add_field;
 
 add_validator:
-	if (strstr(type, "char[") != 0)
+	if (strstr(type, "char") != 0)
 		validator_flags |= VALIDATOR_ENSURE_NULL;
 
 	validator = kmalloc(sizeof(*validator), GFP_KERNEL);
@@ -456,14 +456,21 @@ static const char *user_field_format(const char *type)
 	return "%llu";
 }
 
-static bool user_field_is_dyn_string(const char *type)
+static bool user_field_is_dyn_string(const char *type, const char **str_func)
 {
-	if (str_has_prefix(type, "__data_loc ") ||
-	    str_has_prefix(type, "__rel_loc "))
-		if (strstr(type, "char[") != 0)
-			return true;
+	if (str_has_prefix(type, "__data_loc ")) {
+		*str_func = "__get_str";
+		goto check;
+	}
+
+	if (str_has_prefix(type, "__rel_loc ")) {
+		*str_func = "__get_rel_str";
+		goto check;
+	}
 
 	return false;
+check:
+	return strstr(type, "char") != 0;
 }
 
 #define LEN_OR_ZERO (len ? len - pos : 0)
@@ -472,6 +479,7 @@ 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;
+	const char *str_func;
 
 	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
 
@@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
 	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))
+		if (user_field_is_dyn_string(field->type, &str_func))
 			pos += snprintf(buf + pos, LEN_OR_ZERO,
-					", __get_str(%s)", field->name);
+					", %s(%s)", str_func, field->name);
 		else
 			pos += snprintf(buf + pos, LEN_OR_ZERO,
 					", REC->%s", field->name);
diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
index 16aff1fb295a..b2e5c0765a68 100644
--- a/tools/testing/selftests/user_events/ftrace_test.c
+++ b/tools/testing/selftests/user_events/ftrace_test.c
@@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
 const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
 const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable";
 const char *trace_file = "/sys/kernel/debug/tracing/trace";
+const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format";
 
 static int trace_bytes(void)
 {
@@ -47,6 +48,61 @@ static int trace_bytes(void)
 	return bytes;
 }
 
+static int get_print_fmt(char *buffer, int len)
+{
+	FILE *fp = fopen(fmt_file, "r");
+	int c, index = 0, last = 0;
+
+	if (!fp)
+		return -1;
+
+	/* Read until empty line (Skip Common) */
+	while (true) {
+		c = getc(fp);
+
+		if (c == EOF)
+			break;
+
+		if (last == '\n' && c == '\n')
+			break;
+
+		last = c;
+	}
+
+	last = 0;
+
+	/* Read until empty line (Skip Properties) */
+	while (true) {
+		c = getc(fp);
+
+		if (c == EOF)
+			break;
+
+		if (last == '\n' && c == '\n')
+			break;
+
+		last = c;
+	}
+
+	/* Read in print_fmt: */
+	while (len > 1) {
+		c = getc(fp);
+
+		if (c == EOF || c == '\n')
+			break;
+
+		buffer[index++] = c;
+
+		len--;
+	}
+
+	buffer[index] = 0;
+
+	fclose(fp);
+
+	return 0;
+}
+
 static int clear(void)
 {
 	int fd = open(data_file, O_RDWR);
@@ -63,6 +119,44 @@ static int clear(void)
 	return 0;
 }
 
+static int check_print_fmt(const char *event, const char *expected)
+{
+	struct user_reg reg = {0};
+	char print_fmt[256];
+	int ret;
+	int fd;
+
+	/* Ensure cleared */
+	ret = clear();
+
+	if (ret != 0)
+		return ret;
+
+	fd = open(data_file, O_RDWR);
+
+	if (fd == -1)
+		return fd;
+
+	reg.size = sizeof(reg);
+	reg.name_args = (__u64)event;
+
+	/* Register should work */
+	ret = ioctl(fd, DIAG_IOCSREG, &reg);
+
+	close(fd);
+
+	if (ret != 0)
+		return ret;
+
+	/* Ensure correct print_fmt */
+	ret = get_print_fmt(print_fmt, sizeof(print_fmt));
+
+	if (ret != 0)
+		return ret;
+
+	return strcmp(print_fmt, expected);
+}
+
 FIXTURE(user) {
 	int status_fd;
 	int data_fd;
@@ -282,6 +376,78 @@ TEST_F(user, write_validator) {
 	ASSERT_EQ(EFAULT, errno);
 }
 
+TEST_F(user, print_fmt) {
+	int ret;
+
+	ret = check_print_fmt("__test_event __rel_loc char[] data",
+			      "print fmt: \"data=%s\", __get_rel_str(data)");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event __data_loc char[] data",
+			      "print fmt: \"data=%s\", __get_str(data)");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event s64 data",
+			      "print fmt: \"data=%lld\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event u64 data",
+			      "print fmt: \"data=%llu\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event s32 data",
+			      "print fmt: \"data=%d\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event u32 data",
+			      "print fmt: \"data=%u\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event int data",
+			      "print fmt: \"data=%d\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event unsigned int data",
+			      "print fmt: \"data=%u\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event s16 data",
+			      "print fmt: \"data=%d\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event u16 data",
+			      "print fmt: \"data=%u\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event short data",
+			      "print fmt: \"data=%d\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event unsigned short data",
+			      "print fmt: \"data=%u\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event s8 data",
+			      "print fmt: \"data=%d\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event u8 data",
+			      "print fmt: \"data=%u\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event char data",
+			      "print fmt: \"data=%d\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event unsigned char data",
+			      "print fmt: \"data=%u\", REC->data");
+	ASSERT_EQ(0, ret);
+
+	ret = check_print_fmt("__test_event char[4] data",
+			      "print fmt: \"data=%s\", REC->data");
+	ASSERT_EQ(0, ret);
+}
+
 int main(int argc, char **argv)
 {
 	return test_harness_run(argc, argv);
-- 
2.17.1

Re: [PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields

[Masami Hiramatsu]

Hi Beau,

On Thu,  9 Dec 2021 14:32:10 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> Switch between __get_str and __get_rel_str within the print_fmt of
> user_events. Add unit test to ensure print_fmt is correct on known
> types.
> 
> Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> ---
>  kernel/trace/trace_events_user.c              |  24 ++-
>  .../selftests/user_events/ftrace_test.c       | 166 ++++++++++++++++++
>  2 files changed, 182 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 56eb58ddb4cf..3779fa2ca14a 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
>  	goto add_field;
>  
>  add_validator:
> -	if (strstr(type, "char[") != 0)
> +	if (strstr(type, "char") != 0)
>  		validator_flags |= VALIDATOR_ENSURE_NULL;

What is this change for? This seems not related to the other changes.
(Also, what happen if it is a single char type?)

>  
>  	validator = kmalloc(sizeof(*validator), GFP_KERNEL);
> @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type)
>  	return "%llu";
>  }
>  
> -static bool user_field_is_dyn_string(const char *type)
> +static bool user_field_is_dyn_string(const char *type, const char **str_func)
>  {
> -	if (str_has_prefix(type, "__data_loc ") ||
> -	    str_has_prefix(type, "__rel_loc "))
> -		if (strstr(type, "char[") != 0)
> -			return true;
> +	if (str_has_prefix(type, "__data_loc ")) {
> +		*str_func = "__get_str";
> +		goto check;
> +	}
> +
> +	if (str_has_prefix(type, "__rel_loc ")) {
> +		*str_func = "__get_rel_str";
> +		goto check;
> +	}
>  
>  	return false;
> +check:
> +	return strstr(type, "char") != 0;
>  }
>  
>  #define LEN_OR_ZERO (len ? len - pos : 0)
> @@ -472,6 +479,7 @@ 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;
> +	const char *str_func;
>  
>  	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
>  
> @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
>  	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))
> +		if (user_field_is_dyn_string(field->type, &str_func))
>  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> -					", __get_str(%s)", field->name);
> +					", %s(%s)", str_func, field->name);
>  		else
>  			pos += snprintf(buf + pos, LEN_OR_ZERO,
>  					", REC->%s", field->name);
> diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c

Just a nitpick, if possible, please split this part from the kernel update.

> index 16aff1fb295a..b2e5c0765a68 100644
> --- a/tools/testing/selftests/user_events/ftrace_test.c
> +++ b/tools/testing/selftests/user_events/ftrace_test.c
> @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
>  const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
>  const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable";
>  const char *trace_file = "/sys/kernel/debug/tracing/trace";
> +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format";
>  
>  static int trace_bytes(void)
>  {
> @@ -47,6 +48,61 @@ static int trace_bytes(void)
>  	return bytes;
>  }
>  
> +static int get_print_fmt(char *buffer, int len)
> +{
> +	FILE *fp = fopen(fmt_file, "r");
> +	int c, index = 0, last = 0;
> +
> +	if (!fp)
> +		return -1;
> +
> +	/* Read until empty line (Skip Common) */
> +	while (true) {
> +		c = getc(fp);
> +
> +		if (c == EOF)
> +			break;
> +
> +		if (last == '\n' && c == '\n')
> +			break;
> +
> +		last = c;
> +	}

Another nitpick, maybe you need a function like skip_until_empty_line(fp)
and repeat it like this.

	if (skip_until_empty_line(fp) < 0)
		goto out;
	if (skip_until_empty_line(fp) < 0)
		goto out;

> +
> +	last = 0;
> +
> +	/* Read until empty line (Skip Properties) */
> +	while (true) {
> +		c = getc(fp);
> +
> +		if (c == EOF)
> +			break;
> +
> +		if (last == '\n' && c == '\n')
> +			break;
> +
> +		last = c;
> +	}
> +
> +	/* Read in print_fmt: */
> +	while (len > 1) {
> +		c = getc(fp);
> +
> +		if (c == EOF || c == '\n')
> +			break;
> +
> +		buffer[index++] = c;
> +
> +		len--;
> +	}

And here you can use fgets(buffer, len, fp).


Thank you,

> +
> +	buffer[index] = 0;
> +
> +	fclose(fp);
> +
> +	return 0;
> +}
> +
>  static int clear(void)
>  {
>  	int fd = open(data_file, O_RDWR);
> @@ -63,6 +119,44 @@ static int clear(void)
>  	return 0;
>  }
>  
> +static int check_print_fmt(const char *event, const char *expected)
> +{
> +	struct user_reg reg = {0};
> +	char print_fmt[256];
> +	int ret;
> +	int fd;
> +
> +	/* Ensure cleared */
> +	ret = clear();
> +
> +	if (ret != 0)
> +		return ret;
> +
> +	fd = open(data_file, O_RDWR);
> +
> +	if (fd == -1)
> +		return fd;
> +
> +	reg.size = sizeof(reg);
> +	reg.name_args = (__u64)event;
> +
> +	/* Register should work */
> +	ret = ioctl(fd, DIAG_IOCSREG, &reg);
> +
> +	close(fd);
> +
> +	if (ret != 0)
> +		return ret;
> +
> +	/* Ensure correct print_fmt */
> +	ret = get_print_fmt(print_fmt, sizeof(print_fmt));
> +
> +	if (ret != 0)
> +		return ret;
> +
> +	return strcmp(print_fmt, expected);
> +}
> +
>  FIXTURE(user) {
>  	int status_fd;
>  	int data_fd;
> @@ -282,6 +376,78 @@ TEST_F(user, write_validator) {
>  	ASSERT_EQ(EFAULT, errno);
>  }
>  
> +TEST_F(user, print_fmt) {
> +	int ret;
> +
> +	ret = check_print_fmt("__test_event __rel_loc char[] data",
> +			      "print fmt: \"data=%s\", __get_rel_str(data)");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event __data_loc char[] data",
> +			      "print fmt: \"data=%s\", __get_str(data)");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event s64 data",
> +			      "print fmt: \"data=%lld\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event u64 data",
> +			      "print fmt: \"data=%llu\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event s32 data",
> +			      "print fmt: \"data=%d\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event u32 data",
> +			      "print fmt: \"data=%u\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event int data",
> +			      "print fmt: \"data=%d\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event unsigned int data",
> +			      "print fmt: \"data=%u\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event s16 data",
> +			      "print fmt: \"data=%d\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event u16 data",
> +			      "print fmt: \"data=%u\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event short data",
> +			      "print fmt: \"data=%d\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event unsigned short data",
> +			      "print fmt: \"data=%u\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event s8 data",
> +			      "print fmt: \"data=%d\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event u8 data",
> +			      "print fmt: \"data=%u\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event char data",
> +			      "print fmt: \"data=%d\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event unsigned char data",
> +			      "print fmt: \"data=%u\", REC->data");
> +	ASSERT_EQ(0, ret);
> +
> +	ret = check_print_fmt("__test_event char[4] data",
> +			      "print fmt: \"data=%s\", REC->data");
> +	ASSERT_EQ(0, ret);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	return test_harness_run(argc, argv);
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 03/13] user_events: Add print_fmt generation support for basic types

[Masami Hiramatsu]

On Thu,  9 Dec 2021 14:32:00 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> 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.
> 

This looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> ---
>  kernel/trace/trace_events_user.c | 107 ++++++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 1d96d1c85147..bd8ac46fddb1 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -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;
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[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 <beaub@linux.microsoft.com> wrote:

[..]
> +#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)
> +#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?

[..]
> +
> +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))
> +		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. :)

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

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])

> +
> +	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?
	
> + */
> +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);
> +}

[..]
> +
> +/*
> + * 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

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

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

[..]
> +/*
> + * 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;
> +
> +		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?

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

Thank you,



-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 01/13] user_events: Add UABI header for user access to user_events

[Masami Hiramatsu]

On Thu,  9 Dec 2021 14:31:58 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> 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.)

Thank you,

> Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> ---
>  include/uapi/linux/user_events.h | 68 ++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 include/uapi/linux/user_events.h
> 
> diff --git a/include/uapi/linux/user_events.h b/include/uapi/linux/user_events.h
> new file mode 100644
> index 000000000000..5bff99418deb
> --- /dev/null
> +++ b/include/uapi/linux/user_events.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2021, Microsoft Corporation.
> + *
> + * Authors:
> + *   Beau Belgrave <beaub@linux.microsoft.com>
> + */
> +#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 */
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 12/13] user_events: Validate user payloads for size and null termination

[Masami Hiramatsu]

On Thu,  9 Dec 2021 14:32:09 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> 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.
> 

This looks good to me :)

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> ---
>  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(-)
> 
> diff --git a/include/uapi/linux/user_events.h b/include/uapi/linux/user_events.h
> index 5bff99418deb..f97db05e00c9 100644
> --- a/include/uapi/linux/user_events.h
> +++ b/include/uapi/linux/user_events.h
> @@ -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)
>  
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 1d29f6ec907d..56eb58ddb4cf 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -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;
> diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
> index 9d53717139e6..16aff1fb295a 100644
> --- a/tools/testing/selftests/user_events/ftrace_test.c
> +++ b/tools/testing/selftests/user_events/ftrace_test.c
> @@ -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);
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 09/13] user_events: Optimize writing events by only copying data once

[Masami Hiramatsu]

Hi Beau,

On Thu,  9 Dec 2021 14:32:06 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> 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 <beaub@linux.microsoft.com>
> ---
>  kernel/trace/trace_events_user.c | 102 ++++++++++++++++++++++---------
>  1 file changed, 74 insertions(+), 28 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 807db0af74fb..1d29f6ec907d 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -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);
> -
> -		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;
>  
>  		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,
>  
>  		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;
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 01/13] user_events: Add UABI header for user access to user_events

[Beau Belgrave]

On Fri, Dec 10, 2021 at 10:30:17PM +0900, Masami Hiramatsu wrote:
> On Thu,  9 Dec 2021 14:31:58 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > 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.)
> 

Sure thing.

Thanks,
-Beau

Re: [PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[Steven Rostedt]

On Fri, 10 Dec 2021 19:43:58 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

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

In the future yes!

  https://lore.kernel.org/all/20211125175253.186422-1-tz.stoyanov@gmail.com/

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).

-- Steve

Re: [PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[Beau Belgrave]

On Fri, Dec 10, 2021 at 07:43:58PM +0900, Masami Hiramatsu wrote:
> Hi Beau,
> 
> Thanks for updating the patch! I have some comments below.
> 
> On Thu,  9 Dec 2021 14:31:59 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> [..]
> > +#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)
> > +#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?
> 

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

> [..]
> > +
> > +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))
> > +		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 ;)

> 
> 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.

[..]

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

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

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.

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

> > +	}
> > +
> > +	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;
> > +}
> > +
> 
> [..]
> > +/*
> > + * 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;
> > +
> > +		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.

> > +
> > +		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;
> > +}
> 
> Thank you,
> 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 09/13] user_events: Optimize writing events by only copying data once

[Beau Belgrave]

On Fri, Dec 10, 2021 at 11:51:10PM +0900, Masami Hiramatsu wrote:
> Hi Beau,
> 
> On Thu,  9 Dec 2021 14:32:06 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > 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 <beaub@linux.microsoft.com>
> > ---

[..]

> > @@ -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;
> >  
> >  		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()?
> 

Sure thing.

> Thank you,
> >  
> >  		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;
> > -- 
> > 2.17.1
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Thanks,
-Beau

Re: [PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields

[Beau Belgrave]

On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote:
> Hi Beau,
> 
> On Thu,  9 Dec 2021 14:32:10 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > Switch between __get_str and __get_rel_str within the print_fmt of
> > user_events. Add unit test to ensure print_fmt is correct on known
> > types.
> > 
> > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> > ---
> >  kernel/trace/trace_events_user.c              |  24 ++-
> >  .../selftests/user_events/ftrace_test.c       | 166 ++++++++++++++++++
> >  2 files changed, 182 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > index 56eb58ddb4cf..3779fa2ca14a 100644
> > --- a/kernel/trace/trace_events_user.c
> > +++ b/kernel/trace/trace_events_user.c
> > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
> >  	goto add_field;
> >  
> >  add_validator:
> > -	if (strstr(type, "char[") != 0)
> > +	if (strstr(type, "char") != 0)
> >  		validator_flags |= VALIDATOR_ENSURE_NULL;
> 
> What is this change for? This seems not related to the other changes.
> (Also, what happen if it is a single char type?)
> 

I'm glad you asked, it appears like __data_loc / __rel_loc can take char
as it's type (It doesn't appear to be limited to char[] cases). I wanted
to ensure something malicious couldn't sneak past by using this corner
case.

IE: __data_loc char test

In trace_events_filter.c:
int filter_assign_type(const char *type)
{
        if (strstr(type, "__data_loc") && strstr(type, "char"))
                return FILTER_DYN_STRING;

        if (strchr(type, '[') && strstr(type, "char"))
                return FILTER_STATIC_STRING;

        if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0)
                return FILTER_PTR_STRING;

        return FILTER_OTHER;
}

char[ is only checked if __data_loc is not specified.

> >  
> >  	validator = kmalloc(sizeof(*validator), GFP_KERNEL);
> > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type)
> >  	return "%llu";
> >  }
> >  
> > -static bool user_field_is_dyn_string(const char *type)
> > +static bool user_field_is_dyn_string(const char *type, const char **str_func)
> >  {
> > -	if (str_has_prefix(type, "__data_loc ") ||
> > -	    str_has_prefix(type, "__rel_loc "))
> > -		if (strstr(type, "char[") != 0)
> > -			return true;
> > +	if (str_has_prefix(type, "__data_loc ")) {
> > +		*str_func = "__get_str";
> > +		goto check;
> > +	}
> > +
> > +	if (str_has_prefix(type, "__rel_loc ")) {
> > +		*str_func = "__get_rel_str";
> > +		goto check;
> > +	}
> >  
> >  	return false;
> > +check:
> > +	return strstr(type, "char") != 0;
> >  }
> >  
> >  #define LEN_OR_ZERO (len ? len - pos : 0)
> > @@ -472,6 +479,7 @@ 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;
> > +	const char *str_func;
> >  
> >  	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
> >  
> > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
> >  	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))
> > +		if (user_field_is_dyn_string(field->type, &str_func))
> >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> > -					", __get_str(%s)", field->name);
> > +					", %s(%s)", str_func, field->name);
> >  		else
> >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> >  					", REC->%s", field->name);
> > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
> 
> Just a nitpick, if possible, please split this part from the kernel update.
> 

I will try to do so, could you help me understand why I would split this
out? (For future patches)

I thought the intention of each would be to contain it's logical grouping:
I wanted to show, yes the code changed, and yes we have a unit test for
that new condition.

> > index 16aff1fb295a..b2e5c0765a68 100644
> > --- a/tools/testing/selftests/user_events/ftrace_test.c
> > +++ b/tools/testing/selftests/user_events/ftrace_test.c
> > @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
> >  const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
> >  const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable";
> >  const char *trace_file = "/sys/kernel/debug/tracing/trace";
> > +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format";
> >  
> >  static int trace_bytes(void)
> >  {
> > @@ -47,6 +48,61 @@ static int trace_bytes(void)
> >  	return bytes;
> >  }
> >  
> > +static int get_print_fmt(char *buffer, int len)
> > +{
> > +	FILE *fp = fopen(fmt_file, "r");
> > +	int c, index = 0, last = 0;
> > +
> > +	if (!fp)
> > +		return -1;
> > +
> > +	/* Read until empty line (Skip Common) */
> > +	while (true) {
> > +		c = getc(fp);
> > +
> > +		if (c == EOF)
> > +			break;
> > +
> > +		if (last == '\n' && c == '\n')
> > +			break;
> > +
> > +		last = c;
> > +	}
> 
> Another nitpick, maybe you need a function like skip_until_empty_line(fp)
> and repeat it like this.
> 
> 	if (skip_until_empty_line(fp) < 0)
> 		goto out;
> 	if (skip_until_empty_line(fp) < 0)
> 		goto out;
> 

Sure thing.

> > +
> > +	last = 0;
> > +
> > +	/* Read until empty line (Skip Properties) */
> > +	while (true) {
> > +		c = getc(fp);
> > +
> > +		if (c == EOF)
> > +			break;
> > +
> > +		if (last == '\n' && c == '\n')
> > +			break;
> > +
> > +		last = c;
> > +	}
> > +
> > +	/* Read in print_fmt: */
> > +	while (len > 1) {
> > +		c = getc(fp);
> > +
> > +		if (c == EOF || c == '\n')
> > +			break;
> > +
> > +		buffer[index++] = c;
> > +
> > +		len--;
> > +	}
> 
> And here you can use fgets(buffer, len, fp).
> 

Makes sense.

> 
> Thank you,
> 
> 

[..]

> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Thanks,
-Beau

Re: [PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields

[Masami Hiramatsu]

On Fri, 10 Dec 2021 10:45:51 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote:
> > Hi Beau,
> > 
> > On Thu,  9 Dec 2021 14:32:10 -0800
> > Beau Belgrave <beaub@linux.microsoft.com> wrote:
> > 
> > > Switch between __get_str and __get_rel_str within the print_fmt of
> > > user_events. Add unit test to ensure print_fmt is correct on known
> > > types.
> > > 
> > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> > > ---
> > >  kernel/trace/trace_events_user.c              |  24 ++-
> > >  .../selftests/user_events/ftrace_test.c       | 166 ++++++++++++++++++
> > >  2 files changed, 182 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > > index 56eb58ddb4cf..3779fa2ca14a 100644
> > > --- a/kernel/trace/trace_events_user.c
> > > +++ b/kernel/trace/trace_events_user.c
> > > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
> > >  	goto add_field;
> > >  
> > >  add_validator:
> > > -	if (strstr(type, "char[") != 0)
> > > +	if (strstr(type, "char") != 0)
> > >  		validator_flags |= VALIDATOR_ENSURE_NULL;
> > 
> > What is this change for? This seems not related to the other changes.
> > (Also, what happen if it is a single char type?)
> > 
> 
> I'm glad you asked, it appears like __data_loc / __rel_loc can take char
> as it's type (It doesn't appear to be limited to char[] cases). I wanted
> to ensure something malicious couldn't sneak past by using this corner
> case.
> 
> IE: __data_loc char test
> 
> In trace_events_filter.c:
> int filter_assign_type(const char *type)
> {
>         if (strstr(type, "__data_loc") && strstr(type, "char"))
>                 return FILTER_DYN_STRING;
> 
>         if (strchr(type, '[') && strstr(type, "char"))
>                 return FILTER_STATIC_STRING;
> 
>         if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0)
>                 return FILTER_PTR_STRING;
> 
>         return FILTER_OTHER;
> }
> 
> char[ is only checked if __data_loc is not specified.

OK, but in that case, is this patch good place for that change?

> 
> > >  
> > >  	validator = kmalloc(sizeof(*validator), GFP_KERNEL);
> > > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type)
> > >  	return "%llu";
> > >  }
> > >  
> > > -static bool user_field_is_dyn_string(const char *type)
> > > +static bool user_field_is_dyn_string(const char *type, const char **str_func)
> > >  {
> > > -	if (str_has_prefix(type, "__data_loc ") ||
> > > -	    str_has_prefix(type, "__rel_loc "))
> > > -		if (strstr(type, "char[") != 0)
> > > -			return true;
> > > +	if (str_has_prefix(type, "__data_loc ")) {
> > > +		*str_func = "__get_str";
> > > +		goto check;
> > > +	}
> > > +
> > > +	if (str_has_prefix(type, "__rel_loc ")) {
> > > +		*str_func = "__get_rel_str";
> > > +		goto check;
> > > +	}
> > >  
> > >  	return false;
> > > +check:
> > > +	return strstr(type, "char") != 0;
> > >  }
> > >  
> > >  #define LEN_OR_ZERO (len ? len - pos : 0)
> > > @@ -472,6 +479,7 @@ 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;
> > > +	const char *str_func;
> > >  
> > >  	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
> > >  
> > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
> > >  	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))
> > > +		if (user_field_is_dyn_string(field->type, &str_func))
> > >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> > > -					", __get_str(%s)", field->name);
> > > +					", %s(%s)", str_func, field->name);
> > >  		else
> > >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> > >  					", REC->%s", field->name);
> > > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
> > 
> > Just a nitpick, if possible, please split this part from the kernel update.
> > 
> 
> I will try to do so, could you help me understand why I would split this
> out? (For future patches)
> 
> I thought the intention of each would be to contain it's logical grouping:
> I wanted to show, yes the code changed, and yes we have a unit test for
> that new condition.

Hrm, in this specific case, maybe this can be acceptable. Following
case you might need to take care of it.

- if the feature and the test code are maintained by different maintainer.
- if the test code is added much later than the feature.

In both case, the piece of patches will be applied separately. The former
case, by different maintainer, the latter case by different tree (e.g. 
stable tree may not have the test case.)

BTW, I also think this change is a fix for the previous patches in the series.
In that case, please update those patches so that the patch is completely works.
That will be good for bisecting.

Thank you,

> 
> > > index 16aff1fb295a..b2e5c0765a68 100644
> > > --- a/tools/testing/selftests/user_events/ftrace_test.c
> > > +++ b/tools/testing/selftests/user_events/ftrace_test.c
> > > @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data";
> > >  const char *status_file = "/sys/kernel/debug/tracing/user_events_status";
> > >  const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable";
> > >  const char *trace_file = "/sys/kernel/debug/tracing/trace";
> > > +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format";
> > >  
> > >  static int trace_bytes(void)
> > >  {
> > > @@ -47,6 +48,61 @@ static int trace_bytes(void)
> > >  	return bytes;
> > >  }
> > >  
> > > +static int get_print_fmt(char *buffer, int len)
> > > +{
> > > +	FILE *fp = fopen(fmt_file, "r");
> > > +	int c, index = 0, last = 0;
> > > +
> > > +	if (!fp)
> > > +		return -1;
> > > +
> > > +	/* Read until empty line (Skip Common) */
> > > +	while (true) {
> > > +		c = getc(fp);
> > > +
> > > +		if (c == EOF)
> > > +			break;
> > > +
> > > +		if (last == '\n' && c == '\n')
> > > +			break;
> > > +
> > > +		last = c;
> > > +	}
> > 
> > Another nitpick, maybe you need a function like skip_until_empty_line(fp)
> > and repeat it like this.
> > 
> > 	if (skip_until_empty_line(fp) < 0)
> > 		goto out;
> > 	if (skip_until_empty_line(fp) < 0)
> > 		goto out;
> > 
> 
> Sure thing.
> 
> > > +
> > > +	last = 0;
> > > +
> > > +	/* Read until empty line (Skip Properties) */
> > > +	while (true) {
> > > +		c = getc(fp);
> > > +
> > > +		if (c == EOF)
> > > +			break;
> > > +
> > > +		if (last == '\n' && c == '\n')
> > > +			break;
> > > +
> > > +		last = c;
> > > +	}
> > > +
> > > +	/* Read in print_fmt: */
> > > +	while (len > 1) {
> > > +		c = getc(fp);
> > > +
> > > +		if (c == EOF || c == '\n')
> > > +			break;
> > > +
> > > +		buffer[index++] = c;
> > > +
> > > +		len--;
> > > +	}
> > 
> > And here you can use fgets(buffer, len, fp).
> > 
> 
> Makes sense.
> 
> > 
> > Thank you,
> > 
> > 
> 
> [..]
> 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>
> 
> Thanks,
> -Beau


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[Masami Hiramatsu]

On Fri, 10 Dec 2021 12:43:05 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 10 Dec 2021 19:43:58 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > +/* 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?
> 
> In the future yes!
> 
>   https://lore.kernel.org/all/20211125175253.186422-1-tz.stoyanov@gmail.com/

Ah, nice!

> 
> 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?

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[Masami Hiramatsu]

Hi Beau,

On Fri, 10 Dec 2021 10:03:54 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> On Fri, Dec 10, 2021 at 07:43:58PM +0900, Masami Hiramatsu wrote:
> > Hi Beau,
> > 
> > Thanks for updating the patch! I have some comments below.
> > 
> > On Thu,  9 Dec 2021 14:31:59 -0800
> > Beau Belgrave <beaub@linux.microsoft.com> wrote:
> > 
> > [..]
> > > +#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)
> > > +#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?
> > 
> 
> 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))
-----

> 
> > [..]
> > > +
> > > +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))
> > > +		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 ;)


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

> > 
> > 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.
> 
> [..]
> 
> > > +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);
> > 
> > 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?

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(...)"

> 
> > > +
> > > +	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?
> > 	
> 
> 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 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.

> 
> 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. :( )

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

Ah, then that's OK.

> 
> > > +	}
> > > +
> > > +	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;
> > > +}
> > > +
> > 
> > [..]
> > > +/*
> > > + * 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;
> > > +
> > > +		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.

OK, got it.

Thank you,

> 
> > > +
> > > +		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;
> > > +}
> > 
> > Thank you,
> > 
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[Steven Rostedt]

On Mon, 13 Dec 2021 09:09:15 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > Steve, can we write such a big event data on the trace buffer?  
> > 
> > In the future yes!
> > 
> >   https://lore.kernel.org/all/20211125175253.186422-1-tz.stoyanov@gmail.com/  
> 
> Ah, nice!
> 
> > 
> > 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?

I'm fine with that.

-- Steve

Re: [PATCH v7 02/13] user_events: Add minimal support for trace_event into ftrace

[Beau Belgrave]

On Mon, Dec 13, 2021 at 01:24:39PM +0900, Masami Hiramatsu wrote:
> Hi Beau,
> 
> On Fri, 10 Dec 2021 10:03:54 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > On Fri, Dec 10, 2021 at 07:43:58PM +0900, Masami Hiramatsu wrote:
> > > Hi Beau,
> > > 
> > > Thanks for updating the patch! I have some comments below.
> > > 
> > > On Thu,  9 Dec 2021 14:31:59 -0800
> > > Beau Belgrave <beaub@linux.microsoft.com> wrote:
> > > 
> > > [..]
> > > > +#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)
> > > > +#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?
> > > 
> > 
> > This moved to 1024 in part 12 when validation was added.
> 
> OK, then it should be done in this patch.
> 

Sure thing.

> 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))
> -----
> 
> > 
> > > [..]
> > > > +
> > > > +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))
> > > > +		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 ;)
> 
> 
> Oops, OK. That is the style thing for clarify what you are doing.
> (not initializing the element, but terminating the string)
> 

Ok, I've moved it to '\0' to show intention.

> > > 
> > > 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.
> > 
> > [..]
> > 
> > > > +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);
> > > 
> > > 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?
> 
> 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(...)"
> 
> > 
> > > > +
> > > > +	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?
> > > 	
> > 
> > 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 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.
> 

I'd prefer to get this set of patches in somewhere before reworking
large amounts of it if possible.

It seems like a good idea to have a general way of getting a dyn_event
string parsed out into trace_event fields. That way all parsing components
have the same behaviors, etc.

The proposed approach simplifies some things and makes other things more
complex. For example, if a type is '__data_loc unsigned char[]' there are
now 3 pointers: '__data_loc', 'unsigned' and 'char[]'.

This makes it so I can no longer just point the trace_event's field to
the string, I now need in those cases to join them back up. And because
they aren't from the original pointer, I need to now track which
pointers need to be freed when freeing each field (unless all are always
copied, which feels wasteful).

I'm not worried so much about allocation time, since this is not a hot
path (but it is under a global lock). I took the fact that trace_event
allocates fields internally out of a cache to mean that someone at
sometime did care about either allocation time or fragmentation.

As the patch stands now, there is only a single allocation, all types,
names, etc on the fields are close together in memory for each event.
While this is not required, it is a nice attribute of this approach vs
alloc everything into it's own block (that may or may not be close).

> > 
> > 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. :( )
> 
>  

I get that, I think it would be great if there was a general way to
parse things. That way systems that utilize dyn_event have consistent
behavior across them. Several comments in this series have been around
this area, to the point that it would save the next person a lot of time
if a common mechanism existed for parsing and type sizes, etc.

> > > > +
> > > > +/*
> > > > + * 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
> > > 
> > > 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.
> 
> Ah, then that's OK.
> 
> > 
> > > > +	}
> > > > +
> > > > +	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;
> > > > +}
> > > > +
> > > 
> > > [..]
> > > > +/*
> > > > + * 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;
> > > > +
> > > > +		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.
> 
> OK, got it.
> 
> Thank you,
> 
> > 
> > > > +
> > > > +		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;
> > > > +}
> > > 
> > > Thank you,
> > > 
> > > 
> > > 
> > > -- 
> > > Masami Hiramatsu <mhiramat@kernel.org>
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>

Thanks,
-Beau

Re: [PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields

[Beau Belgrave]

On Mon, Dec 13, 2021 at 12:12:15AM +0900, Masami Hiramatsu wrote:
> On Fri, 10 Dec 2021 10:45:51 -0800
> Beau Belgrave <beaub@linux.microsoft.com> wrote:
> 
> > On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote:
> > > Hi Beau,
> > > 
> > > On Thu,  9 Dec 2021 14:32:10 -0800
> > > Beau Belgrave <beaub@linux.microsoft.com> wrote:
> > > 
> > > > Switch between __get_str and __get_rel_str within the print_fmt of
> > > > user_events. Add unit test to ensure print_fmt is correct on known
> > > > types.
> > > > 
> > > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> > > > ---
> > > >  kernel/trace/trace_events_user.c              |  24 ++-
> > > >  .../selftests/user_events/ftrace_test.c       | 166 ++++++++++++++++++
> > > >  2 files changed, 182 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > > > index 56eb58ddb4cf..3779fa2ca14a 100644
> > > > --- a/kernel/trace/trace_events_user.c
> > > > +++ b/kernel/trace/trace_events_user.c
> > > > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
> > > >  	goto add_field;
> > > >  
> > > >  add_validator:
> > > > -	if (strstr(type, "char[") != 0)
> > > > +	if (strstr(type, "char") != 0)
> > > >  		validator_flags |= VALIDATOR_ENSURE_NULL;
> > > 
> > > What is this change for? This seems not related to the other changes.
> > > (Also, what happen if it is a single char type?)
> > > 
> > 
> > I'm glad you asked, it appears like __data_loc / __rel_loc can take char
> > as it's type (It doesn't appear to be limited to char[] cases). I wanted
> > to ensure something malicious couldn't sneak past by using this corner
> > case.
> > 
> > IE: __data_loc char test
> > 
> > In trace_events_filter.c:
> > int filter_assign_type(const char *type)
> > {
> >         if (strstr(type, "__data_loc") && strstr(type, "char"))
> >                 return FILTER_DYN_STRING;
> > 
> >         if (strchr(type, '[') && strstr(type, "char"))
> >                 return FILTER_STATIC_STRING;
> > 
> >         if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0)
> >                 return FILTER_PTR_STRING;
> > 
> >         return FILTER_OTHER;
> > }
> > 
> > char[ is only checked if __data_loc is not specified.
> 
> OK, but in that case, is this patch good place for that change?
> 

I'll move this to part 12.

> > 
> > > >  
> > > >  	validator = kmalloc(sizeof(*validator), GFP_KERNEL);
> > > > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type)
> > > >  	return "%llu";
> > > >  }
> > > >  
> > > > -static bool user_field_is_dyn_string(const char *type)
> > > > +static bool user_field_is_dyn_string(const char *type, const char **str_func)
> > > >  {
> > > > -	if (str_has_prefix(type, "__data_loc ") ||
> > > > -	    str_has_prefix(type, "__rel_loc "))
> > > > -		if (strstr(type, "char[") != 0)
> > > > -			return true;
> > > > +	if (str_has_prefix(type, "__data_loc ")) {
> > > > +		*str_func = "__get_str";
> > > > +		goto check;
> > > > +	}
> > > > +
> > > > +	if (str_has_prefix(type, "__rel_loc ")) {
> > > > +		*str_func = "__get_rel_str";
> > > > +		goto check;
> > > > +	}
> > > >  
> > > >  	return false;
> > > > +check:
> > > > +	return strstr(type, "char") != 0;
> > > >  }
> > > >  
> > > >  #define LEN_OR_ZERO (len ? len - pos : 0)
> > > > @@ -472,6 +479,7 @@ 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;
> > > > +	const char *str_func;
> > > >  
> > > >  	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
> > > >  
> > > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
> > > >  	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))
> > > > +		if (user_field_is_dyn_string(field->type, &str_func))
> > > >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> > > > -					", __get_str(%s)", field->name);
> > > > +					", %s(%s)", str_func, field->name);
> > > >  		else
> > > >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> > > >  					", REC->%s", field->name);
> > > > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
> > > 
> > > Just a nitpick, if possible, please split this part from the kernel update.
> > > 
> > 
> > I will try to do so, could you help me understand why I would split this
> > out? (For future patches)
> > 
> > I thought the intention of each would be to contain it's logical grouping:
> > I wanted to show, yes the code changed, and yes we have a unit test for
> > that new condition.
> 
> Hrm, in this specific case, maybe this can be acceptable. Following
> case you might need to take care of it.
> 
> - if the feature and the test code are maintained by different maintainer.
> - if the test code is added much later than the feature.
> 
> In both case, the piece of patches will be applied separately. The former
> case, by different maintainer, the latter case by different tree (e.g. 
> stable tree may not have the test case.)
> 
> BTW, I also think this change is a fix for the previous patches in the series.
> In that case, please update those patches so that the patch is completely works.
> That will be good for bisecting.
> 

Do you mean you want the rest of this change rolled into 04/13 (print_fmt
generation)?

And have the char vs char[ rolled into 12/13 (add validators)?

I can then roll the unit test for this case under 05/13 (ftrace
selftest).

Thanks,
-Beau

Re: [PATCH v7 13/13] user_events: Use __get_rel_str for relative string fields

[Masami Hiramatsu]

On Mon, 13 Dec 2021 10:47:23 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> On Mon, Dec 13, 2021 at 12:12:15AM +0900, Masami Hiramatsu wrote:
> > On Fri, 10 Dec 2021 10:45:51 -0800
> > Beau Belgrave <beaub@linux.microsoft.com> wrote:
> > 
> > > On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote:
> > > > Hi Beau,
> > > > 
> > > > On Thu,  9 Dec 2021 14:32:10 -0800
> > > > Beau Belgrave <beaub@linux.microsoft.com> wrote:
> > > > 
> > > > > Switch between __get_str and __get_rel_str within the print_fmt of
> > > > > user_events. Add unit test to ensure print_fmt is correct on known
> > > > > types.
> > > > > 
> > > > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com>
> > > > > ---
> > > > >  kernel/trace/trace_events_user.c              |  24 ++-
> > > > >  .../selftests/user_events/ftrace_test.c       | 166 ++++++++++++++++++
> > > > >  2 files changed, 182 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > > > > index 56eb58ddb4cf..3779fa2ca14a 100644
> > > > > --- a/kernel/trace/trace_events_user.c
> > > > > +++ b/kernel/trace/trace_events_user.c
> > > > > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
> > > > >  	goto add_field;
> > > > >  
> > > > >  add_validator:
> > > > > -	if (strstr(type, "char[") != 0)
> > > > > +	if (strstr(type, "char") != 0)
> > > > >  		validator_flags |= VALIDATOR_ENSURE_NULL;
> > > > 
> > > > What is this change for? This seems not related to the other changes.
> > > > (Also, what happen if it is a single char type?)
> > > > 
> > > 
> > > I'm glad you asked, it appears like __data_loc / __rel_loc can take char
> > > as it's type (It doesn't appear to be limited to char[] cases). I wanted
> > > to ensure something malicious couldn't sneak past by using this corner
> > > case.
> > > 
> > > IE: __data_loc char test
> > > 
> > > In trace_events_filter.c:
> > > int filter_assign_type(const char *type)
> > > {
> > >         if (strstr(type, "__data_loc") && strstr(type, "char"))
> > >                 return FILTER_DYN_STRING;
> > > 
> > >         if (strchr(type, '[') && strstr(type, "char"))
> > >                 return FILTER_STATIC_STRING;
> > > 
> > >         if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0)
> > >                 return FILTER_PTR_STRING;
> > > 
> > >         return FILTER_OTHER;
> > > }
> > > 
> > > char[ is only checked if __data_loc is not specified.
> > 
> > OK, but in that case, is this patch good place for that change?
> > 
> 
> I'll move this to part 12.
> 
> > > 
> > > > >  
> > > > >  	validator = kmalloc(sizeof(*validator), GFP_KERNEL);
> > > > > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type)
> > > > >  	return "%llu";
> > > > >  }
> > > > >  
> > > > > -static bool user_field_is_dyn_string(const char *type)
> > > > > +static bool user_field_is_dyn_string(const char *type, const char **str_func)
> > > > >  {
> > > > > -	if (str_has_prefix(type, "__data_loc ") ||
> > > > > -	    str_has_prefix(type, "__rel_loc "))
> > > > > -		if (strstr(type, "char[") != 0)
> > > > > -			return true;
> > > > > +	if (str_has_prefix(type, "__data_loc ")) {
> > > > > +		*str_func = "__get_str";
> > > > > +		goto check;
> > > > > +	}
> > > > > +
> > > > > +	if (str_has_prefix(type, "__rel_loc ")) {
> > > > > +		*str_func = "__get_rel_str";
> > > > > +		goto check;
> > > > > +	}
> > > > >  
> > > > >  	return false;
> > > > > +check:
> > > > > +	return strstr(type, "char") != 0;
> > > > >  }
> > > > >  
> > > > >  #define LEN_OR_ZERO (len ? len - pos : 0)
> > > > > @@ -472,6 +479,7 @@ 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;
> > > > > +	const char *str_func;
> > > > >  
> > > > >  	pos += snprintf(buf + pos, LEN_OR_ZERO, "\"");
> > > > >  
> > > > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len)
> > > > >  	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))
> > > > > +		if (user_field_is_dyn_string(field->type, &str_func))
> > > > >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> > > > > -					", __get_str(%s)", field->name);
> > > > > +					", %s(%s)", str_func, field->name);
> > > > >  		else
> > > > >  			pos += snprintf(buf + pos, LEN_OR_ZERO,
> > > > >  					", REC->%s", field->name);
> > > > > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c
> > > > 
> > > > Just a nitpick, if possible, please split this part from the kernel update.
> > > > 
> > > 
> > > I will try to do so, could you help me understand why I would split this
> > > out? (For future patches)
> > > 
> > > I thought the intention of each would be to contain it's logical grouping:
> > > I wanted to show, yes the code changed, and yes we have a unit test for
> > > that new condition.
> > 
> > Hrm, in this specific case, maybe this can be acceptable. Following
> > case you might need to take care of it.
> > 
> > - if the feature and the test code are maintained by different maintainer.
> > - if the test code is added much later than the feature.
> > 
> > In both case, the piece of patches will be applied separately. The former
> > case, by different maintainer, the latter case by different tree (e.g. 
> > stable tree may not have the test case.)
> > 
> > BTW, I also think this change is a fix for the previous patches in the series.
> > In that case, please update those patches so that the patch is completely works.
> > That will be good for bisecting.
> > 
> 
> Do you mean you want the rest of this change rolled into 04/13 (print_fmt
> generation)?
> 
> And have the char vs char[ rolled into 12/13 (add validators)?

Sure, both are yes :)

> 
> I can then roll the unit test for this case under 05/13 (ftrace
> selftest).

That's also good to me :)

Thank you!

> 
> Thanks,
> -Beau
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>