From: YASUOKA Masahiko Subject: Re: Kill pipexintr() To: mvs@openbsd.org Cc: bluhm@openbsd.org, tech@openbsd.org Date: Thu, 30 Oct 2025 18:48:21 +0900 On Tue, 28 Oct 2025 15:53:29 +0300 Vitaliy Makkoveev wrote: > It was used because the netlock state is unpredictable while > pipex_pppoe_output() was called. So the PPPoE output was deferred to > netisr to make sure the locks are held. However, the protocol of our > peer is always AF_UNSPEC, so ether_output() and following ether_resolve() > are just "memcpy(eh, dst->sa_data, sizeof(*eh));" where `dst' is the > immutable data which belongs to pipex session. yes > So I propose to manually fill ethernet header and enqueue packet. > Someone could say this the copy-paste from ether_output(), but it is > very small and this time I intentionally want to use it instead of > non-obvious ifp->if_output(). I'd ok if you want. ok yasuoka On Tue, 28 Oct 2025 15:53:29 +0300 Vitaliy Makkoveev wrote: > It was used because the netlock state is unpredictable while > pipex_pppoe_output() was called. So the PPPoE output was deferred to > netisr to make sure the locks are held. However, the protocol of our > peer is always AF_UNSPEC, so ether_output() and following ether_resolve() > are just "memcpy(eh, dst->sa_data, sizeof(*eh));" where `dst' is the > immutable data which belongs to pipex session. > > So I propose to manually fill ethernet header and enqueue packet. > Someone could say this the copy-paste from ether_output(), but it is > very small and this time I intentionally want to use it instead of > non-obvious ifp->if_output(). > > ok? > > Index: sys/net/if.c > =================================================================== > RCS file: /cvs/src/sys/net/if.c,v > diff -u -p -r1.741 if.c > --- sys/net/if.c 9 Sep 2025 09:16:18 -0000 1.741 > +++ sys/net/if.c 28 Oct 2025 12:20:05 -0000 > @@ -1103,10 +1103,6 @@ if_netisr(void *unused) > if (n & (1 << NETISR_BRIDGE)) > bridgeintr(); > #endif > -#ifdef PIPEX > - if (n & (1 << NETISR_PIPEX)) > - pipexintr(); > -#endif > #if NPPPOE > 0 > if (n & (1 << NETISR_PPPOE)) { > KERNEL_LOCK(); > Index: sys/net/netisr.h > =================================================================== > RCS file: /cvs/src/sys/net/netisr.h,v > diff -u -p -r1.61 netisr.h > --- sys/net/netisr.h 6 Jul 2023 04:55:05 -0000 1.61 > +++ sys/net/netisr.h 28 Oct 2025 12:20:05 -0000 > @@ -44,7 +44,6 @@ > #define NETISR_IP 2 /* same as AF_INET */ > #define NETISR_ARP 18 /* same as AF_LINK */ > #define NETISR_IPV6 24 /* same as AF_INET6 */ > -#define NETISR_PIPEX 27 /* for pipex processing */ > #define NETISR_PPP 28 /* for PPP processing */ > #define NETISR_BRIDGE 29 /* for bridge processing */ > #define NETISR_PPPOE 30 /* for pppoe processing */ > @@ -63,7 +62,6 @@ void ipintr(void); > void ip6intr(void); > void pppintr(void); > void bridgeintr(void); > -void pipexintr(void); > void pppoeintr(void); > > #define schednetisr(anisr) \ > Index: sys/net/pipex.c > =================================================================== > RCS file: /cvs/src/sys/net/pipex.c,v > diff -u -p -r1.161 pipex.c > --- sys/net/pipex.c 27 Aug 2025 22:39:04 -0000 1.161 > +++ sys/net/pipex.c 28 Oct 2025 12:20:05 -0000 > @@ -183,46 +183,6 @@ pipex_ioctl(void *ownersc, u_long cmd, c > } > > /************************************************************************ > - * Software Interrupt Handler > - ************************************************************************/ > - > -void > -pipexintr(void) > -{ > - struct mbuf_list ml; > - struct mbuf *m; > - struct pipex_session *session; > - > - NET_ASSERT_LOCKED(); > - > - mq_delist(&pipexoutq, &ml); > - > - while ((m = ml_dequeue(&ml)) != NULL) { > - struct ifnet *ifp; > - > - session = m->m_pkthdr.ph_cookie; > - > - ifp = if_get(session->proto.pppoe.over_ifidx); > - if (ifp != NULL) { > - struct pipex_pppoe_header *pppoe; > - int len; > - > - pppoe = mtod(m, struct pipex_pppoe_header *); > - len = ntohs(pppoe->length); > - ifp->if_output(ifp, m, &session->peer.sa, NULL); > - counters_pkt(session->stat_counters, pxc_opackets, > - pxc_obytes, len); > - } else { > - m_freem(m); > - counters_inc(session->stat_counters, pxc_oerrors); > - } > - if_put(ifp); > - > - pipex_rele_session(session); > - } > -} > - > -/************************************************************************ > * Session management functions > ************************************************************************/ > int > @@ -1335,6 +1295,7 @@ pipex_pppoe_input(struct mbuf *m0, struc > void > pipex_pppoe_output(struct mbuf *m0, struct pipex_session *session) > { > + struct ifnet *ifp; > struct pipex_pppoe_header *pppoe; > int len, padlen; > > @@ -1345,7 +1306,7 @@ pipex_pppoe_output(struct mbuf *m0, stru > M_PREPEND(m0, sizeof(struct pipex_pppoe_header), M_NOWAIT); > if (m0 == NULL) { > PIPEX_DBG((NULL, LOG_ERR, > - "<%s> cannot prepend header.", __func__)); > + "<%s> cannot prepend pppoe header.", __func__)); > counters_inc(session->stat_counters, pxc_oerrors); > return; > } > @@ -1362,15 +1323,37 @@ pipex_pppoe_output(struct mbuf *m0, stru > pppoe->length = htons(len); > > m0->m_pkthdr.ph_ifidx = session->proto.pppoe.over_ifidx; > - refcnt_take(&session->pxs_refcnt); > - m0->m_pkthdr.ph_cookie = session; > m0->m_flags &= ~(M_BCAST|M_MCAST); > > - if (mq_enqueue(&pipexoutq, m0) != 0) { > + M_PREPEND(m0, ETHER_ALIGN + sizeof(struct ether_header), M_NOWAIT); > + if (m0 == NULL) { > + PIPEX_DBG((NULL, LOG_ERR, > + "<%s> cannot prepend ethernet header.", __func__)); > counters_inc(session->stat_counters, pxc_oerrors); > - pipex_rele_session(session); > - } else > - schednetisr(NETISR_PIPEX); > + return; > + } > + > + ifp = if_get(session->proto.pppoe.over_ifidx); > + if (ifp != NULL) { > + struct arpcom *ac = (struct arpcom *)ifp; > + struct ether_header *eh; > + > + /* setup ethernet header information */ > + m_adj(m0, ETHER_ALIGN); > + eh = mtod(m0, struct ether_header *); > + memcpy(eh, session->peer.sa.sa_data, sizeof(*eh)); > + memcpy(eh->ether_shost, ac->ac_enaddr, sizeof(eh->ether_shost)); > + > + if (if_enqueue(ifp, m0) == 0) > + counters_pkt(session->stat_counters, pxc_opackets, > + pxc_obytes, len); > + else > + counters_inc(session->stat_counters, pxc_oerrors); > + } else { > + m_freem(m0); > + counters_inc(session->stat_counters, pxc_oerrors); > + } > + if_put(ifp); > } > #endif /* PIPEX_PPPOE */ > >