linux-trace-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/5] tracing/probes: Fix bugs in process_fetch_insn
@ 2023-07-11 14:15 Masami Hiramatsu (Google)
  2023-07-11 14:15 ` [PATCH v5 1/5] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-07-11 14:15 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

Hi,

Here are the 5th version of fix bugs in process_fetch_insn_*().
The previous version is here;

https://lore.kernel.org/all/168904147563.2908673.18054267804278861545.stgit@mhiramat.roam.corp.google.com/

In this version I added a bugfix to update dynamic data counter only
if the fetcharg uses it [3/5] and update [5/5] to move out the arg->dynamic
check out from unlikely() macro and use likely() macro correctly for
non-error case.

Thank you,

---

Masami Hiramatsu (Google) (5):
      tracing/probes: Fix to avoid double count of the string length on the array
      tracing/probes: Fix not to count error code to total length
      tracing/probes: Fix to update dynamic data counter if fetcharg uses it
      Revert "tracing: Add "(fault)" name injection to kernel probes"
      tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails


 kernel/trace/trace.h              |    2 ++
 kernel/trace/trace_probe.c        |    2 +-
 kernel/trace/trace_probe_kernel.h |   30 ++++++++----------------------
 kernel/trace/trace_probe_tmpl.h   |   10 +++++-----
 kernel/trace/trace_uprobe.c       |    3 ++-
 5 files changed, 18 insertions(+), 29 deletions(-)

--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v5 1/5] tracing/probes: Fix to avoid double count of the string length on the array
  2023-07-11 14:15 [PATCH v5 0/5] tracing/probes: Fix bugs in process_fetch_insn Masami Hiramatsu (Google)
@ 2023-07-11 14:15 ` Masami Hiramatsu (Google)
  2023-07-11 14:15 ` [PATCH v5 2/5] tracing/probes: Fix not to count error code to total length Masami Hiramatsu (Google)
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-07-11 14:15 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

If an array is specified with the ustring or symstr, the length of the
strings are accumlated on both of 'ret' and 'total', which means the
length is double counted.
Just set the length to the 'ret' value for avoiding double counting.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
Fixes: 88903c464321 ("tracing/probe: Add ustring type for user-space string")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_probe_tmpl.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 00707630788d..4735c5cb76fa 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -156,11 +156,11 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
 			code++;
 			goto array;
 		case FETCH_OP_ST_USTRING:
-			ret += fetch_store_strlen_user(val + code->offset);
+			ret = fetch_store_strlen_user(val + code->offset);
 			code++;
 			goto array;
 		case FETCH_OP_ST_SYMSTR:
-			ret += fetch_store_symstrlen(val + code->offset);
+			ret = fetch_store_symstrlen(val + code->offset);
 			code++;
 			goto array;
 		default:


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 2/5] tracing/probes: Fix not to count error code to total length
  2023-07-11 14:15 [PATCH v5 0/5] tracing/probes: Fix bugs in process_fetch_insn Masami Hiramatsu (Google)
  2023-07-11 14:15 ` [PATCH v5 1/5] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
