Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH bpf v3] xdp: fix hang while unregistering device bound
 to xdp socket
To:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn.topel@gmail.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>
Cc:     Netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        Xdp <xdp-newbies@vger.kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn.topel@intel.com>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Jakub Kicinski <jakub.kicinski@netronome.com>
From:   Ilya Maximets <i.maximets@samsung.com>
Message-ID: <e2313edb-6617-cd52-1a40-4712c9f20127@samsung.com>
Date:   Tue, 11 Jun 2019 11:42:28 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
        Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <CAJ+HfNgdiutAwpnc3LDDEGXs2SFCu3UtMnao79sFNyZZpQ2ETw@mail.gmail.com>
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20190610161551eucas1p1f370190ee6d0d5e921de1a21f3da72df@eucas1p1.samsung.com>
        <20190610161546.30569-1-i.maximets@samsung.com>
        <06C99519-64B9-4A91-96B9-0F99731E3857@gmail.com>
        <CAJ+HfNgdiutAwpnc3LDDEGXs2SFCu3UtMnao79sFNyZZpQ2ETw@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11.06.2019 11:09, Björn Töpel wrote:
> On Mon, 10 Jun 2019 at 22:49, Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
>>
>> On 10 Jun 2019, at 9:15, Ilya Maximets 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@samsung.com>
>>> ---
>>>
>>> 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);
>>
>> The call is under the rtnl lock, and we're not modifying
>> the list, so this mutex shouldn't be needed.
>>
> 
> The list can, however, be modified outside the rtnl lock (e.g. at
> socket creation). AFAIK the hlist cannot be traversed lock-less,
> right?

We could use 'rcu_read_lock' instead and iterate with 'sk_for_each_rcu',
but we'll not be able to synchronize inside.

> 
>>
>>> +             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();
>> Isn't this by handled by the unregister_count case below?
>>
> 
> To clarify, setting dev to NULL and xdp_del_sk_umem() + sync makes
> sure that a driver doesn't touch the Tx and Rx rings. Nothing can be
> assumed about completion + fill ring (umem), until zero-copy has been
> disabled via ndo_bpf.
> 
>>> +
>>> +                     /* Clear device references in umem. */
>>> +                     xdp_put_umem(xs->umem);
>>> +                     xs->umem = NULL;
>>
>> This makes me uneasy.  We need to unregister the umem from
>> the device (xdp_umem_clear_dev()) but this can remove the umem
>> pages out from underneath the xsk.
>>
> 
> Yes, this is scary. The socket is alive, and userland typically has
> the fill/completion rings mmapped. Then the umem refcount is decreased
> and can potentially free the umem (fill rings etc.), as Jonathan says,
> underneath the xsk. Also, setting the xs umem/dev to zero, while the
> socket is alive, would allow a user to re-setup the socket, which we
> don't want to allow.
> 
>> Perhaps what's needed here is the equivalent of an unbind()
>> call that just detaches the umem/sk from the device, but does
>> not otherwise tear them down.
>>
> 
> Yeah, I agree. A detached/zombie state is needed during the socket lifetime.


I could try to rip the 'xdp_umem_release()' apart so the 'xdp_umem_clear_dev()'
could be called separately. This will allow to not tear down the 'umem'.
However, it seems that it'll not be easy to synchronize all parts.
Any suggestions are welcome.

Also, there is no way to not clear the 'dev' as we have to put the reference.
Maybe we could add the additional check to 'xsk_bind()' for current device
state (dev->reg_state == NETREG_REGISTERED). This will allow us to avoid
re-setup of the socket.

> 
>>
>>> +                     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);
>> Not needed - xdp_put_umem() already does a null check.

Indeed. Thanks.

>> --
>> Jonathan
>>
>>
>>>
>>>       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
> 
>