[PATCH bpf-next 0/2] selftests/bpf: port test_ftrace.sh into test_progs

[PATCH bpf-next 0/2] selftests/bpf: port test_ftrace.sh into test_progs

[Alexis Lothoré (eBPF Foundation)]

Hello,
this small series aims to port the test_ftrace.sh script (testing that
trampolines and function graph behave correctly when both are enabled)
into the test_progs framework

- commit 1 brings a few new helpers to manipulate ftrace from test_progs
- commit 2 performs the actual porting

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
Alexis Lothoré (eBPF Foundation) (2):
      selftests/bpf: add helpers to manipulate ftrace
      selftests/bpf: port test_ftrace.sh to test_progs

 .../selftests/bpf/prog_tests/fgraph_trampoline.c   | 129 +++++++++++
 .../selftests/bpf/progs/fgraph_trampoline.c        |  29 +++
 tools/testing/selftests/bpf/test_ftrace.sh         |  44 ----
 tools/testing/selftests/bpf/trace_helpers.c        | 247 ++++++++++++++++++++-
 tools/testing/selftests/bpf/trace_helpers.h        |   9 +
 5 files changed, 412 insertions(+), 46 deletions(-)
---
base-commit: 960cc441448b063bbc1f283d7e801af6d872e45c
change-id: 20261027-ftrace_in_test_progs-d5f70a2330ac

Best regards,
--  
Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

[PATCH bpf-next 1/2] selftests/bpf: add helpers to manipulate ftrace

[Alexis Lothoré (eBPF Foundation)]

Add helpers to allow a test_progs test to tune ftrace configuration.
This new set of helpers work with an opaque structure: this one is used
to record any change performed on ftrace, so that we can restore those
automatically once the test is done.

The new helpers distinguish between two entries types:
- basic controls, that are "knobs"
- filters, for which writes are "additive"

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
 tools/testing/selftests/bpf/trace_helpers.c | 247 +++++++++++++++++++++++++++-
 tools/testing/selftests/bpf/trace_helpers.h |   9 +
 2 files changed, 254 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 6d7a4b8c4ab8..1f26a057a810 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c
@@ -21,8 +21,12 @@
 #include "bpf/libbpf_internal.h"
 #include "bpf_util.h"
 
-#define TRACEFS_PIPE	"/sys/kernel/tracing/trace_pipe"
-#define DEBUGFS_PIPE	"/sys/kernel/debug/tracing/trace_pipe"
+#define TRACEFS		"/sys/kernel/tracing"
+#define TRACEFS_PIPE	TRACEFS "/trace_pipe"
+#define DEBUGFS		"/sys/kernel/debug/tracing"
+#define DEBUGFS_PIPE	DEBUGFS "/trace_pipe"
+
+#define CHUNK_SIZE	128
 
 static struct ksyms *ksyms;
 static pthread_mutex_t ksyms_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -557,6 +561,245 @@ void read_trace_pipe(void)
 	read_trace_pipe_iter(trace_pipe_cb, NULL, 0);
 }
 