@ 2023-07-11 14:15 ` Masami Hiramatsu (Google)
  2023-07-11 14:15 ` [PATCH v5 3/5] tracing/probes: Fix to update dynamic data counter if fetcharg uses it Masami Hiramatsu (Google)
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-07-11 14:15 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Fix not to count the error code (which is minus value) to the total
used length of array, because it can mess up the return code of
process_fetch_insn_bottom(). Also clear the 'ret' value because it
will be used for calculating next data_loc entry.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/8819b154-2ba1-43c3-98a2-cbde20892023@moroto.mountain/
Fixes: 9b960a38835f ("tracing: probeevent: Unify fetch_insn processing common part")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace_probe_tmpl.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 4735c5cb76fa..ed9d57c6b041 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -204,6 +204,8 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
 array:
 	/* the last stage: Loop on array */
 	if (code->op == FETCH_OP_LP_ARRAY) {
+		if (ret < 0)
+			ret = 0;
 		total += ret;
 		if (++i < code->param) {
 			code = s3;


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 3/5] tracing/probes: Fix to update dynamic data counter if fetcharg uses it
  2023-07-11 14:15 [PATCH v5 0/5] tracing/probes: Fix bugs in process_fetch_insn Masami Hiramatsu (Google)
  2023-07-11 14:15 ` [PATCH v5 1/5] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
  2023-07-11 14:15 ` [PATCH v5 2/5] tracing/probes: Fix not to count error code to total length Masami Hiramatsu (Google)
@ 2023-07-11 14:15 ` Masami Hiramatsu (Google)
  2023-07-13 13:34   ` Steven Rostedt
  2023-07-11 14:15 ` [PATCH v5 4/5] Revert "tracing: Add "(fault)" name injection to kernel probes" Masami Hiramatsu (Google)
  2023-07-11 14:16 ` [PATCH v5 5/5] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails Masami Hiramatsu (Google)
  4 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-07-11 14:15 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Fix to update dynamic data counter ('dyndata') and max length ('maxlen')
only if the fetcharg uses the dynamic data. Also get out arg->dynamic
from unlikely(). This makes dynamic data address wrong if
process_fetch_insn() returns error on !arg->dynamic case.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Link: https://lore.kernel.org/all/20230710233400.5aaf024e@gandalf.local.home/
Fixes: 9178412ddf5a ("tracing: probeevent: Return consumed bytes of dynamic area")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe_tmpl.h |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index ed9d57c6b041..185da001f4c3 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -267,11 +267,13 @@ 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 {
-			dyndata += ret;
-			maxlen -= ret;
+		if (arg->dynamic) {
+			if (unlikely(ret < 0)) {
+				*dl = make_data_loc(0, dyndata - base);
+			} else {
+				dyndata += ret;
+				maxlen -= ret;
+			}
 		}
 	}
 }


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 4/5] Revert "tracing: Add "(fault)" name injection to kernel probes"
  2023-07-11 14:15 [PATCH v5 0/5] tracing/probes: Fix bugs in process_fetch_insn Masami Hiramatsu (Google)
                   ` (2 preceding siblings ...)
  2023-07-11 14:15 ` [PATCH v5 3/5] tracing/probes: Fix to update dynamic data counter if fetcharg uses it Masami Hiramatsu (Google)
