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.
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. */
