xsk: proper AF_XDP socket teardown ordering

The AF_XDP socket struct can exist in three different, implicit
states: setup, bound and released. Setup is prior the socket has been
bound to a device. Bound is when the socket is active for receive and
send. Released is when the process/userspace side of the socket is
released, but the sock object is still lingering, e.g. when there is a
reference to the socket in an XSKMAP after process termination.

The Rx fast-path code uses the "dev" member of struct xdp_sock to
check whether a socket is bound or relased, and the Tx code uses the
struct xdp_umem "xsk_list" member in conjunction with "dev" to
determine the state of a socket.

However, the transition from bound to released did not tear the socket
down in correct order.

On the Rx side "dev" was cleared after synchronize_net() making the
synchronization useless. On the Tx side, the internal queues were
destroyed prior removing them from the "xsk_list".

This commit corrects the cleanup order, and by doing so
xdp_del_sk_umem() can be simplified and one synchronize_net() can be
removed.

Fixes: f19f2dc69f6e ("xsk: add support for bind for Rx")
Fixes: 09ada008f9a2 ("xsk: wire upp Tx zero-copy functions")
Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index c6007c58231c07657479f08492e85def3dfdd65b..a264cf2accd0f1c2d5a90019535db8bf736ce311 100644 (file)
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -32,14 +32,9 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
 {
        unsigned long flags;
 
-       if (xs->dev) {
-               spin_lock_irqsave(&umem->xsk_list_lock, flags);
-               list_del_rcu(&xs->list);
-               spin_unlock_irqrestore(&umem->xsk_list_lock, flags);
-
-               if (umem->zc)
-                       synchronize_net();
-       }
+       spin_lock_irqsave(&umem->xsk_list_lock, flags);
+       list_del_rcu(&xs->list);
+       spin_unlock_irqrestore(&umem->xsk_list_lock, flags);
 }
 
 /* The umem is stored both in the _rx struct and the _tx struct as we do

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index caeddad15b7c181dcecb2048b96de6751988525c..0577cd49aa72c9fbfa4ea13e58cf2d3f3c10a207 100644 (file)
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -355,12 +355,18 @@ static int xsk_release(struct socket *sock)
        local_bh_enable();
 
        if (xs->dev) {
+               struct net_device *dev = xs->dev;
+
                /* Wait for driver to stop using the xdp socket. */
-               synchronize_net();
-               dev_put(xs->dev);
+               xdp_del_sk_umem(xs->umem, xs);
                xs->dev = NULL;
+               synchronize_net();
+               dev_put(dev);
        }
 
+       xskq_destroy(xs->rx);
+       xskq_destroy(xs->tx);
+
        sock_orphan(sk);
        sock->sk = NULL;
 
@@ -714,9 +720,6 @@ static void xsk_destruct(struct sock *sk)
        if (!sock_flag(sk, SOCK_DEAD))
                return;
 
-       xskq_destroy(xs->rx);
-       xskq_destroy(xs->tx);
-       xdp_del_sk_umem(xs->umem, xs);
        xdp_put_umem(xs->umem);
 
        sk_refcnt_debug_dec(sk);