Re: wg destroy hangs

[Kirill Miazine]

• Vitaliy Makkoveev [2023-10-05 01:10]:

On Thu, Oct 05, 2023 at 12:08:55AM +0200, Kirill Miazine wrote:

• Vitaliy Makkoveev [2023-10-05 00:02]:

On 5 Oct 2023, at 00:56, Kirill Miazine <k...@krot.org> wrote:

new diff doesn't prevent hang in test scenario either.

Which one?

I meant to say new diffS, as I had applied both... what I have now is this:

Understood.

The problem lays here:

ifq_start_task(void *p)
{
         struct ifqueue *ifq = p;
         struct ifnet *ifp = ifq->ifq_if;

         if (!ISSET(ifp->if_flags, IFF_RUNNING) ||
             ifq_empty(ifq) || ifq_is_oactive(ifq))
                 return;

         ifp->if_qstart(ifq);
}

wg_qstart(struct ifqueue *ifq)
{
         /* [...] */
         while ((m = ifq_dequeue(ifq)) != NULL) {
         /* [...] */
}

wg_peer_destroy(struct wg_peer *peer)
{
         /* [...] */
         NET_LOCK();
         while (!ifq_empty(&sc->sc_if.if_snd)) {
                 NET_UNLOCK();
                 tsleep_nsec(sc, PWAIT, "wg_ifq", 1000);
                 NET_LOCK();
         }
         NET_UNLOCK();
         /* [...] */
}

1. wg_output() placed some packets to sc->sc_if.if_snd and scheduled
ifq_start_task() to run.

2. You performed "ifconfig wg1 down", so wg_down() cleared IFF_RUNNING
flag.

3. ifq_start_task() started to run, IFF_RUNNING is not set, so
wg_qstart() will not be called as the ifq_dequeue(). Packets rests
within sc->sc_if.if_snd. The interface is down, so nothing would
schedule ifq_start_task() to run.

4. You performed "ifconfig wg1 destroy". The while(!ifq_empty()) loop is
infinite because nothing would empty sc->sc_if.if_snd at this point.

The unlocked !ISSET(ifp->if_flags, IFF_RUNNING), ifq_empty() and
ifq_is_oactive() are bad, but netlock dances provide caches
synchronisation.

I have no quick solution for this. Probably we should rethink
ifq_start_task().

This diff checks IFF_RUNNING flag within while (!ifq_empty()) loop of
wg_peer_destroy(). If the flag is not set queue will be purged and check
performed again. I intentionally keep netlock to prevent ifconfig
manipulations on the interface.

I confirm that just the diff below solved the issue

Index: sys/net/if_wg.c
===================================================================
RCS file: /cvs/src/sys/net/if_wg.c,v
retrieving revision 1.31
diff -u -p -r1.31 if_wg.c
--- sys/net/if_wg.c     26 Sep 2023 15:16:44 -0000      1.31
+++ sys/net/if_wg.c     4 Oct 2023 23:09:14 -0000
@@ -509,6 +509,13 @@ wg_peer_destroy(struct wg_peer *peer)

NET_LOCK();

        while (!ifq_empty(&sc->sc_if.if_snd)) {
+               /*
+                * XXX: `if_snd' of stopped interface could still packets
+                */
+               if (!ISSET(sc->sc_if.if_flags, IFF_RUNNING)) {
+                       ifq_purge(&sc->sc_if.if_snd);
+                       continue;
+               }
                NET_UNLOCK();
                tsleep_nsec(sc, PWAIT, "wg_ifq", 1000);
                NET_LOCK();