Re: [PATCH bpf-next v4 1/2] xsk: remove AF_XDP socket from map when the socket is released

On Mon, 12 Aug 2019 at 14:28, Daniel Borkmann [off-list ref] wrote:

[...]

quoted

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 59b57d708697..c3447bad608a 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c

@@ -362,6 +362,50 @@ static void xsk_unbind_dev(struct xdp_sock *xs)
      dev_put(dev);
  }

+static struct xsk_map *xsk_get_map_list_entry(struct xdp_sock *xs,
+                                           struct xdp_sock ***map_entry)
+{
+     struct xsk_map *map = NULL;
+     struct xsk_map_node *node;
+
+     *map_entry = NULL;
+
+     spin_lock_bh(&xs->map_list_lock);
+     node = list_first_entry_or_null(&xs->map_list, struct xsk_map_node,
+                                     node);
+     if (node) {
+             WARN_ON(xsk_map_inc(node->map));

Can you elaborate on the refcount usage here and against what scenario it is protecting?

Thanks for having a look!

First we access the map_list (under the lock) and pull out the map
which we intend to clean. In order to clear the map entry, we need to
a reference to the map. However, when the map_list_lock is released,
there's a window where the map entry can be cleared and the map can be
destroyed, and making the "map", which is used in
xsk_delete_from_maps, stale. To guarantee existence the additional
refinc is required. Makes sense?

Do we pretend it never fails on the bpf_map_inc() wrt the WARN_ON(),
why that (what makes it different from the xsk_map_node_alloc() inc
above where we do error out)?

Hmm, given that we're in a cleanup (socket release), we can't really
return any error. What would be a more robust way? Retrying? AFAIK the
release ops return an int, but it's not checked/used.

quoted

+             map = node->map;
+             *map_entry = node->map_entry;
+     }
+     spin_unlock_bh(&xs->map_list_lock);
+     return map;
+}
+
+static void xsk_delete_from_maps(struct xdp_sock *xs)
+{
+     /* This function removes the current XDP socket from all the
+      * maps it resides in. We need to take extra care here, due to
+      * the two locks involved. Each map has a lock synchronizing
+      * updates to the entries, and each socket has a lock that
+      * synchronizes access to the list of maps (map_list). For
+      * deadlock avoidance the locks need to be taken in the order
+      * "map lock"->"socket map list lock". We start off by
+      * accessing the socket map list, and take a reference to the
+      * map to guarantee existence. Then we ask the map to remove
+      * the socket, which tries to remove the socket from the
+      * map. Note that there might be updates to the map between
+      * xsk_get_map_list_entry() and xsk_map_try_sock_delete().
+      */

I tried to clarify here, but I obviously need to do a better job. :-)


Björn