On Tue, Aug 09, 2022 at 11:30:32PM +0200, Kumar Kartikeya Dwivedi wrote: > The LRU map that is preallocated may have its elements reused while > another program holds a pointer to it from bpf_map_lookup_elem. Hence, > only check_and_free_fields is appropriate when the element is being > deleted, as it ensures proper synchronization against concurrent access > of the map value. After that, we cannot call check_and_init_map_value > again as it may rewrite bpf_spin_lock, bpf_timer, and kptr fields while > they can be concurrently accessed from a BPF program. > > This is safe to do as when the map entry is deleted, concurrent access > is protected against by check_and_free_fields, i.e. an existing timer > would be freed, and any existing kptr will be released by it. The > program can create further timers and kptrs after check_and_free_fields, > but they will eventually be released once the preallocated items are > freed on map destruction, even if the item is never reused again. Hence, > the deleted item sitting in the free list can still have resources > attached to it, and they would never leak. > > With spin_lock, we never touch the field at all on delete or update, as > we may end up modifying the state of the lock. Since the verifier > ensures that a bpf_spin_lock call is always paired with bpf_spin_unlock > call, the program will eventually release the lock so that on reuse the > new user of the value can take the lock. The bpf_spin_lock's verifier description makes sense. Note that the lru map does not support spin lock for now. > > Essentially, for the preallocated case, we must assume that the map > value may always be in use by the program, even when it is sitting in > the freelist, and handle things accordingly, i.e. use proper > synchronization inside check_and_free_fields, and never reinitialize the > special fields when it is reused on update. Acked-by: Martin KaFai Lau <kafai@xxxxxx>
