Re: [PATCH v2] kernel/trace: Fix cleanup logic of enable_trace_eprobe

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 30 Jun 2023 15:16:27 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:


Hi Tzvetomir,

FYI, linux-trace-devel is for the tracing user space code, please Cc to
linux-trace-kernel for kernel patches. That makes it fall into the
proper patchwork.

I noticed this because I couldn't find your patch in:

  https://patchwork.kernel.org/project/linux-trace-kernel/list/

Also, the Subject should just start with "tracing:".

> The enable_trace_eprobe() function enables all event probes, attached
> to given trace probe. If an error occurs in enabling one of the event
> probes, all others should be roll backed. There is a bug in that roll
> back logic - instead of all event probes, only the failed one is
> disabled.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
> v2: Added one-time warning, as suggested by Steven Rostedt.

It's always a nice touch (optional, but something I always do) to
add a link to the previous version:

 Changes since v2: https://lore.kernel.org/all/20230628121811.338655-1-tz.stoyanov@gmail.com/
     - Added one-time warning (Steven Rostedt)

> 
>  kernel/trace/trace_eprobe.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 67e854979d53..6629fa217c99 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -702,8 +702,18 @@ static int enable_trace_eprobe(struct trace_event_call *call,
>  
>  	if (ret) {
>  		/* Failed to enable one of them. Roll back all */
> -		if (enabled)
> -			disable_eprobe(ep, file->tr);
> +		if (enabled) {
> +			/*
> +			 * It's a bug if one failed for something other than memory
> +			 * not being available but another eprobe succeeded.
> +			 */
> +			WARN_ON_ONCE(ret != -ENOMEM);
> +
> +			list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> +				ep = container_of(pos, struct trace_eprobe, tp);
> +				disable_eprobe(ep, file->tr);
> +			}

I think we may need the counter again ;-)

But for another reason. We only want to call disable for what we
enabled, to avoid any unforeseen side effects.


	cnt = 0;
        list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
                ep = container_of(pos, struct trace_eprobe, tp);
                ret = enable_eprobe(ep, file);
                if (ret)
                        break;
                enabled = true;
		cnt++;
        }

        if (ret) {
                /* Failed to enable one of them. Roll back all */
                if (enabled) {
			list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
				ep = container_of(pos, struct trace_eprobe, tp);
				disable_eprobe(ep, file->tr);
				if (!--cnt)
					break;
			}
		}

Thoughts?

-- Steve



> +		}
>  		if (file)
>  			trace_probe_remove_file(tp, file);
>  		else