Re: [PATCH bpf-next 3/6] bpf: provide a function to unregister struct_ops objects from consumers.

[Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>]

On Thu, May 2, 2024 at 5:41 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
> >>> +    prev_state = cmpxchg(&st_map->kvalue.common.state,
> >>> +                 BPF_STRUCT_OPS_STATE_INUSE,
> >>> +                 BPF_STRUCT_OPS_STATE_TOBEFREE);

All the uses of cmpxchg mean that there are races.
I know there is already a case for it in the existing code
and maybe it's justified.
But I would very much prefer the whole code converted to
clean locks without cmpxchg tricks.


> >>> +    if (prev_state == BPF_STRUCT_OPS_STATE_INUSE) {
> >>> +        st_map->st_ops_desc->st_ops->unreg(data);
> >>> +        /* Pair with bpf_map_inc() for reg() */
> >>> +        bpf_map_put(&st_map->map);
> >>> +        /* Pair with bpf_map_inc_not_zero() above */
> >>> +        bpf_map_put(&st_map->map);
> >>> +        return true;

I haven't tried to follow the logic, but double bpf_map_put
on the same map screams that this is broken.
Do proper locks everywhere.
inc_not_zero and cmpxchg shouldn't be needed.
keep it simple.