Support associating BPF programs with struct_ops

[kernel-patches-daemon-bpf]

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

[kernel-patches-daemon-bpf]

Upstream branch: 63066b7
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1023753
version: 6

[kernel-patches-review-bot]

commit e9e55f037b0cc696de049ac31b22dcf271301b5e
Author: Amery Hung <[email protected]>

bpf: Support associating BPF program with struct_ops

This patch adds a new BPF command BPF_PROG_ASSOC_STRUCT_OPS to associate
a BPF program with a struct_ops map, allowing programs to query their
associated struct_ops at runtime via bpf_prog_get_assoc_struct_ops().

> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index a41e6730e..626aa9197 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c

[ ... ]

> @@ -811,6 +822,12 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>  		bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS,
>  			      &bpf_struct_ops_link_lops, prog, prog->expected_attach_type);
>  		*plink++ = &link->link;
> +
> +		err = bpf_prog_assoc_struct_ops(prog, &st_map->map);
> +		if (err) {
> +			bpf_prog_put(prog);
> +			goto reset_unlock;
> +		}

Can this double-put the prog reference? In bpf_struct_ops_map_update_elem:

  prog = bpf_prog_get(prog_fd);              // refcnt = 1
  bpf_link_init(..., prog, ...);             // link takes ownership
  *plink++ = &link->link;                    // link stored in st_map
  err = bpf_prog_assoc_struct_ops(prog, ...);
  if (err) {
    bpf_prog_put(prog);                      // refcnt = 0, prog freed
    goto reset_unlock;
  }

Then at reset_unlock:

  bpf_struct_ops_map_put_progs(st_map);      // calls bpf_link_put(link)

The bpf_link_put will eventually call bpf_link_dealloc()->bpf_prog_put(link->prog),
attempting to put the already-freed prog reference. This looks like a use-after-free
if bpf_prog_assoc_struct_ops fails (which can happen when a non-struct_ops program
is already associated and returns -EBUSY).

Should the error path skip the bpf_prog_put and let bpf_struct_ops_map_put_progs
handle the cleanup via the link?

---

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: Support associating BPF program with struct_ops
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19379369447

[kernel-patches-daemon-bpf]

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

[kernel-patches-review-bot]

commit bae632c516e48ee2f7f855fe2591bfb856c3192c
Author: Amery Hung <[email protected]>

libbpf: Add support for associating BPF program with struct_ops

This commit adds 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 b66f5fbfb..21b57a629 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c

[ ... ]

> +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 706e7481b..1d5424276 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> +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, map_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 this crash if prog is NULL? bpf_program__fd() handles NULL by
returning libbpf_err(-EINVAL), so prog_fd will be negative and we
enter this error path. Then prog->name dereferences the NULL pointer.

> +		return -EINVAL;
> +	}
> +
> +	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
            ^^^^^^^^^
Similarly, is prog guaranteed non-NULL here? We know prog_fd is valid,
but that doesn't prove prog itself was checked.

> +		pr_warn("prog '%s': can't associate struct_ops program\n", prog->name);
> +		return -EINVAL;
> +	}
> +
> +	map_fd = bpf_map__fd(map);
> +	if (map_fd < 0) {
> +		pr_warn("map '%s': can't associate BPF map without FD (was it created?)\n", map->name);
                    ^^^^^^^^^^
Same issue with map - bpf_map__fd() handles NULL by returning an error,
but then we dereference map->name in the error message.

> +		return -EINVAL;
> +	}
> +
> +	if (!bpf_map__is_struct_ops(map)) {
            ^^^^^^^^^^^^^^^^^^^^^^^^
bpf_map__is_struct_ops() also dereferences map->def.type without a NULL
check, which would crash here if map is NULL.

> +		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);
> +}

Note: This follows the same pattern as existing functions like
bpf_program__pin() which also dereference prog->name without checking
prog for NULL. However, since the helper functions (bpf_program__fd,
bpf_map__fd) do handle NULL, it seems inconsistent to crash in the
error message when reporting that NULL was passed.

[ ... ]

---

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/19379369447

[kernel-patches-daemon-bpf]

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

[kernel-patches-review-bot]

commit bc7c50234c39e47198c8d313abf7e60021e53f11
Author: Amery Hung <[email protected]>

bpf: Allow verifier to fixup kernel module kfuncs

This commit removes an early return in fixup_kfunc_call() to allow
module kfuncs to be processed for special kfunc handling and __prog
argument fixup. The commit states that module kfunc BTF IDs are greater
than kernel kfunc BTF IDs, preventing collisions.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 99d62d765..d7702b9c6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -22416,8 +22416,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;
> +

Are the special kfunc comparisons below safe for module kfuncs?

The code after this change compares desc->func_id against
special_kfunc_list[] entries without checking desc->offset. Since
special_kfunc_list[] contains BTF IDs from kernel BTF, and module kfuncs
have BTF IDs from their own module BTF, could a module kfunc with a BTF
ID that happens to match a special kernel kfunc ID be incorrectly
processed?

In fixup_kfunc_call():
  desc = find_kfunc_desc(env->prog, insn->imm, insn->off);
  [...]
  if (desc->func_id == special_kfunc_list[KF_bpf_obj_new_impl] || ...)

The desc->func_id contains a BTF-local ID (set at line 3373), while
special_kfunc_list[] contains kernel BTF IDs. Module BTF IDs start from
1 independently, so collision is architecturally possible.

During verification, check_special_kfunc() at line 13691 checks "if
(meta->btf != btf_vmlinux) return 0;" which prevents aux_data fields
like kptr_struct_meta from being set for module kfuncs. But at fixup
time, if a module kfunc's BTF ID collides with a special kernel kfunc
ID, won't it enter these paths and read uninitialized aux_data fields?

Would adding "desc->offset == 0 &&" or "insn->off == 0 &&" to the
comparisons at lines 22420, 22437, 22460, 22481 ensure we only match
kernel kfuncs?

>  	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;

[ ... ]

---

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/19379369447

[kernel-patches-daemon-bpf]

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

[kernel-patches-daemon-bpf]

Upstream branch: c133390
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1023753
version: 6

[kernel-patches-daemon-bpf]

Upstream branch: ef2c0b2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1023753
version: 6

[kernel-patches-daemon-bpf]

Upstream branch: a4d31f4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1023753
version: 6

[kernel-patches-daemon-bpf]

Upstream branch: 4f7bc83
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1023753
version: 6

[kernel-patches-daemon-bpf]

Upstream branch: 6cc73f3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1023753
version: 6

[kernel-patches-daemon-bpf]

Upstream branch: 4722981
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1023753
version: 6