@ 2023-07-11 14:15 ` Masami Hiramatsu (Google)
  2023-07-11 16:19   ` Steven Rostedt
  2023-07-11 14:16 ` [PATCH v5 5/5] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails Masami Hiramatsu (Google)
  4 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-07-11 14:15 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

This reverts commit 2e9906f84fc7c99388bb7123ade167250d50f1c0.

It was turned out that commit 2e9906f84fc7 ("tracing: Add "(fault)"
name injection to kernel probes") did not work correctly and probe
events still show just '(fault)' (instead of '"(fault)"'). Also,
current '(fault)' is more explicit that it faulted.

This also moves FAULT_STRING macro to trace.h so that synthetic
event can keep using it, and uses it in trace_probe.c too.

Link: https://lore.kernel.org/all/20230706230642.3793a593@rorschach.local.home/

Cc: stable@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tom Zanussi <zanussi@kernel.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace.h              |    2 ++
 kernel/trace/trace_probe.c        |    2 +-
 kernel/trace/trace_probe_kernel.h |   31 ++++++-------------------------
 3 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 79bdefe9261b..eee1f3ca4749 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -113,6 +113,8 @@ enum trace_type {
 #define MEM_FAIL(condition, fmt, ...)					\
 	DO_ONCE_LITE_IF(condition, pr_err, "ERROR: " fmt, ##__VA_ARGS__)
 
+#define FAULT_STRING "(fault)"
+
 #define HIST_STACKTRACE_DEPTH	16
 #define HIST_STACKTRACE_SIZE	(HIST_STACKTRACE_DEPTH * sizeof(unsigned long))
 #define HIST_STACKTRACE_SKIP	5
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 2d2616678295..591399ddcee5 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -65,7 +65,7 @@ int PRINT_TYPE_FUNC_NAME(string)(struct trace_seq *s, void *data, void *ent)
 	int len = *(u32 *)data >> 16;
 
 	if (!len)
-		trace_seq_puts(s, "(fault)");
+		trace_seq_puts(s, FAULT_STRING);
 	else
 		trace_seq_printf(s, "\"%s\"",
 				 (const char *)get_loc_data(data, ent));
diff --git a/kernel/trace/trace_probe_kernel.h b/kernel/trace/trace_probe_kernel.h
index c4e1d4c03a85..6deae2ce34f8 100644
--- a/kernel/trace/trace_probe_kernel.h
+++ b/kernel/trace/trace_probe_kernel.h
@@ -2,8 +2,6 @@
 #ifndef __TRACE_PROBE_KERNEL_H_
 #define __TRACE_PROBE_KERNEL_H_
 
-#define FAULT_STRING "(fault)"
-
 /*
  * This depends on trace_probe.h, but can not include it due to
  * the way trace_probe_tmpl.h is used by trace_kprobe.c and trace_eprobe.c.
@@ -15,16 +13,8 @@ static nokprobe_inline int
 fetch_store_strlen_user(unsigned long addr)
 {
 	const void __user *uaddr =  (__force const void __user *)addr;
-	int ret;
 
-	ret = strnlen_user_nofault(uaddr, MAX_STRING_SIZE);
-	/*
-	 * strnlen_user_nofault returns zero on fault, insert the
-	 * FAULT_STRING when that occurs.
-	 */
-	if (ret <= 0)
-		return strlen(FAULT_STRING) + 1;
-	return ret;
+	return strnlen_user_nofault(uaddr, MAX_STRING_SIZE);
 }
 
 /* Return the length of string -- including null terminal byte */
@@ -44,18 +34,7 @@ fetch_store_strlen(unsigned long addr)
 		len++;
 	} while (c && ret == 0 && len < MAX_STRING_SIZE);
 
-	/* For faults, return enough to hold the FAULT_STRING */
-	return (ret < 0) ? strlen(FAULT_STRING) + 1 : len;
-}
-
-static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base, int len)
-{
-	if (ret >= 0) {
-		*(u32 *)dest = make_data_loc(ret, __dest - base);
-	} else {
-		strscpy(__dest, FAULT_STRING, len);
-		ret = strlen(__dest) + 1;
-	}
+	return (ret < 0) ? ret : len;
 }
 
 /*
@@ -76,7 +55,8 @@ 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);
-	set_data_loc(ret, dest, __dest, base, maxlen);
+	if (ret >= 0)
+		*(u32 *)dest = make_data_loc(ret, __dest - base);
 
 	return ret;
 }
@@ -107,7 +87,8 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
 	 * probing.
 	 */
 	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
-	set_data_loc(ret, dest, __dest, base, maxlen);
+	if (ret >= 0)
+		*(u32 *)dest = make_data_loc(ret, __dest - base);
 
 	return ret;
 }


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 5/5] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails
  2023-07-11 14:15 [PATCH v5 0/5] tracing/probes: Fix bugs in process_fetch_insn Masami Hiramatsu (Google)
                   ` (3 preceding siblings ...)
  2023-07-11 14:15 ` [PATCH v5 4/5] Revert "tracing: Add "(fault)" name injection to kernel probes" Masami Hiramatsu (Google)
@ 2023-07-11 14:16 ` Masami Hiramatsu (Google)
  2023-07-11 16:22   ` Steven Rostedt
  4 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu (Google) @ 2023-07-11 14:16 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML, Masami Hiramatsu

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Fix to record 0-length data to data_loc in fetch_store_string*() if it fails
to get the string data.
Currently those expect that the data_loc is updated by store_trace_args() if
it returns the error code. However, that does not work correctly if the
argument is an array of strings. In that case, store_trace_args() only clears
the first entry of the array (which may have no error) and leaves other
entries. So it should be cleared by fetch_store_string*() itself.
Also, 'dyndata' and 'maxlen' in store_trace_args() should be updated
only if it is used (ret > 0 and argument is a dynamic data.)

Fixes: 40b53b771806 ("tracing: probeevent: Add array type support")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v4:
  - Simplify the updating data_loc code with set_data_loc().
 Changes in v5:
  - Move out arg->dynamic check from unlikely() and use likely().
---
 kernel/trace/trace_probe_kernel.h |   13 +++++++++----
 kernel/trace/trace_probe_tmpl.h   |   10 +++-------
 kernel/trace/trace_uprobe.c       |    3 ++-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/trace_probe_kernel.h b/kernel/trace/trace_probe_kernel.h
index 6deae2ce34f8..bb723eefd7b7 100644
--- a/kernel/trace/trace_probe_kernel.h
+++ b/kernel/trace/trace_probe_kernel.h
@@ -37,6 +37,13 @@ fetch_store_strlen(unsigned long addr)
 	return (ret < 0) ? ret : len;
 }
 
+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);
+}
+
 /*
  * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf
  * with max length and relative data location.
@@ -55,8 +62,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);
+	set_data_loc(ret, dest, __dest, base);
 
 	return ret;
 }
@@ -87,8 +93,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);
+	set_data_loc(ret, dest, __dest, base);
 
 	return ret;
 }
diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 185da001f4c3..3935b347f874 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -267,13 +267,9 @@ 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 (arg->dynamic) {
-			if (unlikely(ret < 0)) {
-				*dl = make_data_loc(0, dyndata - base);
-			} else {
-				dyndata += ret;
-				maxlen -= ret;
-			}
+		if (arg->dynamic && likely(ret > 0)) {
+			dyndata += ret;
+			maxlen -= ret;
 		}
 	}
 }
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 8b92e34ff0c8..7b47e9a2c010 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -170,7 +170,8 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
 			 */
 			ret++;
 		*(u32 *)dest = make_data_loc(ret, (void *)dst - base);
-	}
+	} else
+		*(u32 *)dest = make_data_loc(0, (void *)dst - base);
 
 	return ret;
 }


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 4/5] Revert "tracing: Add "(fault)" name injection to kernel probes"
  2023-07-11 14:15 ` [PATCH v5 4/5] Revert "tracing: Add "(fault)" name injection to kernel probes" Masami Hiramatsu (Google)
