Re: [RFC PATCH 1/2] tracing: fixes of ftrace_enable_fops
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Date: 2025-06-13 02:45:32
Also in:
lkml
On Thu, 12 Jun 2025 12:43:48 +0200 Gabriele Paoloni [off-list ref] wrote:
Currently there are different issues associated with ftrace_enable_fops - event_enable_write: *ppos is increased while not used at all in the write operation itself (following a write, this could lead a read to fail or report a corrupted event status);
Here, we expected the "enable" file is a pseudo text file. So if there is a write, the ppos should be incremented.
- event_enable_read: cnt < strlen(buf) is allowed and this can lead to reading an incomplete event status (i.e. not all status characters are retrieved) and/or reading the status in a non-atomic way (i.e. the status could change between two consecutive reads);
As I said, the "enable" file is a kind of text file. So reader must read it until EOF. If you need to get the consistent result, user should use the enough size of buffer.
- .llseek is set to default_llseek: this is wrong since for this type of files it does not make sense to reposition the ppos offset. Hence this should be set instead to noop_llseek.
As I said, it is a kind of text file, default_llseek is better. But, if we change (re-design) what is this "enable" file is, we can accept these changes. So this is not a "Fix" but re-design of the "enable" file as an interface (as a char device), not a text file (or a block device). I want to keep this as is, same as other tracefs files. Thank you,
quoted hunk ↗ jump to hunk
This patch fixes all the issues listed above. Signed-off-by: Gabriele Paoloni <redacted> Tested-by: Alessandro Carminati <redacted> --- kernel/trace/trace_events.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c index 120531268abf..5e84ef01d0c8 100644 --- a/kernel/trace/trace_events.c +++ b/kernel/trace/trace_events.c@@ -1798,6 +1798,13 @@ event_enable_read(struct file *filp, char __user *ubuf, size_t cnt, strcat(buf, "\n"); + /* + * A requested cnt less than strlen(buf) could lead to a wrong + * event status being reported. + */ + if (cnt < strlen(buf)) + return -EINVAL; + return simple_read_from_buffer(ubuf, cnt, ppos, buf, strlen(buf)); }@@ -1833,8 +1840,6 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt, return -EINVAL; } - *ppos += cnt; - return cnt; }@@ -2557,7 +2562,7 @@ static const struct file_operations ftrace_enable_fops = { .read = event_enable_read, .write = event_enable_write, .release = tracing_release_file_tr, - .llseek = default_llseek, + .llseek = noop_llseek, }; static const struct file_operations ftrace_event_format_fops = {-- 2.48.1
-- Masami Hiramatsu (Google) [off-list ref]