| Message ID | 72b05526-4dda-430a-b4ca-2ee4f26f2185@moroto.mountain (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | [RFC] tracing: Cleanup the correct ep in enable_trace_eprobe() | expand |
On Mon, Jun 26, 2023 at 4:35 PM Dan Carpenter <dan.carpenter@linaro.org> 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@linaro.org> > --- > 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 >
On Tue, Jun 27, 2023 at 07:39:17AM +0300, Tzvetomir Stoyanov wrote: > On Mon, Jun 26, 2023 at 4:35 PM Dan Carpenter <dan.carpenter@linaro.org> 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! Okay, but if the loop fails on the first iteration then it won't disable the first ep. Is that an issue? regards, dan carpenter
On Tue, Jun 27, 2023 at 8:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Tue, Jun 27, 2023 at 07:39:17AM +0300, Tzvetomir Stoyanov wrote: > > On Mon, Jun 26, 2023 at 4:35 PM Dan Carpenter <dan.carpenter@linaro.org> 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! > > Okay, but if the loop fails on the first iteration then it won't disable > the first ep. Is that an issue? > I looked at the code again, you are right - there is a problem. Indeed, that clean-up logic looks totally wrong, all eporbes must be disabled. I'll submit a patch. Thanks Dan. > regards, > dan carpenter >
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
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)... 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@linaro.org> --- kernel/trace/trace_eprobe.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)