@ 2023-07-11 16:19   ` Steven Rostedt
  0 siblings, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2023-07-11 16:19 UTC (permalink / raw)
  To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Tue, 11 Jul 2023 23:15:57 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> This reverts commit 2e9906f84fc7c99388bb7123ade167250d50f1c0.
> 
> It was turned out that commit 2e9906f84fc7 ("tracing: Add "(fault)"
> name injection to kernel probes") did not work correctly and probe
> events still show just '(fault)' (instead of '"(fault)"'). Also,
> current '(fault)' is more explicit that it faulted.
> 
> This also moves FAULT_STRING macro to trace.h so that synthetic
> event can keep using it, and uses it in trace_probe.c too.
> 
> Link: https://lore.kernel.org/all/20230706230642.3793a593@rorschach.local.home/
> 
> Cc: stable@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Tom Zanussi <zanussi@kernel.org>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 5/5] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails
  2023-07-11 14:16 ` [PATCH v5 5/5] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails Masami Hiramatsu (Google)
@ 2023-07-11 16:22   ` Steven Rostedt
  0 siblings, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2023-07-11 16:22 UTC (permalink / raw)
  To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Tue, 11 Jul 2023 23:16:07 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Fix to record 0-length data to data_loc in fetch_store_string*() if it fails
> to get the string data.
> Currently those expect that the data_loc is updated by store_trace_args() if
> it returns the error code. However, that does not work correctly if the
> argument is an array of strings. In that case, store_trace_args() only clears
> the first entry of the array (which may have no error) and leaves other
> entries. So it should be cleared by fetch_store_string*() itself.
> Also, 'dyndata' and 'maxlen' in store_trace_args() should be updated
> only if it is used (ret > 0 and argument is a dynamic data.)
> 
> Fixes: 40b53b771806 ("tracing: probeevent: Add array type support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v4:
>   - Simplify the updating data_loc code with set_data_loc().
>  Changes in v5:
>   - Move out arg->dynamic check from unlikely() and use likely().
> ---
>  kernel/trace/trace_probe_kernel.h |   13 +++++++++----
>  kernel/trace/trace_probe_tmpl.h   |   10 +++-------
>  kernel/trace/trace_uprobe.c       |    3 ++-
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 


Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 3/5] tracing/probes: Fix to update dynamic data counter if fetcharg uses it
  2023-07-11 14:15 ` [PATCH v5 3/5] tracing/probes: Fix to update dynamic data counter if fetcharg uses it Masami Hiramatsu (Google)
