Re: [PATCH bpf v1 2/3] bpf: Don't reinit map value in prealloc_lru_pop

[Yonghong Song <yhs@fb.com>]

On 8/5/22 6:46 PM, 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.

If I understand correctly, one lru_node gets freed and pushed to free 
list without doing check_and_free_fields().
If later the same node is used with bpf_map_update_elem() and 
prealloc_lru_pop() is called, then with this patch, 
check_and_init_map_value() is not called, so the new node may contain
leftover values for kptr/timer/spin_lock, could this cause a problem?

To address the above rewrite issue, maybe the solution should be
to push the deleted lru_nodes back to free list only after 
rcu_read_unlock() is done?

> 
> Fixes: 68134668c17f ("bpf: Add map side support for bpf timers.")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>   kernel/bpf/hashtab.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index da7578426a46..4d793a92301b 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -311,12 +311,8 @@ static struct htab_elem *prealloc_lru_pop(struct bpf_htab *htab, void *key,
>   	struct htab_elem *l;
>   
>   	if (node) {
> -		u32 key_size = htab->map.key_size;
> -
>   		l = container_of(node, struct htab_elem, lru_node);
> -		memcpy(l->key, key, key_size);
> -		check_and_init_map_value(&htab->map,
> -					 l->key + round_up(key_size, 8));
> +		memcpy(l->key, key, htab->map.key_size);
>   		return l;
>   	}
>