On Mon, Jun 26, 2023 at 4:35 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> If enable_eprobe() function fails, then we call disable_eprobe() on the
> "ep" that failed. That doesn't feel right. Shouldn't we
> call disable_eprobe() on the previous "ep" instead? Or even better on
> all the previous eps (but I don't know how to do that)...
Hi Dan,
There is no need to disable the eprobes which are already successfully
registered to the given trace probe, as they will be disabled using
the normal logic. The failed epropbe is not registered there, that's
why it must be disabled explicitly. Thanks for digging into that
code!
>
> This patch is not tested at all. I'm mostly sending it as a kind of
> bug report. If this patch is correct or the fix is simple enough for
> an absolute moron to fix it then I can probably do that. But if it's
> something I'm too stupid to handle then could you just give me reported
> by credit? (That is the solution I really would prefer).
>
> Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> kernel/trace/trace_eprobe.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index b5181d690b4d..29de54347b8c 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -640,8 +640,8 @@ static int disable_eprobe(struct trace_eprobe *ep,
> static int enable_trace_eprobe(struct trace_event_call *call,
> struct trace_event_file *file)
> {
> + struct trace_eprobe *ep, *prev = NULL;
> struct trace_probe *pos, *tp;
> - struct trace_eprobe *ep;
> bool enabled;
> int ret = 0;
>
> @@ -666,13 +666,13 @@ static int enable_trace_eprobe(struct trace_event_call *call,
> ret = enable_eprobe(ep, file);
> if (ret)
> break;
> - enabled = true;
> + prev = ep;
> }
>
> if (ret) {
> /* Failed to enable one of them. Roll back all */
> - if (enabled)
> - disable_eprobe(ep, file->tr);
> + if (prev)
> + disable_eprobe(prev, file->tr);
> if (file)
> trace_probe_remove_file(tp, file);
> else
> --
> 2.39.2
>
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
