Re: wg destroy hangs

[Vitaliy Makkoveev]

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.


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();