On Thu, 12 Jun 2025 12:43:48 +0200 Gabriele Paoloni <gpaol...@redhat.com> 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, > > This patch fixes all the issues listed above. > > Signed-off-by: Gabriele Paoloni <gpaol...@redhat.com> > Tested-by: Alessandro Carminati <acarm...@redhat.com> > --- > 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) <mhira...@kernel.org>