Re: [PATCH bpf-next v5 2/7] bpf: Support associating BPF program with struct_ops

bot+bpf-ci
2025-11-04T17:54:55Z

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

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a47d67db3..0f71030c0 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h

[ ... ]

> @@ -2026,6 +2028,9 @@ static inline void bpf_module_put(const void *data, struct module *owner)
> module_put(owner);
> }
> int bpf_struct_ops_link_create(union bpf_attr *attr);
> +int bpf_prog_assoc_struct_ops(struct bpf_prog *prog, struct bpf_map *map);
> +void bpf_prog_disassoc_struct_ops(struct bpf_prog *prog);
> +void *bpf_prog_get_assoc_struct_ops(const struct bpf_prog_aux *aux);
> u32 bpf_struct_ops_id(const void *kdata);

[ ... ]

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

[ ... ]

> @@ -1394,6 +1413,77 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
> return err;
> }
>
> +int bpf_prog_assoc_struct_ops(struct bpf_prog *prog, struct bpf_map *map)
> +{
> + struct bpf_map *st_ops_assoc;
> +
> + guard(mutex)(&prog->aux->st_ops_assoc_mutex);
> +
> + st_ops_assoc = prog->aux->st_ops_assoc;
> +
> + if (st_ops_assoc && st_ops_assoc == map)
> + return 0;
> +
> + if (st_ops_assoc) {
> + if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> + return -EBUSY;
> +
> + WRITE_ONCE(prog->aux->st_ops_assoc, BPF_PTR_POISON);
> + } else {
> + if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> + bpf_map_inc(map);
> +
> + WRITE_ONCE(prog->aux->st_ops_assoc, map);
> + }
> +
> + return 0;
> +}
> +
> +void bpf_prog_disassoc_struct_ops(struct bpf_prog *prog)
> +{
> + struct bpf_map *st_ops_assoc;
> +
> + guard(mutex)(&prog->aux->st_ops_assoc_mutex);
> +
> + st_ops_assoc = prog->aux->st_ops_assoc;
> +
> + if (!st_ops_assoc || st_ops_assoc == BPF_PTR_POISON)
> + return;
> +
> + if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> + bpf_map_put(st_ops_assoc);
> +
> + WRITE_ONCE(prog->aux->st_ops_assoc, NULL);
> +}
> +
> +/*
> + * Get a reference to the struct_ops struct (i.e., kdata) associated with a
> + * program.
> + *
> + * If the returned pointer is not NULL, it must points to a valid and
> + * initialized struct_ops. The struct_ops may or may not be attached.
> + * Kernel struct_ops implementers are responsible for tracking and checking
> + * the state of the struct_ops if the use case requires an attached struct_ops.
> + */
> +void *bpf_prog_get_assoc_struct_ops(const struct bpf_prog_aux *aux)
> +{
> + struct bpf_map *st_ops_assoc = READ_ONCE(aux->st_ops_assoc);
> + struct bpf_struct_ops_map *st_map;
> +
> + if (!st_ops_assoc || st_ops_assoc == BPF_PTR_POISON)
> + return NULL;
> +
> + st_map = (struct bpf_struct_ops_map *)st_ops_assoc;
> +
> + if (smp_load_acquire(&st_map->kvalue.common.state) == BPF_STRUCT_OPS_STATE_INIT) {
> + bpf_map_put(st_ops_assoc);
^^^^

Does bpf_prog_get_assoc_struct_ops() need to call bpf_map_put() here?

The function comment says "Get a reference to the struct_ops struct"
but the function never calls bpf_map_get/inc() to acquire a reference.
It only reads aux->st_ops_assoc via READ_ONCE().

When the state check fails (INIT state), the function calls bpf_map_put()
which drops the reference that was acquired in bpf_prog_assoc_struct_ops().
But on the success path below, it returns kdata without any refcount
operation.

This creates an imbalance:
- INIT state path: drops a reference, returns NULL
- Non-INIT path: no refcount change, returns kdata

The caller has no way to know whether the reference count was modified.
Also, the function is EXPORTED but doesn't document the reference counting
semantics clearly.

> + return NULL;
> + }
> +
> + return &st_map->kvalue.data;
> +}
> +EXPORT_SYMBOL_GPL(bpf_prog_get_assoc_struct_ops);

Can this function race with bpf_prog_disassoc_struct_ops()?

Since bpf_prog_get_assoc_struct_ops() reads aux->st_ops_assoc via
READ_ONCE() without holding the mutex or acquiring a reference, there's
a window where:

Thread A: reads st_ops_assoc pointer
Thread B: calls bpf_prog_disassoc_struct_ops()->bpf_map_put() (drops last ref)
Thread A: dereferences st_map->kvalue.common.state (use-after-free)

For non-struct_ops programs, bpf_prog_assoc_struct_ops() holds a
reference via bpf_map_inc(). This reference is dropped in
bpf_prog_disassoc_struct_ops() when the program is freed. Without
acquiring a new reference in bpf_prog_get_assoc_struct_ops(), the map
can be freed during the race window above.

[ ... ]

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