On Fri, Dec 30, 2022 at 12:11:45PM +0800, Hou Tao wrote: > From: Hou Tao <houtao1@xxxxxxxxxx> > > Hi, > > The patchset tries to fix the problems found when checking how htab map > handles element reuse in bpf memory allocator. The immediate reuse of > freed elements may lead to two problems in htab map: > > (1) reuse will reinitialize special fields (e.g., bpf_spin_lock) in > htab map value and it may corrupt lookup procedure with BFP_F_LOCK > flag which acquires bpf-spin-lock during value copying. The > corruption of bpf-spin-lock may result in hard lock-up. > (2) lookup procedure may get incorrect map value if the found element is > freed and then reused. > > Because the type of htab map elements are the same, so problem #1 can be > fixed by supporting ctor in bpf memory allocator. The ctor initializes > these special fields in map element only when the map element is newly > allocated. If it is just a reused element, there will be no > reinitialization. Instead of adding the overhead of ctor callback let's just add __GFP_ZERO to flags in __alloc(). That will address the issue 1 and will make bpf_mem_alloc behave just like percpu_freelist, so hashmap with BPF_F_NO_PREALLOC and default will behave the same way. > Problem #2 exists for both non-preallocated and preallocated htab map. > By adding seq in htab element, doing reuse check and retrying the > lookup procedure may be a feasible solution, but it will make the > lookup API being hard to use, because the user needs to check whether > the found element is reused or not and repeat the lookup procedure if it > is reused. A simpler solution would be just disabling freed elements > reuse and freeing these elements after lookup procedure ends. You've proposed this 'solution' twice already in qptrie thread and both times the answer was 'no, we cannot do this' with reasons explained. The 3rd time the answer is still the same. This 'issue 2' existed in hashmap since very beginning for many years. It's a known quirk. There is nothing to fix really. The graph apis (aka new gen data structs) with link list and rbtree are in active development. Soon bpf progs will be able to implement their own hash maps with explicit bpf_rcu_read_lock. At that time the progs will be making the trade off between performance and lookup/delete race. So please respin with just __GFP_ZERO and update the patch 6 to check for lockup only.
