On Mon, 10 Jul 2023 18:16:01 -0400
Steven Rostedt [off-list ref] wrote:

On Sat,  8 Jul 2023 11:48:58 +0900
"Masami Hiramatsu (Google)" [off-list ref] wrote:

quoted

--- 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);
}

To avoid confusion, I would like to revert the set_data_loc() at the 3rd patch
and add it again in 4th patch.

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);
}

and introduce it. I also want to put the ternary operator into set_data_loc() too
for simplicity.

static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
{
	if (ret < 0)
		ret = 0;
	*(u32 *)dest = make_data_loc(ret, __dest - base);
}

Thanks,

That would keep the complexity down in this changes set.

-- Steve

quoted

 
 	return ret;
 }

-- 
Masami Hiramatsu (Google) [off-list ref]