Re: [PATCH bpf-next v3 02/17] bpf: allow attaching struct_ops to cgroups

[Roman Gushchin <roman.gushchin@linux.dev>]

Josh Don <joshdon@google.com> writes:

> Hi Roman,
>
> On Mon, Jan 26, 2026 at 6:50 PM Roman Gushchin <roman.gushchin@linux.dev> wrote:
>>
>> Introduce an ability to attach bpf struct_ops'es to cgroups.
>>
> [snip]
>>  struct bpf_struct_ops_value {
>>         struct bpf_struct_ops_common_value common;
>> @@ -1220,6 +1222,10 @@ static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
>>                 st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
>>                 bpf_map_put(&st_map->map);
>>         }
>> +
>> +       if (st_link->cgroup)
>> +               cgroup_bpf_detach_struct_ops(st_link->cgroup, st_link);
>> +

Hi Josh!

>
> I was worried about concurrency with cgroup ops until I saw
> cgroup_bpf_detach_struct_ops() takes cgroup_lock() internally (since
> you take it inline sometimes below I falsely assumed it wasn't
> present). In any case, I'm wondering why you need to pass in the
> cgroup pointer to cgroup_bpf_detach_struct_ops() at all, rather than
> just the link?

Sure, good point.

>> @@ -1357,8 +1386,12 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>>         struct bpf_link_primer link_primer;
>>         struct bpf_struct_ops_map *st_map;
>>         struct bpf_map *map;
>> +       struct cgroup *cgrp;
>>         int err;
>>
>> +       if (attr->link_create.flags & ~BPF_F_CGROUP_FD)
>> +               return -EINVAL;
>> +
>>         map = bpf_map_get(attr->link_create.map_fd);
>>         if (IS_ERR(map))
>>                 return PTR_ERR(map);
>> @@ -1378,11 +1411,26 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>>         bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS, &bpf_struct_ops_map_lops, NULL,
>>                       attr->link_create.attach_type);
>>
>> +       init_waitqueue_head(&link->wait_hup);
>> +
>> +       if (attr->link_create.flags & BPF_F_CGROUP_FD) {
>> +               cgrp = cgroup_get_from_fd(attr->link_create.target_fd);
>> +               if (IS_ERR(cgrp)) {
>> +                       err = PTR_ERR(cgrp);
>> +                       goto err_out;
>> +               }
>> +               link->cgroup = cgrp;
>> +               err = cgroup_bpf_attach_struct_ops(cgrp, link);
>
> We have to be careful at this point. cgroup release could now occur
> concurrently which would clear link->cgroup. Maybe worth a comment
> here since this is a bit subtle.

Agree, will add.

Thanks!