Any OK for this?

For convenience, I have put the patch inline in this mail


On Thu, Oct 15, 2026 at 07:15:37AM +0200, Bjorn Ketelaars wrote:
> From: Bjorn Ketelaars <bket@openbsd.org>
> To: Renaud Allard <renaud@allard.it>
> Cc: tech@openbsd.org
> Subject: Re: pppoe(4): replace PPPOE_TERM_UNKNOWN_SESSIONS with ifconfig
>  knob
> Date: Thu, 15 Oct 2026 07:15:37 +0200
> 
> On Wed 14/10/2026 10:51, Renaud Allard wrote:
> > On Tue, Oct 13, 2026 at 08:07:17PM +0200, Bjorn Ketelaars wrote:
> > > Thanks for looking at this! Your version addresses the issue more
> > > neatly. Avoiding sending when down, keeping the logging quieter, and
> > > sending only one PADT per stale frame all make sense to me.
> > 
> > Actually, I made more tests and testing for IFF_UP defeats the whole
> > thing. After the refused login, sppp_lcp_down() calls if_down() itself,
> > so the interface is down exactly when the stale session has to be
> > killed. Client losing its session without telling the AC.
> > Live testing with my ISP is quite complex as it can bring my line down
> > for 30-60+ minutes.
> > 
> 
> Thanks for testing this further, and sorry I missed the IFF_UP issue.  I
> did not check which paths could change that flag before giving my OK.
> Removing the check brings the behaviour closer to my proposal, which
> worked for me at the time, while retaining your improvements to logging
> and avoiding duplicate PADTs.
> 
> Something appears to have changed on my ISP's side, beyond my control.
> I no longer experience minutes-long connectivity loss after an
> uncontrolled reboot, so I cannot reproduce the original stale-session
> problem. I have built and run the updated patch, as I did with the
> previous version.
> 
> I have now also tested the unknown-session handling using Scapy on a
> second machine, sending through igc1 to gateway's bge0, the parent of
> pppoe9. The script was:
> 
> from scapy.all import Ether, IP, PPP, PPPoE, sendp
> 
> packet = (
>     Ether(dst="d0:bf:9c:46:de:14", type=0x8864)
>     / PPPoE(version=1, type=1, code=0, sessionid=0x1234)
>     / PPP(proto=0x0021)
>     / IP(src="192.0.2.1", dst="192.0.2.2")
> )
> 
> sendp(packet, iface="igc1", count=1, verbose=False)
> 
> I ran the script as root and captured on gateway with:
> 
> doas tcpdump -n -e -vv -i bge0 'ether proto 0x8863 or ether proto 0x8864'
> 
> With:
> 
> doas ifconfig pppoe9 link2 down
> 
> I observed the injected frame followed by one PADT:
> 
> 06:56:46.165141 00:00:00:00:00:00 d0:bf:9c:46:de:14 8864 60: PPPoE-Session
>     code Session, version 1, type 1, id 0x1234, length 22
>     IP 192.0.2.1 > 192.0.2.2: ip-proto-0 0 (ttl 64, id 1, len 20)
> 06:56:46.165187 d0:bf:9c:46:de:14 00:00:00:00:00:00 8863 20: PPPoE-Discovery
>     code Terminate, version 1, type 1, id 0x1234, length 0
> 
> With:
> 
> doas ifconfig pppoe9 -link2 down
> 
> The same injected frame was captured, but no PADT was observed:
> 
> 06:57:09.493644 00:00:00:00:00:00 d0:bf:9c:46:de:14 8864 60: PPPoE-Session
>     code Session, version 1, type 1, id 0x1234, length 22
>     IP 192.0.2.1 > 192.0.2.2: ip-proto-0 0 (ttl 64, id 1, len 20)
> 
> So the updated patch does the right thing in these tests: link2 controls
> the termination response, and the interface being down does not prevent
> it. This verifies PADT generation, although I have not tested the AC
> releasing a stale session and accepting a new connection.
> 
> Thanks again for catching this and improving the diff.


Index: sys/net/if_pppoe.c
===================================================================
RCS file: /cvs/src/sys/net/if_pppoe.c,v
diff -u -p -r1.91 if_pppoe.c
--- sys/net/if_pppoe.c	17 Jun 2026 17:02:03 -0000	1.91
+++ sys/net/if_pppoe.c	14 Oct 2026 08:11:24 -0000
@@ -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,20 @@ 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_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;
 	}
 
Index: share/man/man4/pppoe.4
===================================================================
RCS file: /cvs/src/share/man/man4/pppoe.4,v
diff -u -p -r1.37 pppoe.4
--- share/man/man4/pppoe.4	17 Jun 2026 17:02:03 -0000	1.37
+++ share/man/man4/pppoe.4	14 Oct 2026 08:11:24 -0000
@@ -126,7 +126,8 @@ the gateway will be changed to the sugge
 .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
Index: share/man/man4/options.4
===================================================================
RCS file: /cvs/src/share/man/man4/options.4,v
diff -u -p -r1.273 options.4
--- share/man/man4/options.4	1 Jun 2025 00:32:54 -0000	1.273
+++ share/man/man4/options.4	14 Oct 2026 08:11:24 -0000
@@ -385,10 +385,6 @@ Enables BSD compressor for PPP connectio
 .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