On 2/18/20 6:15 PM, Hillf Danton wrote:

Hey

On Tue, 18 Feb 2020 15:55:02 -0800 Yonghong Song wrote:

quoted

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.

quoted

unexpected.

How about the following patch?

Looks nicer, thanks :P

quoted

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.

quoted

+                                * 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;