Re: [PATCH bpf-next v5 3/7] bpf: Pin associated struct_ops when registering async callback

[Martin KaFai Lau <martin.lau@linux.dev>]

On 11/6/25 9:57 AM, Amery Hung wrote:
> On Wed, Nov 5, 2025 at 6:13 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 11/4/25 9:26 AM, Amery Hung wrote:
>>> Take a refcount of the associated struct_ops map to prevent the map from
>>> being freed when an async callback scheduled from a struct_ops program
>>> runs.
>>>
>>> Since struct_ops programs do not take refcounts on the struct_ops map,
>>> it is possible for a struct_ops map to be freed when an async callback
>>> scheduled from it runs. To prevent this, take a refcount on prog->aux->
>>> st_ops_assoc and save it in a newly created struct bpf_async_res for
>>> every async mechanism. The reference needs to be preserved in
>>> bpf_async_res since prog->aux->st_ops_assoc can be poisoned anytime
>>> and reference leak could happen.
>>>
>>> bpf_async_res will contain a async callback's BPF program and resources
>>> related to the BPF program. The resources will be acquired when
>>> registering a callback and released when cancelled or when the map
>>> associated with the callback is freed.
>>>
>>> Also rename drop_prog_refcnt to bpf_async_cb_reset to better reflect
>>> what it now does.
>>>
>>
>> [ ... ]
>>
>>> +static int bpf_async_res_get(struct bpf_async_res *res, struct bpf_prog *prog)
>>> +{
>>> +     struct bpf_map *st_ops_assoc = NULL;
>>> +     int err;
>>> +
>>> +     prog = bpf_prog_inc_not_zero(prog);
>>> +     if (IS_ERR(prog))
>>> +             return PTR_ERR(prog);
>>> +
>>> +     st_ops_assoc = READ_ONCE(prog->aux->st_ops_assoc);
>>> +     if (prog->type == BPF_PROG_TYPE_STRUCT_OPS &&
>>> +         st_ops_assoc && st_ops_assoc != BPF_PTR_POISON) {
>>> +             st_ops_assoc = bpf_map_inc_not_zero(st_ops_assoc);
>>
>> The READ_ONCE and inc_not_zero is an unusual combo. Should it be
>> rcu_dereference and prog->aux->st_ops_assoc should be "__rcu" tagged?
>>
> 
> Understood the underlying struct_ops map is protected by RCU, but
> prog->aux->st_ops_assoc is not protected by RCU and can change
> anytime.

hmm... at least for BPF_PROG_TYPE_STRUCT_OPS, the struct_ops map refcnt 
is not taken in patch 2. The prog->aux->st_ops_assoc can be used is 
because of the rcu gp.

Another thing I am likely missing is, the refcnted st_ops_assoc is saved 
in res->st_ops_assoc. If I read it correctly, the kfunc is using 
bpf_prog_get_assoc_struct_ops() which is reading from 
[prog->]aux->st_ops_assoc instead of the saved res->st_ops_assoc. Can 
the aux->st_ops_assoc be pointing to another struct_ops map different 
from res->st_ops_assoc?