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

[Steven Rostedt <rostedt@xxxxxxxxxxx>]

On Wed, 26 Feb 2025 15:19:02 +0900
"Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> 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@xxxxxxxxxxx>

-- Steve