diff mbox series

[3/8] tracing: fprobe-events: Log error for exceeding the number of entry args

Message ID 174055074269.4079315.17809232650360988538.stgit@mhiramat.tok.corp.google.com (mailing list archive)
State Queued
Commit db5e228611b118cf7b1f8084063feda5c037f4a7
Headers show
Series tracing: probes: Fixes and enhancing error logs | expand

Commit Message

Masami Hiramatsu (Google) Feb. 26, 2025, 6:19 a.m. UTC 
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add error message when the number of entry argument exceeds the
maximum size of entry data.
This is currently checked when registering fprobe, but in this case
no error message is shown in the error_log file.

Fixes: 25f00e40ce79 ("tracing/probes: Support $argN in return probe (kprobe and fprobe)")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_fprobe.c |    5 +++++
 kernel/trace/trace_probe.h  |    3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Steven Rostedt Feb. 26, 2025, 3:22 p.m. UTC | #1 
On Wed, 26 Feb 2025 15:19:02 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> index 85f037dc1462..e27305d31fc5 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -1230,6 +1230,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	if (is_return && tf->tp.entry_arg) {
>  		tf->fp.entry_handler = trace_fprobe_entry_handler;
>  		tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);

Looking at traceprobe_get_entry_data_size(), the setting of the offset and
what it returns is a bit of magic. It's not intuitive and really could use
some comments. This isn't against this patch, but it does make it hard to
review this patch.

