Re: [RFC PATCH] tracing: fix return value in __ftrace_event_enable_disable for TRACE_REG_UNREGISTER

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 14 Mar 2025 13:57:25 +0100
Gabriele Paoloni <gpaoloni@redhat.com> wrote:

> When __ftrace_event_enable_disable invokes the class callback to
> unregister the event, the return value is not reported up to the
> caller, hence leading to event unregister failures being silently
> ignored.
> 
> This patch assigns the ret variable to the invocation of the
> event unregister callback, so that its return value is stored
> and reported to the caller.

Just out of curiosity, have you saw such issue? I think
event unregister should be succeeded or it warns the
fault.

> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---
> Sending this as RFC since I am not sure if checking the ret
> value is really needed.
> I have been mainly driven by the implementation of
> disable_trace_kprobe, disable_trace_fprobe,
> tracepoint_probe_unregister, disable_trace_eprobe that can
> return an error.
> 
>  kernel/trace/trace_events.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 513de9ceb80e..8d92b271ce0d 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -790,7 +790,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
>  				clear_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags);
>  			}
>  
> -			call->class->reg(call, TRACE_REG_UNREGISTER, file);
> +			ret = call->class->reg(call, TRACE_REG_UNREGISTER, file);

This is not enough. As same as enable failure, this function needs to handle
this error to report it and break.

Thank you,

>  		}
>  		/* If in SOFT_MODE, just set the SOFT_DISABLE_BIT, else clear it */
>  		if (file->flags & EVENT_FILE_FL_SOFT_MODE)
> -- 
> 2.48.1
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>