@ 2023-07-13 13:34   ` Steven Rostedt
  2023-07-13 14:34     ` Masami Hiramatsu
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2023-07-13 13:34 UTC (permalink / raw)
  To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Tue, 11 Jul 2023 23:15:48 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Fix to update dynamic data counter ('dyndata') and max length ('maxlen')
> only if the fetcharg uses the dynamic data. Also get out arg->dynamic
> from unlikely(). This makes dynamic data address wrong if
> process_fetch_insn() returns error on !arg->dynamic case.
> 
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Link: https://lore.kernel.org/all/20230710233400.5aaf024e@gandalf.local.home/
> Fixes: 9178412ddf5a ("tracing: probeevent: Return consumed bytes of dynamic area")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

> ---
>  kernel/trace/trace_probe_tmpl.h |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> index ed9d57c6b041..185da001f4c3 100644
> --- a/kernel/trace/trace_probe_tmpl.h
> +++ b/kernel/trace/trace_probe_tmpl.h
> @@ -267,11 +267,13 @@ 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 {
> -			dyndata += ret;
> -			maxlen -= ret;
> +		if (arg->dynamic) {
> +			if (unlikely(ret < 0)) {
> +				*dl = make_data_loc(0, dyndata - base);
> +			} else {
> +				dyndata += ret;
> +				maxlen -= ret;
> +			}
>  		}
>  	}
>  }


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 3/5] tracing/probes: Fix to update dynamic data counter if fetcharg uses it
  2023-07-13 13:34   ` Steven Rostedt
@ 2023-07-13 14:34     ` Masami Hiramatsu
  0 siblings, 0 replies; 10+ messages in thread
From: Masami Hiramatsu @ 2023-07-13 14:34 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel, LKML

On Thu, 13 Jul 2023 09:34:36 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 11 Jul 2023 23:15:48 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Fix to update dynamic data counter ('dyndata') and max length ('maxlen')
> > only if the fetcharg uses the dynamic data. Also get out arg->dynamic
> > from unlikely(). This makes dynamic data address wrong if
> > process_fetch_insn() returns error on !arg->dynamic case.
> > 
> > Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> > Link: https://lore.kernel.org/all/20230710233400.5aaf024e@gandalf.local.home/
> > Fixes: 9178412ddf5a ("tracing: probeevent: Return consumed bytes of dynamic area")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thank you! I'll pull this series. 

> 
> -- Steve
> 
> > ---
> >  kernel/trace/trace_probe_tmpl.h |   12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
> > index ed9d57c6b041..185da001f4c3 100644
> > --- a/kernel/trace/trace_probe_tmpl.h
> > +++ b/kernel/trace/trace_probe_tmpl.h
> > @@ -267,11 +267,13 @@ 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 {
> > -			dyndata += ret;
> > -			maxlen -= ret;
> > +		if (arg->dynamic) {
> > +			if (unlikely(ret < 0)) {
> > +				*dl = make_data_loc(0, dyndata - base);
> > +			} else {
> > +				dyndata += ret;
> > +				maxlen -= ret;
> > +			}
> >  		}
> >  	}
> >  }
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-07-13 14:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-11 14:15 [PATCH v5 0/5] tracing/probes: Fix bugs in process_fetch_insn Masami Hiramatsu (Google)
2023-07-11 14:15 ` [PATCH v5 1/5] tracing/probes: Fix to avoid double count of the string length on the array Masami Hiramatsu (Google)
2023-07-11 14:15 ` [PATCH v5 2/5] tracing/probes: Fix not to count error code to total length Masami Hiramatsu (Google)
2023-07-11 14:15 ` [PATCH v5 3/5] tracing/probes: Fix to update dynamic data counter if fetcharg uses it Masami Hiramatsu (Google)
2023-07-13 13:34   ` Steven Rostedt
2023-07-13 14:34     ` Masami Hiramatsu
2023-07-11 14:15 ` [PATCH v5 4/5] Revert "tracing: Add "(fault)" name injection to kernel probes" Masami Hiramatsu (Google)
2023-07-11 16:19   ` Steven Rostedt
2023-07-11 14:16 ` [PATCH v5 5/5] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails Masami Hiramatsu (Google)
2023-07-11 16:22   ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).