ppp: close unit fd only after session cleanup finishes

destablish_ppp() closed the ppp unit fd (via
triton_md_unregister_handler(..., 1)) before calling
ap_session_finished(). Closing the fd releases the ppp unit index, so
the kernel can assign the same unit (and thus the same pppX ifname) to
a new session while the old session's cleanup is still running.

pppd_compat performs its cleanup from the EV_SES_FINISHED handler: it
runs the ip-down script (blocking the context until the script exits)
and then deletes radattr.pppX. If the unit index is reused in that
window, the ip-down script is executed with an IFNAME that now belongs
to a different, active session, and remove_radattr() deletes the
radattr file of that new session. External accounting/shaper scripts
that read radattr.pppX then fail for a live session.

Fix this by unregistering the unit fd handler without closing the fd
and closing it only after ap_session_finished() returns, unless the fd
was handed to the unit cache. This keeps the unit index reserved until
session cleanup has finished, so the ifname cannot be reused early.

The unit-cache path is not affected (the cached fd already keeps the
unit reserved); the race only hits configurations with unit-cache
disabled. Only PPP sessions (PPPoE/PPTP/L2TP/SSTP) go through this
path; IPoE is unaffected. Note the unit index is now held slightly
longer during teardown (while ip-down runs) - this is intentional.

Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

1 files changed, 10 insertions, 3 deletions

diff --git a/accel-pppd/ppp/ppp.c b/accel-pppd/ppp/ppp.c
index 55d08bb7..cd726e17 100644
--- a/accel-pppd/ppp/ppp.c
+++ b/accel-pppd/ppp/ppp.c
@@ -273,7 +273,7 @@ static void destablish_ppp(struct ppp_t *ppp)
 		if (ppp->ses.net != def_net) {
 			if (net->move_link(def_net, ppp->ses.ifindex)) {
 				log_ppp_warn("failed to attach to default namespace\n");
-				triton_md_unregister_handler(&ppp->unit_hnd, 1);
+				triton_md_unregister_handler(&ppp->unit_hnd, 0);
 				goto skip;
 			}
 			ppp->ses.net = def_net;
@@ -286,7 +286,7 @@ static void destablish_ppp(struct ppp_t *ppp)
 			strncpy(ifr.ifr_name, ppp->ses.ifname, IFNAMSIZ);
 			if (net->sock_ioctl(SIOCSIFNAME, &ifr)) {
 				log_ppp_warn("failed to rename ppp to default name\n");
-				triton_md_unregister_handler(&ppp->unit_hnd, 1);
+				triton_md_unregister_handler(&ppp->unit_hnd, 0);
 				goto skip;
 			}
 		}
@@ -299,11 +299,18 @@ static void destablish_ppp(struct ppp_t *ppp)
 		uc->fd = ppp->unit_fd;
 		uc->unit_idx = ppp->ses.unit_idx;
 	} else
-		triton_md_unregister_handler(&ppp->unit_hnd, 1);
+		triton_md_unregister_handler(&ppp->unit_hnd, 0);
 
 skip:
 	ap_session_finished(&ppp->ses);
 
+	/* The unit fd is closed only after session cleanup (ip-down scripts,
+	 * radattr removal) has finished, so the kernel cannot give the same
+	 * unit index (and thus ifname) to a new session while cleanup still
+	 * references the ifname. */
+	if (!uc)
+		close(ppp->unit_fd);
+
 	ppp->unit_fd = -1;
 
 	destroy_ppp_channel(ppp);