Messages in this thread | | | Subject | Re: possible deadlock in bpf_lru_push_free | From | Yonghong Song <> | Date | Tue, 18 Feb 2020 20:03:06 -0800 |
| |
On 2/18/20 6:15 PM, Hillf Danton wrote: > > Hey > > On Tue, 18 Feb 2020 15:55:02 -0800 Yonghong Song wrote: >> >> Thanks for Martin for explanation! I think changing l->hash_node.next is >> unsafe here as another thread may execute on a different cpu and >> traverse the same list. It will see hash_node.next = NULL and it is > > Good catch. > >> unexpected. >> >> How about the following patch? >> > Looks nicer, thanks :P > >> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c >> index 2d182c4ee9d9..246ef0f2e985 100644 >> --- a/kernel/bpf/hashtab.c >> +++ b/kernel/bpf/hashtab.c >> @@ -56,6 +56,7 @@ struct htab_elem { >> union { >> struct bpf_htab *htab; >> struct pcpu_freelist_node fnode; >> + struct htab_elem *link; >> }; >> }; >> }; >> @@ -1256,6 +1257,7 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map, >> void __user *ukeys = u64_to_user_ptr(attr->batch.keys); >> void *ubatch = u64_to_user_ptr(attr->batch.in_batch); >> u32 batch, max_count, size, bucket_size; >> + struct htab_elem *node_to_free = NULL; >> u64 elem_map_flags, map_flags; >> struct hlist_nulls_head *head; >> struct hlist_nulls_node *n; >> @@ -1370,9 +1372,14 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map, >> } >> if (do_delete) { >> hlist_nulls_del_rcu(&l->hash_node); >> - if (is_lru_map) >> - bpf_lru_push_free(&htab->lru, &l->lru_node); >> - else >> + if (is_lru_map) { >> + /* l->hnode overlaps with *l->hash_node.pprev > > nit: looks like you mean l->link
Yes, my previous attempt uses "hnode" and later changed to "link" but forget to change the comments.
Will post a patch soon.
> >> + * in memory. l->hash_node.pprev has been >> + * poisoned and nobody should access it. >> + */ >> + l->link = node_to_free; >> + node_to_free = l; >> + } else >> free_htab_elem(htab, l); >> } >> dst_key += key_size; >
| |