Re: [PATCH bpf v3] xdp: fix hang while unregistering device bound to xdp socket

[Andrii Nakryiko]

On Mon, Jun 10, 2019 at 9:39 AM Ilya Maximets <i.maximets@xxxxxxxxxxx> wrote:
>
> Device that bound to XDP socket will not have zero refcount until the
> userspace application will not close it. This leads to hang inside
> 'netdev_wait_allrefs()' if device unregistering requested:
>
>   # ip link del p1
>   < hang on recvmsg on netlink socket >
>
>   # ps -x | grep ip
>   5126  pts/0    D+   0:00 ip link del p1
>
>   # journalctl -b
>
>   Jun 05 07:19:16 kernel:
>   unregister_netdevice: waiting for p1 to become free. Usage count = 1
>
>   Jun 05 07:19:27 kernel:
>   unregister_netdevice: waiting for p1 to become free. Usage count = 1
>   ...
>
> Fix that by implementing NETDEV_UNREGISTER event notification handler
> to properly clean up all the resources and unref device.
>
> This should also allow socket killing via ss(8) utility.
>
> Fixes: 965a99098443 ("xsk: add support for bind for Rx")
> Signed-off-by: Ilya Maximets <i.maximets@xxxxxxxxxxx>
> ---
>
> Version 3:
>
>     * Declaration lines ordered from longest to shortest.
>     * Checking of event type moved to the top to avoid unnecessary
>       locking.
>
> Version 2:
>
>     * Completely re-implemented using netdev event handler.
>
>  net/xdp/xsk.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a14e8864e4fa..273a419a8c4d 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -693,6 +693,57 @@ static int xsk_mmap(struct file *file, struct socket *sock,
>                                size, vma->vm_page_prot);
>  }
>
> +static int xsk_notifier(struct notifier_block *this,
> +                       unsigned long msg, void *ptr)
> +{
> +       struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +       struct net *net = dev_net(dev);
> +       int i, unregister_count = 0;
> +       struct sock *sk;
> +
> +       switch (msg) {
> +       case NETDEV_UNREGISTER:
> +               mutex_lock(&net->xdp.lock);
> +               sk_for_each(sk, &net->xdp.list) {
> +                       struct xdp_sock *xs = xdp_sk(sk);
> +
> +                       mutex_lock(&xs->mutex);
> +                       if (dev != xs->dev) {
> +                               mutex_unlock(&xs->mutex);
> +                               continue;
> +                       }
> +
> +                       sk->sk_err = ENETDOWN;
> +                       if (!sock_flag(sk, SOCK_DEAD))
> +                               sk->sk_error_report(sk);
> +
> +                       /* Wait for driver to stop using the xdp socket. */
> +                       xdp_del_sk_umem(xs->umem, xs);
> +                       xs->dev = NULL;
> +                       synchronize_net();
> +
> +                       /* Clear device references in umem. */
> +                       xdp_put_umem(xs->umem);
> +                       xs->umem = NULL;
> +
> +                       mutex_unlock(&xs->mutex);
> +                       unregister_count++;
> +               }
> +               mutex_unlock(&net->xdp.lock);
> +
> +               if (unregister_count) {
> +                       /* Wait for umem clearing completion. */
> +                       synchronize_net();
> +                       for (i = 0; i < unregister_count; i++)
> +                               dev_put(dev);
> +               }
> +
> +               break;
> +       }
> +
> +       return NOTIFY_DONE;
> +}
> +
>  static struct proto xsk_proto = {
>         .name =         "XDP",
>         .owner =        THIS_MODULE,
> @@ -727,7 +778,8 @@ static void xsk_destruct(struct sock *sk)
>         if (!sock_flag(sk, SOCK_DEAD))
>                 return;
>
> -       xdp_put_umem(xs->umem);
> +       if (xs->umem)
> +               xdp_put_umem(xs->umem);

xpd_put_umem already checks for NULL umem, so you don't have to do it here.

>
>         sk_refcnt_debug_dec(sk);
>  }
> @@ -784,6 +836,10 @@ static const struct net_proto_family xsk_family_ops = {
>         .owner  = THIS_MODULE,
>  };
>
> +static struct notifier_block xsk_netdev_notifier = {
> +       .notifier_call  = xsk_notifier,
> +};
> +
>  static int __net_init xsk_net_init(struct net *net)
>  {
>         mutex_init(&net->xdp.lock);
> @@ -816,8 +872,15 @@ static int __init xsk_init(void)
>         err = register_pernet_subsys(&xsk_net_ops);
>         if (err)
>                 goto out_sk;
> +
> +       err = register_netdevice_notifier(&xsk_netdev_notifier);
> +       if (err)
> +               goto out_pernet;
> +
>         return 0;
>
> +out_pernet:
> +       unregister_pernet_subsys(&xsk_net_ops);
>  out_sk:
>         sock_unregister(PF_XDP);
>  out_proto:
> --
> 2.17.1
>