Hi. > On 29 Dec 2020, at 22:48, Klemens Nanni <k...@openbsd.org> wrote: > > Earlier this year `struct pppoe_softc' was annotated with lock comments > showing no member being protected by KERNEL_LOCK() alone. > > After further review of the code paths starting from pppoeintr() I also > could not find sleeping points, which must be avoided in the sofnet > thread. (As part of this, I specifically went through all possible > malloc(9) calls and verified that none of them use `M_WAITOK'.) > > The only thing I see neccessary now is wrapping mbuf queue access with > if_get(9) to prevent interfaces from disappearing while processing the > mbuf -- currently the big lock protects against that.
For me these if_{get,put}(9) dances are useless. This `ph_ifidx’ is related to ethernet device and used to find pppoe(4) related software context. pppoe_send_padt() will get this `ifp’ as `eth_if’ by itself and correctly handle the case where this ethernet interface was gone. Also `if_list’ modifications are simultaneously protected by kernel lock and netlock. > > I've been running with this diff for over four months on an octeon > edgerouter 4 which updates to snapshots modulo the custom kernel roughly > once a week. > > Others also successfully tested this on octeon and amd64 based setups. > > Did I miss anything? > Feedback? Objections? OK? > > > Index: if.c > =================================================================== > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.621 > diff -u -p -r1.621 if.c > --- if.c 15 Dec 2020 03:43:34 -0000 1.621 > +++ if.c 28 Dec 2020 18:13:02 -0000 > @@ -907,9 +907,7 @@ if_netisr(void *unused) > #endif > #if NPPPOE > 0 > if (n & (1 << NETISR_PPPOE)) { > - KERNEL_LOCK(); > pppoeintr(); > - KERNEL_UNLOCK(); > } > #endif > t |= n; > Index: if_pppoe.c > =================================================================== > RCS file: /cvs/src/sys/net/if_pppoe.c,v > retrieving revision 1.73 > diff -u -p -r1.73 if_pppoe.c > --- if_pppoe.c 13 Sep 2020 11:00:40 -0000 1.73 > +++ if_pppoe.c 28 Dec 2020 18:13:02 -0000 > @@ -346,14 +346,29 @@ void > pppoeintr(void) > { > struct mbuf *m; > + struct ifnet *ifp; > > NET_ASSERT_LOCKED(); > > - while ((m = niq_dequeue(&pppoediscinq)) != NULL) > + while ((m = niq_dequeue(&pppoediscinq)) != NULL) { > + ifp = if_get(m->m_pkthdr.ph_ifidx); > + if (ifp == NULL) { > + m_freem(m); > + continue; > + } > pppoe_disc_input(m); > + if_put(ifp); > + } > > - while ((m = niq_dequeue(&pppoeinq)) != NULL) > + while ((m = niq_dequeue(&pppoeinq)) != NULL) { > + ifp = if_get(m->m_pkthdr.ph_ifidx); > + if (ifp == NULL) { > + m_freem(m); > + continue; > + } > pppoe_data_input(m); > + if_put(ifp); > + } > } > > /* Analyze and handle a single received packet while not in session state. */ >