Re: [PATCH bpf-next v3 07/15] bpf: introduce new bpf AF_XDP map type BPF_MAP_TYPE_XSKMAP

[Björn Töpel]

Den mån 8 okt. 2018 kl 18:05 skrev Björn Töpel <bjorn.to...@gmail.com>:
>
> Den mån 8 okt. 2018 kl 17:31 skrev Eric Dumazet <eric.duma...@gmail.com>:
> >
[...]
> > So it is illegal to call synchronize_net(), since it is a reschedule point.
> >
>
> Thanks for finding and pointing this out, Eric!
>
> I'll have look and get back with a patch.
>

Eric, something in the lines of the patch below? Or is it considered
bad practice to use call_rcu in this context (prone to DoSing the
kernel)?

Thanks for spending time on the xskmap code. Very much appreciated!

>From 491f7bd87705f72c45e59242fc6c3b1db9d3b56d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20T=C3=B6pel?= <bjorn.to...@intel.com>
Date: Mon, 8 Oct 2018 18:34:11 +0200
Subject: [PATCH] xsk: do not call synchronize_net() under RCU read lock
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

XSKMAP update and delete functions called synchronize_net(), which can
sleep. It is not allowed to sleep during an RCU read section.

Fixes: fbfc504a24f5 ("bpf: introduce new bpf AF_XDP map type
BPF_MAP_TYPE_XSKMAP")
Reported-by: Eric Dumazet <eric.duma...@gmail.com>
Signed-off-by: Björn Töpel <bjorn.to...@intel.com>
---
 include/net/xdp_sock.h |  1 +
 kernel/bpf/xskmap.c    | 21 +++++++++++----------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 13acb9803a6d..5b430141a3f6 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -68,6 +68,7 @@ struct xdp_sock {
      */
     spinlock_t tx_completion_lock;
     u64 rx_dropped;
+    struct rcu_head rcu;
 };

 struct xdp_buff;
diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
index 9f8463afda9c..51e8e2785612 100644
--- a/kernel/bpf/xskmap.c
+++ b/kernel/bpf/xskmap.c
@@ -157,6 +157,13 @@ static void *xsk_map_lookup_elem(struct bpf_map
*map, void *key)
     return NULL;
 }

+static void __xsk_map_remove_async(struct rcu_head *rcu)
+{
+    struct xdp_sock *xs = container_of(rcu, struct xdp_sock, rcu);
+
+    sock_put((struct sock *)xs);
+}
+
 static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
                    u64 map_flags)
 {
@@ -192,11 +199,8 @@ static int xsk_map_update_elem(struct bpf_map
*map, void *key, void *value,
     sock_hold(sock->sk);

     old_xs = xchg(&m->xsk_map[i], xs);
-    if (old_xs) {
-        /* Make sure we've flushed everything. */
-        synchronize_net();
-        sock_put((struct sock *)old_xs);
-    }
+    if (old_xs)
+        call_rcu(&old_xs->rcu, __xsk_map_remove_async);

     sockfd_put(sock);
     return 0;
@@ -212,11 +216,8 @@ static int xsk_map_delete_elem(struct bpf_map
*map, void *key)
         return -EINVAL;

     old_xs = xchg(&m->xsk_map[k], NULL);
-    if (old_xs) {
-        /* Make sure we've flushed everything. */
-        synchronize_net();
-        sock_put((struct sock *)old_xs);
-    }
+    if (old_xs)
+        call_rcu(&old_xs->rcu, __xsk_map_remove_async);

     return 0;
 }
-- 
2.17.1