Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 11 Jul 2023 13:56:03 +0900 | From | Masami Hiramatsu (Google) <> | Subject | Re: [PATCH v4 4/4] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails |
| |
On Mon, 10 Jul 2023 23:34:00 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 11 Jul 2023 11:11:51 +0900 > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > > > --- a/kernel/trace/trace_probe_tmpl.h > > +++ b/kernel/trace/trace_probe_tmpl.h > > @@ -267,9 +267,7 @@ store_trace_args(void *data, struct trace_probe *tp, void *rec, > > if (unlikely(arg->dynamic)) > > *dl = make_data_loc(maxlen, dyndata - base); > > ret = process_fetch_insn(arg->code, rec, dl, base); > > - if (unlikely(ret < 0 && arg->dynamic)) { > > - *dl = make_data_loc(0, dyndata - base); > > - } else { > > + if (unlikely(ret > 0 && arg->dynamic)) { > > To match the current code, that should be: > > if (likely(ret >= 0 || !arg->dynamic)) { > > But I'm guessing that the original code was buggy, as the else block should > only have been processed if arg->dynamic was set?
Good point, yes, that's right. Since dyndata and maxlen is only used when arg->dynamic == true, we don't have to care about that.
> That is, it should have been: > > if (arg->dynamic) { > if (unlikely(ret < 0)) { > *dl = make_data_loc(0, dyndata - base); > } else { > dyndata += ret; > maxlen -= ret; > } > } > > > I guess you only want to update if arg->dynamic is true (even though that > wasn't the case before :-/) But in any case, I think you want likely() and > not unlikely(). > > if (arg->dynamic && likely(ret > 0)) { > > That is, if we only want to updated this if the arg is dynamic.
Indeed.
> > And I don't think that the arg->dynamic() should have likely/unlikely > around it, as that's determined by user space, and the kernel should not be > adding assumptions about what user space wants.
OK.
Let me fix that with a new patch because it is another bug.
Thanks,
> > -- Steve > > > dyndata += ret; > > maxlen -= ret; > > }
-- Masami Hiramatsu (Google) <mhiramat@kernel.org>
| ![\](/images/icornerr.gif) |