On Thu, Oct 15, 2026 at 08:20:17AM +0200, renaud@openbsd.org wrote:
> 
> Any OK for this?
 
Diff looks ok to me. I don't use pppoe(4) so I can't test. Using an
ifconfig link flag for this is certainly an improvement over the config
option.

> 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
> 

-- 
:wq Claudio