Re: [RFC PATCH bpf-next 04/12] bpf: Create insn_array map for bpf SDT probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Size of struct bpf_insn_array_value changes, breaking UAPI backwards compatibility for BPF_MAP_TYPE_INSN_ARRAY map creation.
- [High] SDT probes in shared subprograms are dropped for all but the first main program that absorbs the subprogram.
- [High] Incorrect instruction offset calculation for SDT probes located in subprograms, leading to offset corruption and verifier rejection.
--

commit 4c79777e0549d710e07ecb99bf27d3c4bb18d256
Author: Xu Kuohai <xukuohai@huawei.com>

bpf: Create insn_array map for bpf SDT probe

Create one BPF_MAP_TYPE_INSN_ARRAY map per program from the parsed
SDT entries. Each map is tagged with BPF_F_INSN_ARRAY_SDT and
populated with the instruction offset, argument count, and per-argument
registers.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
[ ... ]
> @@ -7732,7 +7739,9 @@ struct bpf_insn_array_value {
>  	__u32 orig_off;
>  	__u32 xlated_off;
>  	__u32 jitted_off;
> -	__u32 :32;
> +	__u8  nargs;		/* argument count (0..5) */
> +	__u8  arg_reg[5];	/* BPF register for each argument */
> +	__u8  pad[2];
>  };

[Severity: High]
Does this struct size change break UAPI backwards compatibility?

Because insn_array_alloc_check() unconditionally enforces a strict size check:

kernel/bpf/bpf_insn_array.c:insn_array_alloc_check() {
    u32 value_size = sizeof(struct bpf_insn_array_value);
    
    if (attr->max_entries == 0 || attr->key_size != 4 ||
        attr->value_size != value_size ||
        attr->map_flags & ~BPF_F_INSN_ARRAY_SDT)
        return -EINVAL;
}

Older userspace binaries calling bpf_map_create() with the old value_size
will be rejected with -EINVAL.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
[ ... ]
> @@ -6686,6 +6692,131 @@ static int create_jt_map(struct bpf_object *obj, struct bpf_program *prog, struc
>  	return err;
>  }
>  
> +static int bpf_object__resolve_sdt_progs(struct bpf_object *obj)
> +{
> +	int i, j;
> +	struct bpf_light_subprog *sp;
> +
> +	for (i = 0; i < obj->sdt_entry_cnt; i++) {
> +		struct sdt_entry *e = &obj->sdt_entries[i];
> +		struct bpf_program *prog, *subprog;
> +
> +		prog = find_prog_by_sec_insn(obj, e->sec_idx, e->insn_idx);
[ ... ]
> +		/* resolve subprog probe in the main prog that absorbed the subprog */
> +		if (prog_is_subprog(obj, prog) && prog->absorbed_by != -1) {
> +			subprog = prog;
> +			prog = &obj->programs[prog->absorbed_by];

[Severity: High]
Does this logic drop SDT probes in shared subprograms for all but the first
main program that absorbed it?

By resolving the shared SDT entry exclusively to the main program indicated
by prog->absorbed_by, any tracing tools relying on SDT probes in this
subprogram will silently miss executions when the subprogram is executed
through alternate main programs.

> +			for (j = 0; j < prog->subprog_cnt; j++) {
> +				sp = &prog->subprogs[j];
> +				if (sp->sec_insn_off == subprog->sec_insn_off) {
> +					e->insn_idx = sp->sub_insn_off +
> +						       (e->insn_idx - sp->sec_insn_off);
> +					break;
> +				}
> +			}
[ ... ]
> +static int bpf_object__create_sdt_maps(struct bpf_object *obj)
> +{
[ ... ]
> +	/* create insn_array maps per program and populate entries */
> +	for (i = 0; i < obj->sdt_entry_cnt; i++) {
> +		__u32 key;
> +		struct sdt_entry *e = &obj->sdt_entries[i];
> +		__u32 sdt_cnt = prog_sdt[e->prog_idx].sdt_cnt;
> +
> +		if (!sdt_cnt)
> +			continue;
> +
> +		prog = &obj->programs[e->prog_idx];
[ ... ]
> +		key = prog_sdt[e->prog_idx].next_key++;
> +
> +		memset(&val, 0, sizeof(val));
> +		val.nargs = e->nargs;
> +		val.orig_off = e->insn_idx - prog->sec_insn_off;

[Severity: High]
Can this offset calculation lead to verifier rejection later?

Earlier in bpf_object__resolve_sdt_progs(), e->insn_idx is updated to a 0-based
local offset relative to the main program.

Here in bpf_object__create_sdt_maps(), subtracting prog->sec_insn_off again
from an already localized offset will cause an integer underflow or offset
corruption if the main program's section offset is greater than 0. This will
cause the verifier to reject the BPF object.

> +		memcpy(val.arg_reg, e->arg_reg, sizeof(val.arg_reg));
> +
> +		err = bpf_map_update_elem(prog->sdt_map_fd, &key, &val, 0);
> +		if (err)
> +			goto out_free;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782571533.git.xukuohai@huawei.com?part=4