Re: [PATCH v4 4/4] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2023-07-11 03:34:11
Also in:
lkml
On Tue, 11 Jul 2023 11:11:51 +0900 "Masami Hiramatsu (Google)" [off-list ref] wrote:
quoted hunk ↗ jump to hunk
--- 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? 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.
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.
-- Steve
dyndata += ret; maxlen -= ret; }