Re: [PATCH] tracing: Have trace_event_file have ref counters

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 2 Nov 2023 11:14:33 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > 
> > What happens here is that the kprobe event creates a trace_event_file
> > "file" descriptor that represents the file in tracefs to the event. It
> > maintains state of the event (is it enabled for the given instance?).
> > Opening the "enable" file gets a reference to the event "file" descriptor
> > via the open file descriptor. When the kprobe event is deleted, the file is
> > also deleted from the tracefs system which also frees the event "file"
> > descriptor.  
> 
> Ouch! I thought the file descriptor has been hold by the opened process.

Well, the struct *filp is, but not the filp->private that points to the
struct trace_event_file *file.

> 
> > 
> > But as the tracefs file is still opened by user space, it will not be
> > totally removed until the final dput() is called on it. But this is not
> > true with the event "file" descriptor that is already freed. If the user
> > does a write to or simply closes the file descriptor it will reference the
> > event "file" descriptor that was just freed, causing a use-after-free bug.
> > 
> > To solve this, add a ref count to the event "file" descriptor as well as a
> > new flag called "FREED". The "file" will not be freed until the last
> > reference is released. But the FREE flag will be set when the event is
> > removed to prevent any more modifications to that event from happening,
> > even if there's still a reference to the event "file" descriptor.
> > 
> > Link: https://lore.kernel.org/linux-trace-kernel/20231031000031.1e705592@gandalf.local.home/
> >   
> 
> Looks good to me.
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> BTW, can we add some tracefs selftests to ftracetest or independent test?

Yes, I was thinking about this, but that will have to wait till after this
gets in mainline. I'm already way behind schedule.

Thanks,

-- Steve