Re: [PATCH v2] tracing/probes: Allow use of BTF names to dereference pointers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Uninitialized variable `btf` in `btf_find_offset()` causes a kernel crash/panic on error paths.
- [Medium] Nested structure resolution fails if members use BTF modifiers (const, volatile) or typedefs.
- [Medium] The updated user-space dereference parser misidentifies valid BTF struct accesses and breaks negative user-space offsets.
- [Low] Missing error mapping for `-ENOENT` suppresses trace log errors when a struct member is not found.
- [Low] The `-E2BIG` error for excessively deep anonymous structures is silently dropped, creating dead code in the error handler.
--

commit 59c817c69d1d406c3cfa1e1e3659ff16a7e33c9d
Author: Steven Rostedt <rostedt@goodmis.org>
Subject: tracing/probes: Allow use of BTF names to dereference pointers

This patch adds syntax to the FETCHARGS parsing of probes to allow the use
of structure and member names to get the offsets to dereference pointers,
using BTF.

> diff --git a/kernel/trace/trace_btf.c b/kernel/trace/trace_btf.c
> index 00172f301f25a..be88cc4d97dd3 100644
> --- a/kernel/trace/trace_btf.c
> +++ b/kernel/trace/trace_btf.c

[ ... ]

> +static int find_member(const char *ptr, struct btf *btf,
> +		       const struct btf_type **type, int level)
> +{

[ ... ]

> +		/* Handle anonymous structures */
> +		if (strlen(tname))
> +			continue;
> +
> +		*type = btf_type_by_id(btf, member->type);
> +		if (btf_type_is_struct(*type)) {
> +			int offset = find_member(ptr, btf, type, level + 1);
> +
> +			if (offset < 0)
> +				continue;

If find_member() returns -E2BIG due to deep anonymous structure nesting,
this check appears to swallow the error and just skip to the next member.
Does this prevent -E2BIG from ever reaching the caller and make the
TP_ERR_MEMBER_TOO_DEEP error handling dead code?

> +
> +			return offset + BITS_ROUNDDOWN_BYTES(member->offset);
> +		}
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +/**
> + * btf_find_offset - Find an offset of a member for a structure

[ ... ]

> +int btf_find_offset(char *arg, long *offset_p)
> +{
> +	const struct btf_type *t;
> +	struct btf *btf;
> +	long offset = 0;
> +	char *ptr;
> +	int ret;
> +	s32 id;
> +
> +	ptr = strchr(arg, '.');
> +	if (!ptr)
> +		return -EINVAL;
> +
> +	*ptr = '\0';
> +
> +	ret = -ENXIO;
> +	id = bpf_find_btf_id(arg, BTF_KIND_STRUCT, &btf);
> +	if (id < 0)
> +		goto error;

If bpf_find_btf_id() fails here, it returns an error code and does not
initialize the btf pointer.

Does this cause btf_put(btf) to dereference an uninitialized stack pointer
in the error path below, potentially leading to a panic?

> +
> +	/* Get BTF_KIND_FUNC type */
> +	t = btf_type_by_id(btf, id);
> +
> +	/* May allow more than one member, as long as they are structures */
> +	do {
> +		ret = -ENXIO;
> +		if (!t || !btf_type_is_struct(t))
> +			goto error;

If a structure member is a typedef or has a modifier (like const), the
updated type from find_member() will be BTF_KIND_TYPEDEF or BTF_KIND_CONST.

Will btf_type_is_struct() then fail on the next iteration of this loop?
Should btf_type_skip_modifiers() be used to resolve the underlying structure
type first?

[ ... ]

> +error:
> +	btf_put(btf);
> +	if (ptr)
> +		*ptr = '.';
> +	return ret;
> +}

[ ... ]

> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index e0d3a0da26af5..6fcede2de1a55 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -1165,7 +1165,7 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
>  
>  	case '+':	/* deref memory */
>  	case '-':
> -		if (arg[1] == 'u') {
> +		if (arg[1] == 'u' && isdigit(arg[2])) {

Does this new condition misidentify valid BTF structure lookups that happen
to start with 'u' followed by a digit, such as +u16_port.id($arg)?

Also, does it break previously valid negative user-space offsets like
+u-10($arg) since arg[2] would be a hyphen instead of a digit?

>  			deref = FETCH_OP_UDEREF;
>  			arg[1] = arg[0];
>  			arg++;
> @@ -1178,7 +1178,22 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
>  			return -EINVAL;
>  		}
>  		*tmp = '\0';
> -		ret = kstrtol(arg, 0, &offset);
> +		if (arg[0] != '-' && !isdigit(*arg)) {
> +			int err = 0;
> +			ret = btf_find_offset(arg, &offset);
> +			switch (ret) {
> +			case -ENODEV: err = TP_ERR_NOSUP_BTFARG; break;
> +			case -E2BIG: err = TP_ERR_MEMBER_TOO_DEEP; break;
> +			case -EINVAL: err = TP_ERR_BAD_STRUCT_FMT; break;
> +			case -ENXIO: err = TP_ERR_BAD_BTF_TID; break;
> +			}

If find_member() cannot find the specified member, it returns -ENOENT.
Since -ENOENT is missing from this switch statement, err remains 0 and
__trace_probe_log_err() is skipped.

Should there be a mapping for -ENOENT (perhaps to TP_ERR_NO_BTF_FIELD) so
users get a specific error log when they misspell a field name?

> +			if (err)
> +				__trace_probe_log_err(ctx->offset, err);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			ret = kstrtol(arg, 0, &offset);
> +		}
>  		if (ret) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516173310.1dbad146@fedora?part=1