Skip to content

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: Support associating BPF programs with struct_ops
version: 3
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013078

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 48a97ff
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013078
version: 3

@kernel-patches-review-bot
Copy link

commit 9f7f2c4fade809ef3a54439c557d850013600265
Author: Amery Hung <[email protected]>

libbpf: Add support for associating BPF program with struct_ops

Add low-level wrapper and libbpf API for BPF_PROG_ASSOC_STRUCT_OPS
command in the bpf() syscall.

> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 339b19797..885b0f891 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -1397,3 +1397,22 @@ int bpf_prog_stream_read(int prog_fd, __u32 stream_id, void *buf, __u32 buf_len,
>  	err = sys_bpf(BPF_PROG_STREAM_READ_BY_FD, &attr, attr_sz);
>  	return libbpf_err_errno(err);
>  }
> +
> +int bpf_prog_assoc_struct_ops(int prog_fd, int map_fd,
> +			      struct bpf_prog_assoc_struct_ops_opts *opts)
> +{
> +	const size_t attr_sz = offsetofend(union bpf_attr, prog_assoc_struct_ops);
> +	union bpf_attr attr;
> +	int err;
> +
> +	if (!OPTS_VALID(opts, bpf_prog_assoc_struct_ops_opts))
> +		return libbpf_err(-EINVAL);
> +
> +	memset(&attr, 0, attr_sz);
> +	attr.prog_assoc_struct_ops.map_fd = map_fd;
> +	attr.prog_assoc_struct_ops.prog_fd = prog_fd;
> +	attr.prog_assoc_struct_ops.flags = OPTS_GET(opts, flags, 0);
> +
> +	err = sys_bpf(BPF_PROG_ASSOC_STRUCT_OPS, &attr, attr_sz);
> +	return libbpf_err_errno(err);
> +}

[ ... ]

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index dd3b2f570..7e2098970 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -13891,6 +13891,36 @@ int bpf_program__set_attach_target(struct bpf_program *prog,
>  	return 0;
>  }
>
> +int bpf_program__assoc_struct_ops(struct bpf_program *prog, struct bpf_map *map,
> +				  struct bpf_prog_assoc_struct_ops_opts *opts)
> +{
> +	int prog_fd;
> +
> +	prog_fd = bpf_program__fd(prog);
> +	if (prog_fd < 0) {
> +		pr_warn("prog '%s': can't associate BPF program without FD (was it loaded?)\n",
> +			prog->name);
                        ^^^^^^^^^^

Can prog be NULL here? If bpf_program__assoc_struct_ops() is called with
prog==NULL, then bpf_program__fd() will return -EINVAL, but prog->name
will NULL dereference in the error message. Similar functions like
bpf_program__set_attach_target() check for NULL prog at the start.

> +		return -EINVAL;
> +	}
> +
> +	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> +		pr_warn("prog '%s': can't associate struct_ops program\n", prog->name);
> +		return -EINVAL;
> +	}
> +
> +	if (map->fd < 0) {
> +		pr_warn("map '%s': can't associate BPF map without FD (was it created?)\n", map->name);
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can map be NULL here? If map is NULL, map->fd and map->name will NULL
dereference.

> +		return -EINVAL;
> +	}
> +
> +	if (!bpf_map__is_struct_ops(map)) {
> +		pr_warn("map '%s': can't associate non-struct_ops map\n", map->name);
> +		return -EINVAL;
> +	}
> +
> +	return bpf_prog_assoc_struct_ops(prog_fd, map->fd, opts);
> +}

[ ... ]



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

In-Reply-To-Subject: libbpf: Add support for associating BPF program with struct_ops
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18605893586

@kernel-patches-review-bot
Copy link

commit 477ea85cb885c46109ac868f354b3c74eb15d9bc
Author: Amery Hung <[email protected]>

bpf: Allow verifier to fixup kernel module kfuncs

