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

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

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


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