Re: [PATCH v2 bpf-next 01/18] btf: Extend UAPI to support BTF location (inline site) info

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 6250 bytes --]

> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index ddd0f4f32d24..a4412bc16688 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -559,7 +574,7 @@ struct btf_field_desc {
>  	/* member struct size, or zero, if no members */
>  	int m_sz;
>  	/* repeated per-member offsets */
> -	int m_off_cnt, m_offs[1];
> +	int m_off_cnt, m_offs[2];
>  };

Eduard Zingerman noted in the RFC v1 review that this change to m_offs[2]
should be moved to patch #2, since that patch's commit message explains
why it is needed. The change was acknowledged but not addressed in v2.

See: https://lore.kernel.org/bpf/531305ee76a5ef186b1204dc8281ebc7ebb2b1c0.camel@gmail.com/

> diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
> index 618167cab4e6..6062c9958034 100644
> --- a/include/uapi/linux/btf.h
> +++ b/include/uapi/linux/btf.h

[ ... ]

> +/*
> + * The combination of size, vlen and flags gives us the means to interpret
> + * the following vlen-specified set of 4-byte values:
> + *
> + * - a BTF_LOC_PARAM_CONST is a constant value; combination
> + *   of size, vlen and _SIGNED flag determines it. If the value requires
> + *   64 bits it is stored in {lo,hi} order.
> + * - a BTF_LOC_PARAM_ADDR is an address that will be normalized with
> + *   respect to kernel base address.
> + * - a BTF_LOC_PARAM_REG with vlen 1 is a simple register number;
> + *   with vlen 2 it is a multi-register parameter.
> + * - a _REG | DEREF with vlen 1 dereferences the value in the register
> + *   number specified.
> + * - a REG | DEREF | OFFSET with vlen specifies the register value in
> + *   the first 4-byte value and the offset in the remainder.
> + * - binary logical operators operate on a combination of register
> + *   number and constant value, aside from _NOT which operates on
> + *   a register
> + */
> +enum btf_loc_param_flags {
> +	BTF_LOC_PARAM_SIGNED		=	0x1,
> +	BTF_LOC_PARAM_CONST		=	0x2,
> +	BTF_LOC_PARAM_ADDR		=	0x4,
> +	BTF_LOC_PARAM_REG		=	0x8,
> +	BTF_LOC_PARAM_DEREF		=	0x10,
> +	BTF_LOC_PARAM_OFFSET		=	0x20,
> +};

Does the comment above the enum match the actual flags? The text mentions
"binary logical operators" and "_NOT which operates on a register," but
the enum defines only SIGNED, CONST, ADDR, REG, DEREF and OFFSET. Where
is BTF_LOC_PARAM_NOT and where are the logical operator flags?

This same mismatch appears in tools/include/uapi/linux/btf.h as well.

> +/*
> + * BTF_KIND_LOC_PROTO specifies location prototypes; i.e. how locations relate
> + * to parameters; a struct btf_type of BTF_KIND_LOC_PROTO is followed by a
> + * a vlen-specified number of __u32 BTF type ids which specify the associated
> + * BTF_KIND_LOC_PARAM for each function parameter associated with the
> + * location.  The type should either be 0 (no location info) or point at
> + * a BTF_KIND_LOC_PARAM.
> + */

This isn't a bug, but there's a duplicated word: "followed by a / a
vlen-specified number". The same duplication appears in the tools copy at
tools/include/uapi/linux/btf.h.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 9c2cab08bb79..d74c8668aa3f 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c

[ ... ]

> +static s32 btf_loc_param_check_meta(struct btf_verifier_env *env,
> +				    const struct btf_type *t,
> +				    u32 meta_left)
> +{
> +	const struct btf_loc_param *p = btf_loc_param(t);
> +	u32 meta_needed;
> +	u32 size;
> +
> +	meta_needed = sizeof(*p) + sizeof(__u32) * btf_vlen(t);
> +	if (meta_left < meta_needed) {
> +		btf_verifier_log_basic(env, t,
> +				       "meta_left:%u meta_needed:%u",
> +				      meta_left, meta_needed);
> +		return -EINVAL;
> +	}
> +
> +	if (t->name_off) {
> +		btf_verifier_log_type(env, t, "Invalid name");
> +		return -EINVAL;
> +	}
> +	size = t->size;
> +	if (size > 16 || !is_power_of_2(size)) {
> +		btf_verifier_log_type(env, t, "Unexpected size");
> +		return -EINVAL;
> +	}
> +
> +	return meta_needed;
> +}

Does this handler miss the trailing btf_verifier_log_type(env, t, NULL)
call before returning successfully? Looking at __btf_verifier_log_type()
in this file, the comment states: "btf verifier prints all types it is
processing via btf_verifier_log_type(..., fmt = NULL)". Every other
check_meta handler in kernel/bpf/btf.c ends with that call on the
success path (int, ptr, array, struct, enum, enum64, fwd, typedef,
func_proto, func, var, datasec, decl_tag, float), but
btf_loc_param_check_meta, btf_loc_proto_check_meta, and
btf_locsec_check_meta all return successfully without it.

When userspace passes a log buffer to BPF_BTF_LOAD, wouldn't the per-type
"[id] KIND name ..." trace skip every LOC_PARAM/LOC_PROTO/LOCSEC type,
creating gaps in the [id] sequence? Since env->log_type_id advances
inside btf_verifier_log_type() and not on the successful path here, could
later error messages for subsequent types be attributed to the wrong id?

> +static void btf_locsec_log(struct btf_verifier_env *env,
> +			   const struct btf_type *t)
> +{
> +	const struct btf_loc *loc = btf_type_loc_secinfo(t);
> +	u32 nr_locs = btf_type_vlen(t), i;
> +	const struct btf *btf = env->btf;
> +
> +	btf_verifier_log(env, "locsec %s locs=(",
> +			 __btf_name_by_offset(btf, t->name_off));
> +	for (i = 0; i < nr_locs; i++, loc++) {
> +		btf_verifier_log(env, "\n\tfunc %u loc_proto %u offset 0x%x%s",
> +				 loc->func, loc->loc_proto, loc->offset,
> +				 i + 1 == nr_locs ? ")" : ", ");
> +	}
> +}

A subsystem pattern flags this as potentially concerning: the log_details
callback re-prints the kind and name that __btf_verifier_log_type() has
already emitted, so the output would read "[7] LOCSEC name locsec name
locs=(...)". Every other log_details handler (btf_datasec_log,
btf_struct_log, etc.) prints only the kind-specific tail.

Also, if vlen is zero, does the loop body that would print the closing
parenthesis ever run? A zero-vlen LOCSEC would pass check_meta and leave
a dangling "(" in the log. The same pattern appears in btf_loc_proto_log
and btf_loc_param_log.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33537080133