This patch removes the early return check for insn->off in
fixup_kfunc_call(), allowing kernel module kfuncs to be processed
through the special kfunc fixup logic that was previously only applied
to kernel kfuncs.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c908015b2..b8387dedd 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -21917,8 +21917,7 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>
>  	if (!bpf_jit_supports_far_kfunc_call())
>  		insn->imm = BPF_CALL_IMM(desc->addr);
> -	if (insn->off)
> -		return 0;
> +
>  	if (desc->func_id == special_kfunc_list[KF_bpf_obj_new_impl] ||
>  	    desc->func_id == special_kfunc_list[KF_bpf_percpu_obj_new_impl]) {
>  		struct btf_struct_meta *kptr_struct_meta = env->insn_aux_data[insn_idx].kptr_struct_meta;

Can this comparison match incorrectly for module kfuncs? The
special_kfunc_list array contains BTF IDs from the kernel's vmlinux BTF.
When a kernel module kfunc has a BTF ID (within its own module BTF) that
numerically equals one of these special kernel kfunc IDs, the comparison
desc->func_id == special_kfunc_list[KF_xxx] will match even though
they're different functions from different BTF objects.

During verification in check_special_kfunc() (line 13648), there's a
check "if (meta->btf != btf_vmlinux) return 0;" that prevents special
kfunc handling for module kfuncs. This means insn_aux_data fields like
kptr_struct_meta, obj_new_size, and arg_prog won't be populated for
module kfuncs. But in fixup_kfunc_call(), after removing the insn->off
check, a module kfunc with a colliding BTF ID would enter the special
kfunc fixup branches and use these uninitialized fields to generate
incorrect instructions.

In check_kfunc_call() at line 13846, similar special kfunc comparisons
are protected by checking "!insn->off &&" before comparing against
special_kfunc_list. Should fixup_kfunc_call() have the same protection,
checking both desc->func_id and desc->offset (or equivalently insn->off)
before applying special kfunc fixups?



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

In-Reply-To-Subject: bpf: Allow verifier to fixup kernel module kfuncs
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18605893586

@kernel-patches-daemon-bpf
Copy link
Author

Forwarding comment 3417395855 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

@kernel-patches-daemon-bpf
Copy link
Author

Forwarding comment 3417400436 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 48a97ff
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013078
version: 3

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 50de48a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013078
version: 3

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 4f8543b
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013078
version: 3

Allow verifier to fixup kfuncs in kernel module to support kfuncs with
__prog arguments. Currently, special kfuncs and kfuncs with __prog
arguments are kernel kfuncs. Allowing kernel module kfuncs should not
affect existing kfunc fixup as kernel module kfuncs have BTF IDs greater
than kernel kfuncs' BTF IDs.

Signed-off-by: Amery Hung <[email protected]>
Add a new BPF command BPF_PROG_ASSOC_STRUCT_OPS to allow associating
a BPF program with a struct_ops map. This command takes a file
descriptor of a struct_ops map and a BPF program and set
prog->aux->st_ops_assoc to the kdata of the struct_ops map.

The command does not accept a struct_ops program or a non-struct_ops
map. Programs of a struct_ops map is automatically associated with the
map during map update. If a program is shared between two struct_ops
maps, prog->aux->st_ops_assoc will be poisoned to indicate that the
associated struct_ops is ambiguous. The pointer, once poisoned, cannot
be reset since we have lost track of associated struct_ops. For other
program types, the associated struct_ops map, once set, cannot be
changed later. This restriction may be lifted in the future if there is
a use case.

A kernel helper bpf_prog_get_assoc_struct_ops() can be used to retrieve
the associated struct_ops pointer. The pointer returned, if not NULL, is
guaranteed to be valid and point to a fully updated struct_ops struct.
This is done by increasing the refcount of the map for every associated
non-struct_ops programs. For struct_ops program reused in multiple
struct_ops map, the return will be NULL. struct_ops implementers should
note that the struct_ops returned may or may not be attached. The
struct_ops implementer will be responsible for tracking and checking the
state of the associated struct_ops map if the use case requires an
attached struct_ops.

Signed-off-by: Amery Hung <[email protected]>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 7361c86
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013078
version: 3

Add low-level wrapper and libbpf API for BPF_PROG_ASSOC_STRUCT_OPS
command in the bpf() syscall.

Signed-off-by: Amery Hung <[email protected]>
Test BPF_PROG_ASSOC_STRUCT_OPS command that associates a BPF program
with a struct_ops. The test follows the same logic in commit
ba7000f ("selftests/bpf: Test multi_st_ops and calling kfuncs from
different programs"), but instead of using map id to identify a specific
struct_ops, this test uses the new BPF command to associate a struct_ops
with a program.

The test consists of two sets of almost identical struct_ops maps and BPF
programs associated with the map. Their only difference is the unique
value returned by bpf_testmod_multi_st_ops::test_1().

The test first loads the programs and associates them with struct_ops
maps. Then, it exercises the BPF programs. They will in turn call kfunc
bpf_kfunc_multi_st_ops_test_1_prog_arg() to trigger test_1() of the
associated struct_ops map, and then check if the right unique value is
returned.

Signed-off-by: Amery Hung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant