Otherwise `if_snd' could still contain packets. If any of them has
associated data with bumped reference counter, the corresponding
refcnt_finalize() will sleep forever. I don't think we need to
transmit these packets when interface became running again.

I don't like this unlocked if_flags check we have in ifq_start_task().
Guess READ_ONCE() is much better to load value.

The problem reported in the "wg destroy hangs" [1] thread is the kind of
described above. Also, current ifq_start_task() implementation denies to
introduce reference counters for wg_peer and remove this
wg_peer_destroy() hack:

         NET_LOCK();
         while (!ifq_empty(&sc->sc_if.if_snd)) {
                 NET_UNLOCK();
                 tsleep_nsec(sc, PWAIT, "wg_ifq", 1000);
                 NET_LOCK();
         }
         NET_UNLOCK();

1. https://marc.info/?t=169642396300001&r=1&w=2

ok?

Index: sys/net/ifq.c
===================================================================
RCS file: /cvs/src/sys/net/ifq.c,v
retrieving revision 1.51
diff -u -p -r1.51 ifq.c
--- sys/net/ifq.c       5 Oct 2023 11:08:56 -0000       1.51
+++ sys/net/ifq.c       5 Oct 2023 11:49:40 -0000
@@ -140,8 +140,11 @@ 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))
+       if (!ISSET(READ_ONCE(ifp->if_flags), IFF_RUNNING)){
+               ifq_purge(ifq);
+               return;
+       }
+       if (ifq_empty(ifq) || ifq_is_oactive(ifq))
                return;
 
        ifp->if_qstart(ifq);

Reply via email to