On Fri, Jan 10, 2025 at 06:37:17AM +0100, Bjorn Ketelaars wrote:
> From: Bjorn Ketelaars <bket@openbsd.org>
> To: tech@openbsd.org
> Subject: Re: pppoe(4): replace PPPOE_TERM_UNKNOWN_SESSIONS with ifconfig
>  knob
> Date: Fri, 10 Jan 2025 06:37:17 +0100
> 
> On Fri 27/12/2024 20:32, Bjorn Ketelaars wrote:
> > The pppoe(4) manpage describes the kernel option
> > PPPOE_TERM_UNKNOWN_SESSIONS, which addresses an issue with
> > re-establishing a connection after a power failure. Rather than
> > requiring a kernel recompile, the proposal is to make this fix
> > configurable via an ifconfig(8) knob.
> > This fix provides a convenient solution for resolving issues I encounter
> > with reconnecting to my ISP.
> > 
> > The diff below includes the necessary changes to implement this and
> > updates to relevant manpages.
> > 
> > Comments/tests/OK?
> 
> Anyone interested in this?
> 
> Diff enclosed again for your convenience.
> 
 
May I propose you the following, which doesn't send packets when down,
is a little bit quieter and only sends one PADT per stale frame.


diff --git share/man/man4/options.4 share/man/man4/options.4
index 635532df510..dccc7c92d80 100644
--- share/man/man4/options.4
+++ share/man/man4/options.4
@@ -385,10 +385,6 @@ Enables BSD compressor for PPP connections.
 .It Cd option PPP_DEFLATE
 For use in conjunction with PPP_BSDCOMP; provides an interface to zlib for PPP
 for deflate compression/decompression.
-.It Cd option PPPOE_TERM_UNKNOWN_SESSIONS
-Send PADT to terminate open sessions before connecting.
-See
-.Xr pppoe 4 .
 .It Cd option SOCKET_SPLICE
 Enables zero-copy socket splicing in the kernel.
 See
diff --git share/man/man4/pppoe.4 share/man/man4/pppoe.4
index 98637cf22fb..d078d926eae 100644
--- share/man/man4/pppoe.4
+++ share/man/man4/pppoe.4
@@ -126,7 +126,8 @@ the gateway will be changed to the suggested address as well.
 .Pp
 Otherwise, PPP negotiation will only agree to exactly the IPv4 addresses
 which are configured on the interface.
-.Sh KERNEL OPTIONS
+.Pp
+By default,
 .Nm
 does not interfere with other PPPoE implementations
 running on the same machine.
@@ -137,13 +138,22 @@ reestablish a new PPPoE connection
 because there is already an open session.
 This would be indicated by the client sending a high number of PADI packets
 before successfully connecting.
-The
+Setting
+.Em link2
+makes
 .Nm
-driver can be told to kill all unknown PPPoE sessions
-by sending a PADT packet to explicitly terminate the old session.
-Add the following to the kernel config file:
-.Pp
-.Dl option PPPOE_TERM_UNKNOWN_SESSIONS
+answer packets for unknown sessions on its Ethernet interface
+with a PADT packet to explicitly terminate the old session,
+as long as no
+.Nm
+interface has an established session.
+Sessions of other PPPoE implementations
+using the same Ethernet interface are unknown to
+.Nm
+and would be terminated as well,
+so
+.Em link2
+should not be set in that case.
 .Sh PPPOE AND MTU/MSS
 PPPoE has an 8-byte header.
 When run over a network interface with the
diff --git sys/net/if_pppoe.c sys/net/if_pppoe.c
index 96bcf56fd3d..a5a8a81613f 100644
--- sys/net/if_pppoe.c
+++ sys/net/if_pppoe.c
@@ -834,17 +834,14 @@ pppoe_data_input(struct mbuf *m)
 	struct pppoe_softc *sc;
 	struct pppoehdr *ph;
 	u_int16_t session, plen;
-#ifdef PPPOE_TERM_UNKNOWN_SESSIONS
 	u_int8_t shost[ETHER_ADDR_LEN];
-#endif
+
 	if (LIST_EMPTY(&pppoe_softc_list))
 		goto drop;
 
 	KASSERT(m->m_flags & M_PKTHDR);
 
-#ifdef PPPOE_TERM_UNKNOWN_SESSIONS
 	memcpy(shost, mtod(m, struct ether_header*)->ether_shost, ETHER_ADDR_LEN);
-#endif
 	m_adj(m, sizeof(struct ether_header));
 	if (m->m_pkthdr.len <= PPPOE_HEADERLEN) {
 		printf("pppoe (data): dropping too short packet: %d bytes\n",
@@ -870,11 +867,21 @@ pppoe_data_input(struct mbuf *m)
 	session = ntohs(ph->session);
 	sc = pppoe_find_softc_by_session(session, m->m_pkthdr.ph_ifidx);
 	if (sc == NULL) {
-#ifdef PPPOE_TERM_UNKNOWN_SESSIONS
-		printf("pppoe (data): input for unknown session 0x%x, sending PADT\n",
-		    session);
-		pppoe_send_padt(m->m_pkthdr.ph_ifidx, session, shost, 0);
-#endif
+		/*
+		 * The peer may still hold a session from before a crash and
+		 * refuse to open a new one. If link2 is set, terminate it.
+		 */
+		LIST_FOREACH(sc, &pppoe_softc_list, sc_list)
+			if (sc->sc_eth_ifidx == m->m_pkthdr.ph_ifidx &&
+			    (sc->sc_sppp.pp_if.if_flags & (IFF_UP|IFF_LINK2)) ==
+			    (IFF_UP|IFF_LINK2))
+				break;
+		if (sc != NULL) {
+			PPPOEDEBUG(("%s: input for unknown session 0x%x, "
+			    "sending PADT\n", sc->sc_sppp.pp_if.if_xname,
+			    session));
+			pppoe_send_padt(m->m_pkthdr.ph_ifidx, session, shost, 0);
+		}
 		goto drop;
 	}