On Wed, 26 Feb 2025 15:19:02 +0900
"Masami Hiramatsu (Google)" <[email protected]> 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) <[email protected]>
-- Steve
