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>

Reply via email to