+static const char *tracefs_mountpoint(void)
+{
+	return access(TRACEFS_PIPE, F_OK) == 0 ? TRACEFS : DEBUGFS;
+}
+
+static int tracefs_path(char *buf, size_t size, const char *file)
+{
+	int ret = snprintf(buf, size, "%s/%s", tracefs_mountpoint(), file);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int tracefs_read(int fd, char **content)
+{
+	size_t len = 0;
+	char tmp[CHUNK_SIZE];
+	char *buf = NULL;
+	ssize_t n;
+	int ret;
+
+	ret = lseek(fd, 0, SEEK_SET);
+	if (ret < 0)
+		return ret;
+
+	do {
+		n = read(fd, tmp, CHUNK_SIZE);
+		if (n < 0) {
+			if (buf)
+				free(buf);
+			return n;
+		}
+		if (!n)
+			break;
+		buf = realloc(buf, len + n + 1);
+		memcpy(buf + len, tmp, n);
+		len += n;
+	} while (1);
+
+	/* No data read at all, file was empty */
+	if (!buf) {
+		buf = malloc(1);
+		if (!buf)
+			return -ENOMEM;
+	}
+	buf[len] = '\0';
+	*content = buf;
+	return 0;
+}
+
+static int tracefs_write(int fd, const char *str)
+{
+	ssize_t ret;
+
+	ret = lseek(fd, 0, SEEK_SET);
+	if (ret < 0)
+		return ret;
+
+	ret = write(fd, str, strlen(str));
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int tracefs_write_file(const char *path, const char *str)
+{
+	int fd, err;
+
+	fd = open(path, O_CLOEXEC | O_WRONLY | (*str ? 0 : O_TRUNC));
+	if (fd < 0)
+		return fd;
+
+	err = tracefs_write(fd, str);
+	close(fd);
+	return err;
+}
+
+enum tracefs_ctl_kind {
+	TRACEFS_CTL_FILTER,
+	TRACEFS_CTL_VALUE,
+};
+
+struct tracefs_ctl_entry {
+	char path[PATH_MAX];
+	char *old;
+	enum tracefs_ctl_kind kind;
+};
+
+struct tracefs_ctl {
+	struct tracefs_ctl_entry *entries;
+	size_t cnt;
+};
+
+struct tracefs_ctl *tracefs_ctl_init(void)
+{
+	return calloc(1, sizeof(struct tracefs_ctl));
+}
+
+static int tracefs_ctl_update(struct tracefs_ctl *ctl, const char *file,
+			      const char *str, enum tracefs_ctl_kind kind)
+{
+	struct tracefs_ctl_entry *entry, *tmp;
+	char path[PATH_MAX];
+	char *old = NULL;
+	bool recorded = false;
+	size_t i;
+	int fd, err;
+
+	if (!ctl)
+		return -EINVAL;
+
+	err = tracefs_path(path, sizeof(path), file);
+	if (err)
+		return err;
+
+	for (i = 0; i < ctl->cnt; i++) {
+		if (strcmp(ctl->entries[i].path, path))
+			continue;
+		if (ctl->entries[i].kind != kind)
+			return -EINVAL;
+		/* Already altered, keep the value read on the first write. */
+		return tracefs_write_file(ctl->entries[i].path, str);
+	}
+
+	fd = open(path, O_CLOEXEC | O_RDWR);
+	if (fd < 0)
+		return fd;
+
+	err = tracefs_read(fd, &old);
+	if (!err) {
+		tmp = realloc(ctl->entries, (ctl->cnt + 1) * sizeof(*tmp));
+		if (!tmp) {
+			free(old);
+			close(fd);
+			return -ENOMEM;
+		}
+		ctl->entries = tmp;
+		entry = &ctl->entries[ctl->cnt];
+		strcpy(entry->path, path);
+		entry->old = old;
+		entry->kind = kind;
+		recorded = true;
+	}
+
+	err = tracefs_write(fd, str);
+	close(fd);
+	if (err) {
+		free(old);
+		return err;
+	}
+
+	if (recorded)
+		ctl->cnt++;
+	return 0;
+}
+
+int tracefs_ctl_filter_write(struct tracefs_ctl *ctl, const char *file,
+			     const char *pattern)
+{
+	return tracefs_ctl_update(ctl, file, pattern, TRACEFS_CTL_FILTER);
+}
+
+int tracefs_ctl_set(struct tracefs_ctl *ctl, const char *file, const char *value)
+{
+	return tracefs_ctl_update(ctl, file, value, TRACEFS_CTL_VALUE);
+}
+
+static const char *tracefs_next_line(const char *content, char *buf, size_t size,
+				     int *err)
+{
+	const char *end;
+	size_t len;
+
+	*err = 0;
+	while (*content) {
+		end = strchrnul(content, '\n');
+		len = end - content;
+		content = *end ? end + 1 : end;
+
+		if (!len || end[-len] == '#')
+			continue;
+
+		if (len + 2 > size) {
+			*err = -ENAMETOOLONG;
+			return NULL;
+		}
+		memcpy(buf, end - len, len);
+		buf[len++] = '\n';
+		buf[len] = '\0';
+		return content;
+	}
+
+	return NULL;
+}
+
+static int tracefs_restore_file(const struct tracefs_ctl_entry *entry)
+{
+	char buf[CHUNK_SIZE];
+	const char *content = entry->old;
+	int err = 0;
+
+	if (!content)
+		return 0;
+
+	if (entry->kind == TRACEFS_CTL_FILTER) {
+		err = tracefs_write_file(entry->path, "");
+		if (err)
+			return err;
+	}
+
+	while ((content = tracefs_next_line(content, buf, sizeof(buf), &err))) {
+		err = tracefs_write_file(entry->path, buf);
+		if (err)
+			return err;
+	}
+
+	return err;
+}
+
+void tracefs_ctl_restore(struct tracefs_ctl *ctl)
+{
+	size_t i;
+	int err;
+
+	if (!ctl)
+		return;
+
+	for (i = ctl->cnt; i > 0; i--) {
+		struct tracefs_ctl_entry *entry = &ctl->entries[i - 1];
+
+		err = tracefs_restore_file(entry);
+		if (err)
+			fprintf(stderr, "Failed to restore %s: %s\n",
+				entry->path, strerror(-err));
+		free(entry->old);
+	}
+	free(ctl->entries);
+	free(ctl);
+}
+
 static size_t symbol_hash(long key, void *ctx __maybe_unused)
 {
 	return str_hash((const char *) key);
diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
index 0f8d99746084..432a95016b69 100644
--- a/tools/testing/selftests/bpf/trace_helpers.h
+++ b/tools/testing/selftests/bpf/trace_helpers.h
@@ -57,6 +57,15 @@ void read_trace_pipe(void);
 int read_trace_pipe_iter(void (*cb)(const char *str, void *data),
 			 void *data, int iter);
 
+struct tracefs_ctl;
+
+struct tracefs_ctl *tracefs_ctl_init(void);
+int tracefs_ctl_filter_write(struct tracefs_ctl *ctl, const char *file,
+			     const char *pattern);
+int tracefs_ctl_set(struct tracefs_ctl *ctl, const char *file,
+		    const char *value);
+void tracefs_ctl_restore(struct tracefs_ctl *ctl);
+
 ssize_t get_uprobe_offset(const void *addr);
 ssize_t get_rel_offset(uintptr_t addr);
 

-- 
2.56.0

Re: [PATCH bpf-next 1/2] selftests/bpf: add helpers to manipulate ftrace

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] selftests/bpf: unchecked realloc() in tracefs_read() causes NULL deref
- [Medium] selftests/bpf: tracefs_ctl_update() fails to truncate filter on first write
- [Medium] selftests/bpf: tracefs_ctl_update() ignores tracefs_read() failure
- [Medium] selftests/bpf: tracefs_write() drops multi-token filters on short write
- [Medium] selftests/bpf: tracefs helpers return -1 instead of -errno on syscall error
--

commit 569748d9037859f0dfb26ca03c10fd22fc3a99b9
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

selftests/bpf: add helpers to manipulate ftrace

This commit adds tracefs control helpers to tools/testing/selftests/bpf/
so tests can modify ftrace settings and filters and automatically restore
their original state upon completion.

> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index 6d7a4b8c4ab8c..1f26a057a810a 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c

[ ... ]

> @@ -557,6 +561,245 @@ void read_trace_pipe(void)
>  	read_trace_pipe_iter(trace_pipe_cb, NULL, 0);
>  }
>  

[ ... ]

> +static int tracefs_read(int fd, char **content)
> +{
> +	size_t len = 0;
> +	char tmp[CHUNK_SIZE];
> +	char *buf = NULL;
> +	ssize_t n;
> +	int ret;
> +
> +	ret = lseek(fd, 0, SEEK_SET);
> +	if (ret < 0)
> +		return ret;
> +
> +	do {
> +		n = read(fd, tmp, CHUNK_SIZE);
> +		if (n < 0) {
> +			if (buf)
> +				free(buf);
> +			return n;
> +		}
> +		if (!n)
> +			break;
> +		buf = realloc(buf, len + n + 1);
> +		memcpy(buf + len, tmp, n);

[Severity: Medium]
What happens in tracefs_read() if realloc() fails while reading chunks from
the tracefs file? Assigning the return value directly to buf loses the
existing buf allocation and then dereferences NULL + len in memcpy().

> +		len += n;
> +	} while (1);
> +
> +	/* No data read at all, file was empty */
> +	if (!buf) {
> +		buf = malloc(1);
> +		if (!buf)
> +			return -ENOMEM;
> +	}
> +	buf[len] = '\0';
> +	*content = buf;
> +	return 0;
> +}
> +
> +static int tracefs_write(int fd, const char *str)
> +{
> +	ssize_t ret;
> +
> +	ret = lseek(fd, 0, SEEK_SET);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = write(fd, str, strlen(str));
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}

[Severity: Medium]
Can a single write() call in tracefs_write() drop patterns when writing to
ftrace filter files?

In kernel/trace/ftrace.c, ftrace_regex_write() (for set_ftrace_filter and
set_ftrace_notrace) and ftrace_graph_write() (for set_graph_function and
set_graph_notrace) parse user input with trace_get_user(), which stops at
the first whitespace character and returns the byte count consumed for only
that single token.

When a caller passes multiple space-separated patterns (or a function with
a module suffix like "func [mod]\n" during tracefs_restore_file()), write()
returns a short byte count for the first token and tracefs_write() returns 0
without looping to write the remaining tokens.

In addition, if a pattern does not end with whitespace or a newline,
trace_get_user() marks the parser as a continuation (parser->cont = true)
and defers regex evaluation until close(), where release errors are not
reported back to close(fd).

> +
> +static int tracefs_write_file(const char *path, const char *str)
> +{
> +	int fd, err;
> +
> +	fd = open(path, O_CLOEXEC | O_WRONLY | (*str ? 0 : O_TRUNC));
> +	if (fd < 0)
> +		return fd;
> +
> +	err = tracefs_write(fd, str);
> +	close(fd);
> +	return err;
> +}

[ ... ]

> +static int tracefs_ctl_update(struct tracefs_ctl *ctl, const char *file,
> +			      const char *str, enum tracefs_ctl_kind kind)
> +{
> +	struct tracefs_ctl_entry *entry, *tmp;
> +	char path[PATH_MAX];
> +	char *old = NULL;
> +	bool recorded = false;
> +	size_t i;
> +	int fd, err;
> +
> +	if (!ctl)
> +		return -EINVAL;
> +
> +	err = tracefs_path(path, sizeof(path), file);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < ctl->cnt; i++) {
> +		if (strcmp(ctl->entries[i].path, path))
> +			continue;
> +		if (ctl->entries[i].kind != kind)
> +			return -EINVAL;
> +		/* Already altered, keep the value read on the first write. */
> +		return tracefs_write_file(ctl->entries[i].path, str);
> +	}
> +
> +	fd = open(path, O_CLOEXEC | O_RDWR);
> +	if (fd < 0)
> +		return fd;
> +
> +	err = tracefs_read(fd, &old);
> +	if (!err) {
> +		tmp = realloc(ctl->entries, (ctl->cnt + 1) * sizeof(*tmp));
> +		if (!tmp) {
> +			free(old);
> +			close(fd);
> +			return -ENOMEM;
> +		}
> +		ctl->entries = tmp;
> +		entry = &ctl->entries[ctl->cnt];
> +		strcpy(entry->path, path);
> +		entry->old = old;
> +		entry->kind = kind;
> +		recorded = true;
> +	}
> +
> +	err = tracefs_write(fd, str);
> +	close(fd);

[Severity: Medium]
Should tracefs_ctl_update() bail out when tracefs_read() returns an error?
Right now, if tracefs_read() fails, execution skips recording the entry in
ctl->entries and proceeds to tracefs_write(fd, str), which modifies the
tracefs file without saving its original value for tracefs_ctl_restore().

Also, when the first operation on a tracefs file passes an empty string ""
to clear it (such as clearing set_ftrace_filter or set_graph_notrace), does
calling tracefs_write(fd, str) on the O_RDWR descriptor actually clear the
filter?

In kernel/trace/ftrace.c (ftrace_regex_open() and __ftrace_graph_open()) and
kernel/trace/trace.c (tracing_open()), clearing a filter or trace file
requires opening it with O_TRUNC, whereas a 0-byte write() to a descriptor
opened without O_TRUNC is a no-op. Unlike tracefs_write_file(), which opens
with O_TRUNC when *str is empty, the first update in tracefs_ctl_update()
writes to the descriptor opened with open(path, O_CLOEXEC | O_RDWR). Should
tracefs_ctl_update() close fd after tracefs_read() and use
tracefs_write_file(path, str) instead?

[ ... ]

> +void tracefs_ctl_restore(struct tracefs_ctl *ctl)
> +{
> +	size_t i;
> +	int err;
> +
> +	if (!ctl)
> +		return;
> +
> +	for (i = ctl->cnt; i > 0; i--) {
> +		struct tracefs_ctl_entry *entry = &ctl->entries[i - 1];
> +
> +		err = tracefs_restore_file(entry);
> +		if (err)
> +			fprintf(stderr, "Failed to restore %s: %s\n",
> +				entry->path, strerror(-err));

[Severity: Medium]
In tracefs_ctl_restore(), strerror(-err) expects negative errno values,
which matches internal errors like -ENOMEM, -EINVAL, and -ENAMETOOLONG.
However, when open(), lseek(), read(), or write() fails in tracefs_read(),
tracefs_write(), tracefs_write_file(), or tracefs_ctl_update(), those
functions return -1 rather than -errno. Won't any syscall failure during
tracefs_restore_file() cause strerror(-err) to evaluate strerror(1) (EPERM)
instead of the real errno?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-ftrace_in_test_progs-v1-0-aa50c5dd1ccc@bootlin.com?part=1

Re: [PATCH bpf-next 1/2] selftests/bpf: add helpers to manipulate ftrace

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3178 bytes --]

> selftests/bpf: add helpers to manipulate ftrace

TL;DR: Restoring ftrace filters in trace_helpers.c may abort on a saved line
over 126 bytes (128-byte buffer), leaving the filter empty, so all functions
get traced and global ftrace state isn't restored.

> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index 6d7a4b8c4ab8..1f26a057a810 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c

[ ... ]

> +#define CHUNK_SIZE	128

[ ... ]

> +static const char *tracefs_next_line(const char *content, char *buf, size_t size,
> +				     int *err)
> +{
> +	const char *end;
> +	size_t len;
> +
> +	*err = 0;
> +	while (*content) {
> +		end = strchrnul(content, '\n');
> +		len = end - content;
> +		content = *end ? end + 1 : end;
> +
> +		if (!len || end[-len] == '#')
> +			continue;
> +
> +		if (len + 2 > size) {
> +			*err = -ENAMETOOLONG;
> +			return NULL;
> +		}
> +		memcpy(buf, end - len, len);
> +		buf[len++] = '\n';
> +		buf[len] = '\0';
> +		return content;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int tracefs_restore_file(const struct tracefs_ctl_entry *entry)
> +{
> +	char buf[CHUNK_SIZE];
> +	const char *content = entry->old;
> +	int err = 0;
> +
> +	if (!content)
> +		return 0;
> +
> +	if (entry->kind == TRACEFS_CTL_FILTER) {
> +		err = tracefs_write_file(entry->path, "");
> +		if (err)
> +			return err;
> +	}
> +
> +	while ((content = tracefs_next_line(content, buf, sizeof(buf), &err))) {
> +		err = tracefs_write_file(entry->path, buf);
> +		if (err)
> +			return err;
> +	}
> +
> +	return err;
> +}

Can a saved line longer than 126 bytes abort the restore here?

tracefs_restore_file() splits the saved content into a buffer sized by the
read chunk constant:

	char buf[CHUNK_SIZE];     /* CHUNK_SIZE is 128 */

and tracefs_next_line() fails the whole restore with -ENAMETOOLONG when
len + 2 > size.  The kernel can print much longer entries than that.
print_rec() emits the full symbol (KSYM_NAME_LEN is 512) plus the
" [module]" suffix, so mangled Rust symbols or long C names with a module
suffix can exceed 126 bytes.

For TRACEFS_CTL_FILTER entries the file has already been emptied by
tracefs_write_file(entry->path, "") before the long line is reached:

	if (entry->kind == TRACEFS_CTL_FILTER) {
		err = tracefs_write_file(entry->path, "");
		...
	}
	while ((content = tracefs_next_line(content, buf, sizeof(buf), &err))) {

so the restore stops part-way, every entry after the long line is lost, and
if none were restored yet the filter is left empty.  For set_ftrace_filter
an empty filter means all functions are traced, which is the opposite of
the original restriction, and the global ftrace state is not restored.

Should the line buffer be sized to the actual line length (or should the
content be written straight from the saved string) instead of being tied to
CHUNK_SIZE?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41113297251

[PATCH bpf-next 2/2] selftests/bpf: port test_ftrace.sh to test_progs

[Alexis Lothoré (eBPF Foundation)]

Commit ff205766dbbe ("ftrace: Fix function_graph tracer interaction with
BPF trampoline") made sure that trampolines generated for tracing
programs did not collide with the function graph when we are returning
from the hooked function. The series bringing this commit also came with
a new test ensuring that the kernel does not crash when executing BPF
programs handled by trampolines while the function graph trace is
enabled, but this test is a standalone shell script, hence not executed
in CI.

Convert the script to the test_progs runner so it can be executed
automatically by the CI infrastructure. There are two main options to
port this test:
  - just enable ftrace during some existing trampoline-related tests
    (eg: fentry/fexit tests)
  - create a dedicated, minimal test with function graph

The new test adopts the latter approach, with a smaller scope to keep
execution time reasonable. On top of some simple fentry/fexit program,
it also adds a small fmod_ret program to ensure that return values are
correctly handled while the function graph enabled. Despite the few
asserts in the new test, the actual goal  is to make sure that the
kernel does not crash because of some incorrect stack frame skipping
when both the function graph and tracing programs are running.

ftrace is configured through the new ftrace helpers, and restored to its
original state afterward. Despite being restored afterward, it is
configured for the whole system, so the test is marked as serial_ to
avoid conflicts with other tests needing traces (eg trace_vprintk)

With the new test integrated into test_progs, drop test_ftrace.sh.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
 .../selftests/bpf/prog_tests/fgraph_trampoline.c   | 129 +++++++++++++++++++++
 .../selftests/bpf/progs/fgraph_trampoline.c        |  29 +++++
 tools/testing/selftests/bpf/test_ftrace.sh         |  44 -------
 3 files changed, 158 insertions(+), 44 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/fgraph_trampoline.c b/tools/testing/selftests/bpf/prog_tests/fgraph_trampoline.c
new file mode 100644
index 000000000000..98edd77cf214
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/fgraph_trampoline.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "trace_helpers.h"
+#include "fgraph_trampoline.skel.h"
+
+#define TRACER_GRAPH	"function_graph"
+
+#define FTRACE_NOTRACE	"*printk* *console* *wake* *serial* *lock*"
+#define GRAPH_FUNCS	"bpf_prog_test*"
+
+/* See test_run.c: we are overriding bpf_modify_return_test with a program
+ * returning 5, but not bpf_modify_return_test2
+ */
+#define FMOD_EXPECTED_RETVAL ((1 << 16) | (5 + 1 + 3 + 3 + 4 + 5 + 6 + 7))
+
+static int configure_tracer(struct tracefs_ctl *ftrace)
+{
+	int err;
+
+	err = tracefs_ctl_filter_write(ftrace, "set_ftrace_filter", "");
+	if (err)
+		return err;
+	err = tracefs_ctl_filter_write(ftrace, "set_ftrace_notrace",
+				       FTRACE_NOTRACE);
+	if (err)
+		return err;
+	err = tracefs_ctl_filter_write(ftrace, "set_graph_function", GRAPH_FUNCS);
+	if (err)
+		return err;
+	err = tracefs_ctl_filter_write(ftrace, "set_graph_notrace", "");
+	if (err)
+		return err;
+	err = tracefs_ctl_set(ftrace, "current_tracer", TRACER_GRAPH);
+	if (err)
+		return err;
+
+	return err;
+}
+
+static int run_test_under_ftrace_graph(struct tracefs_ctl *ftrace,
+				       struct bpf_program *prog, int *retval)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+	int err, stop_err;
+
+	err = tracefs_ctl_set(ftrace, "tracing_on", "1");
+	if (err)
+		return err;
+
+	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &opts);
+	if (retval)
+		*retval = opts.retval;
+
+	stop_err = tracefs_ctl_set(ftrace, "tracing_on", "0");
+	if (!err)
+		err = stop_err;
+
+	return err;
+}
+
+void serial_test_fgraph_trampoline(void)
+{
+	struct fgraph_trampoline *skel = NULL;
+	struct tracefs_ctl *ftrace;
+	int retval;
+
+	ftrace = tracefs_ctl_init();
+	if (!ASSERT_OK_PTR(ftrace, "tracefs_ctl_init"))
+		return;
+
+	if (configure_tracer(ftrace)) {
+		test__skip();
+		tracefs_ctl_restore(ftrace);
+		goto cleanup;
+	}
+
+	skel = fgraph_trampoline__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "fgraph_trampoline__open_and_load"))
+		goto cleanup;
+
+	if (test__start_subtest("no_trampoline")) {
+		ASSERT_OK(run_test_under_ftrace_graph(ftrace,
+						      skel->progs.fentry_test1,
+						      NULL),
+			  "run test no trampoline");
+	}
+
+	if (test__start_subtest("attach_fentry_fexit")) {
+		skel->links.fentry_test1 =
+			bpf_program__attach(skel->progs.fentry_test1);
+		if (!ASSERT_OK_PTR(skel->links.fentry_test1, "attach_fentry"))
+			goto destroy_cleanup;
+		skel->links.fexit_test1 =
+			bpf_program__attach(skel->progs.fexit_test1);
+		if (!ASSERT_OK_PTR(skel->links.fexit_test1, "attach_fexit"))
+			goto destroy_cleanup;
+
+		memset(skel->bss, 0, sizeof(*skel->bss));
+		ASSERT_OK(run_test_under_ftrace_graph(ftrace,
+						      skel->progs.fentry_test1,
+						      NULL),
+			  "run test with fentry and fexit");
+
+		ASSERT_EQ(skel->bss->fexit_result, 1, "fexit_result");
+
+		fgraph_trampoline__detach(skel);
+	}
+
+	if (test__start_subtest("attach_fmod_ret")) {
+		skel->links.fmod_ret_test =
+			bpf_program__attach(skel->progs.fmod_ret_test);
+		if (!ASSERT_OK_PTR(skel->links.fmod_ret_test, "attach_fmod_ret"))
+			goto destroy_cleanup;
+
+		ASSERT_OK(run_test_under_ftrace_graph(ftrace,
+						      skel->progs.fmod_ret_test,
+						      &retval),
+			  "run test with fmod_ret");
+		ASSERT_EQ(retval, FMOD_EXPECTED_RETVAL, "fmod return value");
+
+		fgraph_trampoline__detach(skel);
+	}
+
+destroy_cleanup:
+	fgraph_trampoline__destroy(skel);
+cleanup:
+	tracefs_ctl_restore(ftrace);
+}
diff --git a/tools/testing/selftests/bpf/progs/fgraph_trampoline.c b/tools/testing/selftests/bpf/progs/fgraph_trampoline.c
new file mode 100644
index 000000000000..1cc2dadc817e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/fgraph_trampoline.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+__u64 fexit_result;
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(fentry_test1, int a)
+{
+	return 0;
+}
+
+SEC("fexit/bpf_fentry_test1")
+int BPF_PROG(fexit_test1, int a, int ret)
+{
+	fexit_result = (a == 1 && ret == 2);
+	return 0;
+}
+
+SEC("fmod_ret/bpf_modify_return_test")
+int BPF_PROG(fmod_ret_test, int a, int *b, int ret)
+{
+	return 5;
+}
+
+char _license[] SEC("license") = "GPL";
+
diff --git a/tools/testing/selftests/bpf/test_ftrace.sh b/tools/testing/selftests/bpf/test_ftrace.sh
deleted file mode 100755
index f5109eb0e951..000000000000
--- a/tools/testing/selftests/bpf/test_ftrace.sh
+++ /dev/null
@@ -1,44 +0,0 @@
-#!/bin/bash
-
-if [[ -e /sys/kernel/tracing/trace ]]; then
-    TR=/sys/kernel/tracing/
-else
-    TR=/sys/kernel/debug/tracing/
-fi
-
-clear_trace() { # reset trace output
-    echo > $TR/trace
-}
-
-disable_tracing() { # stop trace recording
-    echo 0 > $TR/tracing_on
-}
-
-enable_tracing() { # start trace recording
-    echo 1 > $TR/tracing_on
-}
-
-reset_tracer() { # reset the current tracer
-    echo nop > $TR/current_tracer
-}
-
-disable_tracing
-clear_trace
-
-echo "" > $TR/set_ftrace_filter
-echo '*printk* *console* *wake* *serial* *lock*' > $TR/set_ftrace_notrace
-
-echo "bpf_prog_test*" > $TR/set_graph_function
-echo "" > $TR/set_graph_notrace
-
-echo function_graph > $TR/current_tracer
-
-enable_tracing
-./test_progs -t fentry
-./test_progs -t fexit
-disable_tracing
-clear_trace
-
-reset_tracer
-
-exit 0

-- 
2.56.0

Re: [PATCH bpf-next 2/2] selftests/bpf: port test_ftrace.sh to test_progs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] selftests/bpf: double free of ftrace in serial_test_fgraph_trampoline()
- [Medium] selftests/bpf: incomplete filter write and clear in configure_tracer()
- [Medium] selftests/bpf: unchecked realloc() in tracefs_read()
- [Medium] selftests/bpf: ignored tracefs_read() error in tracefs_ctl_update()
- [Low] selftests/bpf: uninitialized retval read in serial_test_fgraph_trampoline()
- [Low] selftests/bpf: multi-line comment style violation in fgraph_trampoline.c
--

commit 504c8aaf5a6be7eba87f8423527b44e26046eff9
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

selftests/bpf: port test_ftrace.sh to test_progs

This commit ports the standalone test_ftrace.sh script to a serial test_progs
test (fgraph_trampoline) using the new tracefs_ctl helpers, testing fentry,
fexit, and fmod_ret BPF programs while the function_graph tracer is enabled,
and removes test_ftrace.sh.

> diff --git a/tools/testing/selftests/bpf/prog_tests/fgraph_trampoline.c b/tools/testing/selftests/bpf/prog_tests/fgraph_trampoline.c
> new file mode 100644
> index 0000000000000..98edd77cf214e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/fgraph_trampoline.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "trace_helpers.h"
> +#include "fgraph_trampoline.skel.h"
> +
> +#define TRACER_GRAPH	"function_graph"
> +
> +#define FTRACE_NOTRACE	"*printk* *console* *wake* *serial* *lock*"
> +#define GRAPH_FUNCS	"bpf_prog_test*"
> +
> +/* See test_run.c: we are overriding bpf_modify_return_test with a program
> + * returning 5, but not bpf_modify_return_test2
> + */

[Severity: Low]
This isn't a bug, but in BPF code and selftests, should multi-line comments
place the opening /* on its own line with the comment text starting on the
following line?

> +#define FMOD_EXPECTED_RETVAL ((1 << 16) | (5 + 1 + 3 + 3 + 4 + 5 + 6 + 7))
> +
> +static int configure_tracer(struct tracefs_ctl *ftrace)
> +{
> +	int err;
> +
> +	err = tracefs_ctl_filter_write(ftrace, "set_ftrace_filter", "");
> +	if (err)
> +		return err;
> +	err = tracefs_ctl_filter_write(ftrace, "set_ftrace_notrace",
> +				       FTRACE_NOTRACE);
> +	if (err)
> +		return err;
> +	err = tracefs_ctl_filter_write(ftrace, "set_graph_function", GRAPH_FUNCS);
> +	if (err)
> +		return err;
> +	err = tracefs_ctl_filter_write(ftrace, "set_graph_notrace", "");
> +	if (err)
> +		return err;

[Severity: Medium]
Do these tracefs_ctl_filter_write() calls in configure_tracer() actually clear
existing filters and write all patterns in FTRACE_NOTRACE?

First, when configure_tracer() passes "" to clear set_ftrace_filter and
set_graph_notrace on the first access to those files, tracefs_ctl_update() in
tools/testing/selftests/bpf/trace_helpers.c opens the file without O_TRUNC and
writes to the same descriptor instead of calling tracefs_write_file():

    fd = open(path, O_CLOEXEC | O_RDWR);
    if (fd < 0)
        return fd;

    err = tracefs_read(fd, &old);
    ...
    err = tracefs_write(fd, str);
    close(fd);

In the kernel (ftrace_regex_open() and __ftrace_graph_open() in
kernel/trace/ftrace.c), filter hashes are only cleared when the file is opened
with O_TRUNC; opening without O_TRUNC copies the existing filter hash, and a
zero-byte write(fd, "", 0) returns 0 without clearing it. In addition,
configure_tracer() does not clear set_ftrace_notrace or set_graph_function
before writing to them, so pre-existing filters in all four files remain active.

Second, FTRACE_NOTRACE contains multiple space-separated patterns
("*printk* *console* *wake* *serial* *lock*"). In the kernel,
ftrace_notrace_write() -> ftrace_regex_write() -> trace_get_user() consumes
only one whitespace-delimited token per write() call and returns the number of
bytes consumed for that first token. Because tracefs_write() in
tools/testing/selftests/bpf/trace_helpers.c does a single write() without
looping over partial writes:

    ret = write(fd, str, strlen(str));
    if (ret < 0)
        return ret;
    return 0;

won't all patterns after "*printk*" be silently dropped?

> +	err = tracefs_ctl_set(ftrace, "current_tracer", TRACER_GRAPH);
> +	if (err)
> +		return err;
> +
> +	return err;
> +}

[Severity: Medium]
When configure_tracer() calls tracefs_ctl_filter_write() or tracefs_ctl_set(),
tracefs_ctl_update() invokes tracefs_read() in
tools/testing/selftests/bpf/trace_helpers.c:

    do {
        n = read(fd, tmp, CHUNK_SIZE);
        if (n < 0) {
            if (buf)
                free(buf);
            return n;
        }
        if (!n)
            break;
        buf = realloc(buf, len + n + 1);
        memcpy(buf + len, tmp, n);
        len += n;
    } while (1);

If realloc() fails and returns NULL, doesn't this leak the prior buf allocation
and immediately dereference NULL in memcpy(buf + len, tmp, n)?

> +
> +static int run_test_under_ftrace_graph(struct tracefs_ctl *ftrace,
> +				       struct bpf_program *prog, int *retval)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	int err, stop_err;
> +
> +	err = tracefs_ctl_set(ftrace, "tracing_on", "1");
> +	if (err)
> +		return err;
> +
> +	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &opts);
> +	if (retval)
> +		*retval = opts.retval;
> +
> +	stop_err = tracefs_ctl_set(ftrace, "tracing_on", "0");
> +	if (!err)
> +		err = stop_err;
> +
> +	return err;
> +}
> +
> +void serial_test_fgraph_trampoline(void)
> +{
> +	struct fgraph_trampoline *skel = NULL;
> +	struct tracefs_ctl *ftrace;
> +	int retval;
> +
> +	ftrace = tracefs_ctl_init();
> +	if (!ASSERT_OK_PTR(ftrace, "tracefs_ctl_init"))
> +		return;
> +
> +	if (configure_tracer(ftrace)) {
> +		test__skip();
> +		tracefs_ctl_restore(ftrace);
> +		goto cleanup;
> +	}

[Severity: Medium]
If configure_tracer(ftrace) fails in serial_test_fgraph_trampoline(), won't
calling tracefs_ctl_restore(ftrace) before jumping to cleanup cause a
use-after-free and double-free of ftrace?

In tools/testing/selftests/bpf/trace_helpers.c, tracefs_ctl_restore() frees
each entry->old buffer, ctl->entries, and ctl itself:

    for (i = ctl->cnt; i > 0; i--) {
        struct tracefs_ctl_entry *entry = &ctl->entries[i - 1];
        ...
        free(entry->old);
    }
    free(ctl->entries);
    free(ctl);

Jumping to the cleanup label then calls tracefs_ctl_restore(ftrace) a second
time on the already freed pointer.

[ ... ]

> +	if (test__start_subtest("attach_fmod_ret")) {
> +		skel->links.fmod_ret_test =
> +			bpf_program__attach(skel->progs.fmod_ret_test);
> +		if (!ASSERT_OK_PTR(skel->links.fmod_ret_test, "attach_fmod_ret"))
> +			goto destroy_cleanup;
> +
> +		ASSERT_OK(run_test_under_ftrace_graph(ftrace,
> +						      skel->progs.fmod_ret_test,
> +						      &retval),
> +			  "run test with fmod_ret");
> +		ASSERT_EQ(retval, FMOD_EXPECTED_RETVAL, "fmod return value");

[Severity: Low]
In serial_test_fgraph_trampoline(), retval is declared without initialization.
If run_test_under_ftrace_graph() fails at tracefs_ctl_set(ftrace, "tracing_on",
"1") and returns early before writing *retval, ASSERT_OK() will record the
failure without returning and ASSERT_EQ(retval, FMOD_EXPECTED_RETVAL, ...) will
read the uninitialized retval variable. Could retval be initialized or only
checked when run_test_under_ftrace_graph() succeeds?

> +
> +		fgraph_trampoline__detach(skel);
> +	}
> +
> +destroy_cleanup:
> +	fgraph_trampoline__destroy(skel);
> +cleanup:
> +	tracefs_ctl_restore(ftrace);
> +}

[Severity: Medium]
Does tracefs_ctl_update() in tools/testing/selftests/bpf/trace_helpers.c leave
tracefs state unrecorded for tracefs_ctl_restore() if tracefs_read() fails?

In tracefs_ctl_update():

    err = tracefs_read(fd, &old);
    if (!err) {
        ...
        recorded = true;
    }

    err = tracefs_write(fd, str);

If tracefs_read(fd, &old) returns an error, tracefs_ctl_update() skips recording
the original value in ctl->entries but still calls tracefs_write(fd, str),
modifying the system-wide tracefs file without saving its previous state for
tracefs_ctl_restore().

Also, in tracefs_ctl_restore():

    err = tracefs_restore_file(entry);
    if (err)
        fprintf(stderr, "Failed to restore %s: %s\n",
                entry->path, strerror(-err));

open(), lseek(), and write() failures in tracefs_write_file() and
tracefs_write() return -1 rather than -errno, so passing -err to strerror()
will always report EPERM when a syscall fails.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-ftrace_in_test_progs-v1-0-aa50c5dd1ccc@bootlin.com?part=2