Hi, wg(4) stores reference to peer in the outgoing mbuf. Since it doesn't use reference counting, to make the `peer' dereference safe within wg_qstart(), wg_peer_destroy() has the following delay loop:
NET_LOCK(); while (!ifq_empty(&sc->sc_if.if_snd)) { /* * XXX: `if_snd' of stopped interface could still * contain packets */ if (!ISSET(sc->sc_if.if_flags, IFF_RUNNING)) { ifq_purge(&sc->sc_if.if_snd); continue; } NET_UNLOCK(); tsleep_nsec(&nowake, PWAIT, "wg_ifq", 1000); NET_LOCK(); } NET_UNLOCK(); Regardless on exclusive netlock acquisistion which stops packet processing, this loop also could became infinite on huge traffic. I want to remove it. Unfortunately, we can't use reference counting for outgoing packets because they could stuck within `if_snd' of stopped interface. These packets will be removed on interface destruction, so the reference of `peer' will never be released. There is another way: to do wg_aip_lookup() for each packet within wg_qstart(). This diff implements it. Obviously, wg_aip_lookup() provides some performance impact, so I'm asking for help with testing this diff on configuration with huge amount of peers and traffic pressure. I'm interesting what is the performance impact and is the performance impact acceptable. Of course any other help with testing is also welcomed. Also, this diff restores pf(4) delay option for wg(4) interfaces. Index: sys/net/if_wg.c =================================================================== RCS file: /cvs/src/sys/net/if_wg.c,v retrieving revision 1.36 diff -u -p -r1.36 if_wg.c --- sys/net/if_wg.c 18 Jan 2024 08:46:41 -0000 1.36 +++ sys/net/if_wg.c 19 Jan 2024 13:45:06 -0000 @@ -507,22 +507,6 @@ wg_peer_destroy(struct wg_peer *peer) noise_remote_clear(&peer->p_remote); - NET_LOCK(); - while (!ifq_empty(&sc->sc_if.if_snd)) { - /* - * XXX: `if_snd' of stopped interface could still - * contain packets - */ - if (!ISSET(sc->sc_if.if_flags, IFF_RUNNING)) { - ifq_purge(&sc->sc_if.if_snd); - continue; - } - NET_UNLOCK(); - tsleep_nsec(&nowake, PWAIT, "wg_ifq", 1000); - NET_LOCK(); - } - NET_UNLOCK(); - taskq_barrier(wg_crypt_taskq); taskq_barrier(net_tq(sc->sc_if.if_index)); @@ -2092,20 +2076,39 @@ wg_qstart(struct ifqueue *ifq) struct ifnet *ifp = ifq->ifq_if; struct wg_softc *sc = ifp->if_softc; struct wg_peer *peer; - struct wg_tag *t; struct mbuf *m; SLIST_HEAD(,wg_peer) start_list; SLIST_INIT(&start_list); + rw_enter_read(&sc->sc_lock); /* * We should be OK to modify p_start_list, p_start_onlist in this * function as there should only be one ifp->if_qstart invoked at a * time. */ while ((m = ifq_dequeue(ifq)) != NULL) { - t = wg_tag_get(m); - peer = t->t_peer; + switch (m->m_pkthdr.ph_family) { + case AF_INET: + peer = wg_aip_lookup(sc->sc_aip4, + &mtod(m, struct ip *)->ip_dst); + break; +#ifdef INET6 + case AF_INET6: + peer = wg_aip_lookup(sc->sc_aip6, + &mtod(m, struct ip6_hdr *)->ip6_dst); + break; +#endif + default: + m_freem(m); + continue; + } + + if (peer == NULL) { + m_freem(m); + continue; + } + if (mq_push(&peer->p_stage_queue, m) != 0) counters_inc(ifp->if_counters, ifc_oqdrops); if (!peer->p_start_onlist) { @@ -2120,6 +2123,8 @@ wg_qstart(struct ifqueue *ifq) wg_timers_event_want_initiation(&peer->p_timers); peer->p_start_onlist = 0; } + rw_exit_read(&sc->sc_lock); + task_add(wg_crypt_taskq, &sc->sc_encap); } @@ -2175,19 +2180,6 @@ wg_output(struct ifnet *ifp, struct mbuf if (m->m_pkthdr.ph_loopcnt++ > M_MAXLOOP) { DPRINTF(sc, "Packet looped\n"); ret = ELOOP; - goto error; - } - - /* - * As we hold a reference to peer in the mbuf, we can't handle a - * delayed packet without doing some refcnting. If a peer is removed - * while a delayed holds a reference, bad things will happen. For the - * time being, delayed packets are unsupported. This may be fixed with - * another aip_lookup in wg_qstart, or refcnting as mentioned before. - */ - if (m->m_pkthdr.pf.delay > 0) { - DPRINTF(sc, "PF delay unsupported\n"); - ret = EOPNOTSUPP; goto error; }