On 5/3/24 09:19, Alexei Starovoitov wrote:
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.
Got it!
