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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ+HfNjCg5pNb9ZtFLLZym-+pSeX46h8ncU30nmXLHQ=1+KdBA@mail.gmail.com>
Date:   Mon, 8 Oct 2018 18:52:22 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "Duyck, Alexander H" <alexander.h.duyck@...el.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Netdev <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        michael.lundkvist@...csson.com,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Singhai, Anjali" <anjali.singhai@...el.com>,
        "Zhang, Qi Z" <qi.z.zhang@...el.com>
Subject: Re: [PATCH bpf-next v3 07/15] bpf: introduce new bpf AF_XDP map type BPF_MAP_TYPE_XSKMAP

Den mån 8 okt. 2018 kl 18:05 skrev Björn Töpel <bjorn.topel@...il.com>:
>
> Den mån 8 okt. 2018 kl 17:31 skrev Eric Dumazet <eric.dumazet@...il.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.topel@...el.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.dumazet@...il.com>
Signed-off-by: Björn Töpel <bjorn.topel@...el.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