> +		if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
> +			trace_probe_log_set_index(2);
> +			trace_probe_log_err(0, TOO_MANY_EARGS);
> +			return -E2BIG;
> +		}
>  	}
>  
>  	ret = traceprobe_set_print_fmt(&tf->tp,


We have this in trace_probe.c:

static int __store_entry_arg(struct trace_probe *tp, int argnum)
{
	struct probe_entry_arg *earg = tp->entry_arg;
	bool match = false;
	int i, offset;

	if (!earg) {
		earg = kzalloc(sizeof(*tp->entry_arg), GFP_KERNEL);
		if (!earg)
			return -ENOMEM;
		earg->size = 2 * tp->nr_args + 1;
		earg->code = kcalloc(earg->size, sizeof(struct fetch_insn),
				     GFP_KERNEL);
		if (!earg->code) {
			kfree(earg);
			return -ENOMEM;
		}
		/* Fill the code buffer with 'end' to simplify it */
		for (i = 0; i < earg->size; i++)
			earg->code[i].op = FETCH_OP_END;
		tp->entry_arg = earg;
	}

	offset = 0;
	for (i = 0; i < earg->size - 1; i++) {
		switch (earg->code[i].op) {
		case FETCH_OP_END:
			earg->code[i].op = FETCH_OP_ARG;
			earg->code[i].param = argnum;
			earg->code[i + 1].op = FETCH_OP_ST_EDATA;
			earg->code[i + 1].offset = offset;

// There's definitely some dependency between FETCH_OP_END, FETCH_OP_ARG
// and FETCH_OP_ST_EDATA that isn't documented. At least not in this file.

			return offset;
		case FETCH_OP_ARG:
			match = (earg->code[i].param == argnum);
			break;
		case FETCH_OP_ST_EDATA:
			offset = earg->code[i].offset;
			if (match)
				return offset;
			offset += sizeof(unsigned long);
			break;
		default:
			break;
		}
	}
	return -ENOSPC;
}

int traceprobe_get_entry_data_size(struct trace_probe *tp)
{
	struct probe_entry_arg *earg = tp->entry_arg;
	int i, size = 0;

	if (!earg)
		return 0;

	for (i = 0; i < earg->size; i++) {
		switch (earg->code[i].op) {
		case FETCH_OP_END:
			goto out;
		case FETCH_OP_ST_EDATA:
			size = earg->code[i].offset + sizeof(unsigned long);

// What makes earg->code[i].offset so special?
// What is the guarantee that code[i + 1].offset is greater than code[i].offset?
// I guess the above function guarantees that, but it's not obvious here.

			break;
		default:
			break;
		}
	}
out:
	return size;
}

Assuming that traceprobe_get_entry_data_size() does properly return the max size.

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

-- Steve
Masami Hiramatsu (Google) Feb. 26, 2025, 10:10 p.m. UTC | #2 
On Wed, 26 Feb 2025 10:22:23 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 26 Feb 2025 15:19:02 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > index 85f037dc1462..e27305d31fc5 100644
> > --- a/kernel/trace/trace_fprobe.c
> > +++ b/kernel/trace/trace_fprobe.c
> > @@ -1230,6 +1230,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  	if (is_return && tf->tp.entry_arg) {
> >  		tf->fp.entry_handler = trace_fprobe_entry_handler;
> >  		tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);
> 
> Looking at traceprobe_get_entry_data_size(), the setting of the offset and
> what it returns is a bit of magic. It's not intuitive and really could use
> some comments. This isn't against this patch, but it does make it hard to
> review this patch.

Indeed. It is a bit tricky.

> 
> > +		if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
> > +			trace_probe_log_set_index(2);
> > +			trace_probe_log_err(0, TOO_MANY_EARGS);
> > +			return -E2BIG;
> > +		}
> >  	}
> >  
> >  	ret = traceprobe_set_print_fmt(&tf->tp,
> 
> 
> We have this in trace_probe.c:
> 
> static int __store_entry_arg(struct trace_probe *tp, int argnum)
> {
> 	struct probe_entry_arg *earg = tp->entry_arg;
> 	bool match = false;
> 	int i, offset;
> 
> 	if (!earg) {
> 		earg = kzalloc(sizeof(*tp->entry_arg), GFP_KERNEL);
> 		if (!earg)
> 			return -ENOMEM;
> 		earg->size = 2 * tp->nr_args + 1;
> 		earg->code = kcalloc(earg->size, sizeof(struct fetch_insn),
> 				     GFP_KERNEL);
> 		if (!earg->code) {
> 			kfree(earg);
> 			return -ENOMEM;
> 		}
> 		/* Fill the code buffer with 'end' to simplify it */
> 		for (i = 0; i < earg->size; i++)
> 			earg->code[i].op = FETCH_OP_END;
> 		tp->entry_arg = earg;
> 	}
> 
> 	offset = 0;
> 	for (i = 0; i < earg->size - 1; i++) {
> 		switch (earg->code[i].op) {
> 		case FETCH_OP_END:
> 			earg->code[i].op = FETCH_OP_ARG;
> 			earg->code[i].param = argnum;
> 			earg->code[i + 1].op = FETCH_OP_ST_EDATA;
> 			earg->code[i + 1].offset = offset;
> 
> // There's definitely some dependency between FETCH_OP_END, FETCH_OP_ARG
> // and FETCH_OP_ST_EDATA that isn't documented. At least not in this file.

This accumurates the fetching operation on the code. The problem is
limitation of the entry data size. So this scans the entry code and
checks whether the specified function argument is already stored, and
reuse it (return the offset where it is stored). If there isn't,
it stores a pair of FETCH_OP_ARG + FETCH_OP_ST_EDATA at the end of
the code array.

> 
> 			return offset;
> 		case FETCH_OP_ARG:
> 			match = (earg->code[i].param == argnum);
> 			break;
> 		case FETCH_OP_ST_EDATA:
> 			offset = earg->code[i].offset;
> 			if (match)
> 				return offset;
> 			offset += sizeof(unsigned long);
> 			break;
> 		default:
> 			break;
> 		}
> 	}
> 	return -ENOSPC;
> }
> 
> int traceprobe_get_entry_data_size(struct trace_probe *tp)
> {
> 	struct probe_entry_arg *earg = tp->entry_arg;
> 	int i, size = 0;
> 
> 	if (!earg)
> 		return 0;
> 
> 	for (i = 0; i < earg->size; i++) {
> 		switch (earg->code[i].op) {
> 		case FETCH_OP_END:
> 			goto out;
> 		case FETCH_OP_ST_EDATA:
> 			size = earg->code[i].offset + sizeof(unsigned long);
> 
> // What makes earg->code[i].offset so special?
> // What is the guarantee that code[i + 1].offset is greater than code[i].offset?
> // I guess the above function guarantees that, but it's not obvious here.

Yeah, let me explain.

	/*
	 * earg->code[] array has an operation sequence which is run in
	 * the entry handler.
	 * The sequence stopped by FETCH_OP_END and each data stored in
	 * the entry data buffer by FETCH_OP_ST_EDATA. The FETCH_OP_ST_EDATA
	 * stores the data at the data buffer + its offset, and all data are
	 * "unsigned long" size. The offset must be increased when a data is
	 * stored. Thus we need to find the last FETCH_OP_ST_EDATA in the
	 * code array.
	 */

> 
> 			break;
> 		default:
> 			break;
> 		}
> 	}
> out:
> 	return size;
> }
> 
> Assuming that traceprobe_get_entry_data_size() does properly return the max size.

Yes, maybe we can scan the code array from the end for optimization but it still
needs the explanation.


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

Thank you!

> 
> -- Steve
diff mbox series

Patch

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 85f037dc1462..e27305d31fc5 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -1230,6 +1230,11 @@  static int trace_fprobe_create_internal(int argc, const char *argv[],
 	if (is_return && tf->tp.entry_arg) {
 		tf->fp.entry_handler = trace_fprobe_entry_handler;
 		tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);
+		if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
+			trace_probe_log_set_index(2);
+			trace_probe_log_err(0, TOO_MANY_EARGS);
+			return -E2BIG;
+		}
 	}
 
 	ret = traceprobe_set_print_fmt(&tf->tp,
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index fba3ede87054..c47ca002347a 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -545,7 +545,8 @@  extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(NO_BTF_FIELD,		"This field is not found."),	\
 	C(BAD_BTF_TID,		"Failed to get BTF type info."),\
 	C(BAD_TYPE4STR,		"This type does not fit for string."),\
-	C(NEED_STRING_TYPE,	"$comm and immediate-string only accepts string type"),
+	C(NEED_STRING_TYPE,	"$comm and immediate-string only accepts string type"),\
+	C(TOO_MANY_EARGS,	"Too many entry arguments specified"),
 
 #undef C
 #define C(a, b)		TP_ERR_##a