Messages in this thread | | | Date | Mon, 10 Jul 2023 18:16:01 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH v3 4/4] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails |
| |
On Sat, 8 Jul 2023 11:48:58 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> --- a/kernel/trace/trace_probe_kernel.h > +++ b/kernel/trace/trace_probe_kernel.h > @@ -55,8 +55,7 @@ fetch_store_string_user(unsigned long addr, void *dest, void *base) > __dest = get_loc_data(dest, base); > > ret = strncpy_from_user_nofault(__dest, uaddr, maxlen); > - if (ret >= 0) > - *(u32 *)dest = make_data_loc(ret, __dest - base); > + *(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base); > > return ret; > } > @@ -87,8 +86,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base) > * probing. > */ > ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen); > - if (ret >= 0) > - *(u32 *)dest = make_data_loc(ret, __dest - base); > + *(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);
The above is a complex line, and not something that I think should be cut and pasted between two different locations.
I know you took out the set_data_loc() helper, but really it should have stayed, and have used that to update this code in the two places it affected, instead of making the changes in those two locations.
That is, patch 3 could have had kept.
static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base) { if (ret >= 0) *(u32 *)dest = make_data_loc(ret, __dest - base); }
And this patch could have been:
static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base) { *(u32 *)dest = make_data_loc(ret, __dest - base); }
That would keep the complexity down in this changes set.
-- Steve
> > return ret; > }
| |