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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 5/3/24 09:19, Alexei Starovoitov wrote:
> On Thu, May 2, 2024 at 5:41 PM Kui-Feng Lee <sinquersw@gmail.com> 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.

Got it!