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

[sashiko-bot@kernel.org]

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