Re: possible deadlock in bpf_lru_push_free

[Yonghong Song]

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;