Merge some sppp improvements from NetBSD:

1. better timeout and keepalive handling
2. fix some memory leaks on error paths.
3. use arc4random instead of random
4. always send keepalives in cHDLC mode, from claudio@
Tested by Greg Mortensen (san) and jmc@ (pppoe), ok claudio@

2 files changed, 116 insertions, 64 deletions

diff --git a/sys/net/if_sppp.h b/sys/net/if_sppp.h
index 24698cf32c8..b02f4c3d071 100644
--- a/sys/net/if_sppp.h
+++ b/sys/net/if_sppp.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_sppp.h,v 1.8 2005/01/08 20:35:59 canacar Exp $	*/
+/*	$OpenBSD: if_sppp.h,v 1.9 2005/03/23 00:26:06 canacar Exp $	*/
 /*	$NetBSD: if_sppp.h,v 1.2.2.1 1999/04/04 06:57:39 explorer Exp $	*/
 
 /*
@@ -109,6 +109,8 @@ struct sppp {
 	u_short pp_loopcnt;     /* loopback detection counter */
 	u_long  pp_seq;         /* local sequence number */
 	u_long  pp_rseq;        /* remote sequence number */
+	time_t	pp_last_receive;	/* peer's last "sign of life" */
+	time_t	pp_last_activity;	/* second of last payload data s/r */
 	enum ppp_phase pp_phase;	/* phase we're currently in */
 	int	state[IDX_COUNT];	/* state machine */
 	u_char  confid[IDX_COUNT];	/* id of last configuration request */
diff --git a/sys/net/if_spppsubr.c b/sys/net/if_spppsubr.c
index 263b18d98c1..9d25b0e8ee3 100644
--- a/sys/net/if_spppsubr.c
+++ b/sys/net/if_spppsubr.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_spppsubr.c,v 1.27 2005/03/23 00:11:40 canacar Exp $	*/
+/*	$OpenBSD: if_spppsubr.c,v 1.28 2005/03/23 00:26:06 canacar Exp $	*/
 /*
  * Synchronous PPP/Cisco link level subroutines.
  * Keepalive protocol implemented in both Cisco and PPP modes.
@@ -116,7 +116,10 @@
 # define UNTIMEOUT(fun, arg, handle)	\
 	untimeout(fun, arg)
 #endif
-#define MAXALIVECNT     3               /* max. alive packets */
+
+#define LOOPALIVECNT     		3	/* loopback detection tries */
+#define MAXALIVECNT    			3	/* max. missed alive packets */
+#define	NORECV_TIME			15	/* before we get worried */
 
 /*
  * Interface flags that can be set in an ifconfig command.
@@ -462,9 +465,12 @@ sppp_input(struct ifnet *ifp, struct mbuf *m)
 	struct sppp *sp = (struct sppp *)ifp;
 	int debug = ifp->if_flags & IFF_DEBUG;
 
-	if (ifp->if_flags & IFF_UP)
+	if (ifp->if_flags & IFF_UP) {
 		/* Count received bytes, add hardware framing */
 		ifp->if_ibytes += m->m_pkthdr.len + sp->pp_framebytes;
+		/* Note time of last receive */
+		sp->pp_last_receive = mono_time.tv_sec;
+	}
 
 	if (m->m_pkthdr.len <= PPP_HEADER_LEN) {
 		/* Too small packet, drop it. */
@@ -542,6 +548,7 @@ sppp_input(struct ifnet *ifp, struct mbuf *m)
 			if (sp->state[IDX_IPCP] == STATE_OPENED) {
 				schednetisr (NETISR_IP);
 				inq = &ipintrq;
+				sp->pp_last_activity = mono_time.tv_sec;
 			}
 			break;
 #endif
@@ -651,6 +658,8 @@ sppp_output(struct ifnet *ifp, struct mbuf *m,
 
 	s = splimp();
 
+	sp->pp_last_activity = mono_time.tv_sec;
+
 	if ((ifp->if_flags & IFF_UP) == 0 ||
 	    (ifp->if_flags & (IFF_RUNNING | IFF_AUTO)) == 0) {
 		m_freem (m);
@@ -865,6 +874,8 @@ sppp_attach(struct ifnet *ifp)
 	sp->pp_cpq.ifq_maxlen = 50;
 	sp->pp_loopcnt = 0;
 	sp->pp_alivecnt = 0;
+	sp->pp_last_activity = 0;
+	sp->pp_last_receive = 0;
 	sp->pp_seq = 0;
 	sp->pp_rseq = 0;
 	sp->pp_phase = PHASE_DEAD;
@@ -986,7 +997,7 @@ sppp_ioctl(struct ifnet *ifp, u_long cmd, void *data)
 {
 	struct ifreq *ifr = (struct ifreq*) data;
 	struct sppp *sp = (struct sppp*) ifp;
-	int s, rv, going_up, going_down;
+	int s, rv, going_up, going_down, newmode;
 
 	s = splimp();
 	rv = 0;
@@ -1000,28 +1011,27 @@ sppp_ioctl(struct ifnet *ifp, u_long cmd, void *data)
 		/* fall through... */
 
 	case SIOCSIFFLAGS:
+		going_up = (ifp->if_flags & IFF_UP) &&
+			(ifp->if_flags & IFF_RUNNING) == 0;
+		going_down = (ifp->if_flags & IFF_UP) == 0 &&
+			(ifp->if_flags & IFF_RUNNING);
+		newmode = ifp->if_flags & (IFF_AUTO | IFF_PASSIVE);
+		if (newmode == (IFF_AUTO | IFF_PASSIVE)) {
 			/* sanity */
-		if (ifp->if_flags & IFF_PASSIVE)
+			newmode = IFF_PASSIVE;
 			ifp->if_flags &= ~IFF_AUTO;
+		}
 
-		going_up = (ifp->if_flags & IFF_UP) && ! (ifp->if_flags &
-			(IFF_RUNNING | IFF_AUTO | IFF_PASSIVE));
-		going_down = ! (ifp->if_flags & IFF_UP) &&
-			(ifp->if_flags & IFF_RUNNING);
-
-		if (going_up || going_down) {
-			if (! (sp->pp_flags & PP_CISCO))
+		if (going_up || going_down)
 			lcp.Close(sp);
-			else {
-				sppp_flush(ifp);
-				ifp->if_flags &= ~IFF_RUNNING;
-			}
-		}
-		if (going_up) {
+		if (going_up && newmode == 0) {
 			/* neither auto-dial nor passive */
 			ifp->if_flags |= IFF_RUNNING;
 			if (!(sp->pp_flags & PP_CISCO))
 				lcp.Open(sp);
+		} else if (going_down) {
+			sppp_flush(ifp);
+			ifp->if_flags &= ~IFF_RUNNING;
 		}
 		break;
 
@@ -1114,7 +1124,7 @@ sppp_cisco_input(struct sppp *sp, struct mbuf *m)
 		if (sp->pp_seq == sp->pp_rseq) {
 			/* Local and remote sequence numbers are equal.
 			 * Probably, the line is in loopback mode. */
-			if (sp->pp_loopcnt >= MAXALIVECNT) {
+			if (sp->pp_loopcnt >= LOOPALIVECNT) {
 				printf (SPP_FMT "loopback\n",
 					SPP_ARGS(ifp));
 				sp->pp_loopcnt = 0;
@@ -1127,7 +1137,7 @@ sppp_cisco_input(struct sppp *sp, struct mbuf *m)
 
 			/* Generate new local sequence number */
 #if defined (__FreeBSD__) || defined (__NetBSD__) || defined(__OpenBSD__)
-			sp->pp_seq = random();
+			sp->pp_seq = arc4random();
 #else
 			sp->pp_seq ^= time.tv_sec ^ time.tv_usec;
 #endif
@@ -1905,16 +1915,19 @@ sppp_lcp_up(struct sppp *sp)
  	sp->lcp.magic = 0;
  	sp->lcp.protos = 0;
  	sp->lcp.mru = sp->lcp.their_mru = PP_MTU;
+
+	sp->pp_last_receive = sp->pp_last_activity = mono_time.tv_sec;
+
 	/*
 	 * If this interface is passive or dial-on-demand, and we are
 	 * still in Initial state, it means we've got an incoming
 	 * call.  Activate the interface.
 	 */
-	ifp->if_flags |= IFF_RUNNING;
 	if ((ifp->if_flags & (IFF_AUTO | IFF_PASSIVE)) != 0) {
 		if (debug)
 			log(LOG_DEBUG,
 			    SPP_FMT "Up event", SPP_ARGS(ifp));
+		ifp->if_flags |= IFF_RUNNING;
 		if (sp->state[IDX_LCP] == STATE_INITIAL) {
 			if (debug)
 				addlog("(incoming call)\n");
@@ -1922,6 +1935,10 @@ sppp_lcp_up(struct sppp *sp)
 			lcp.Open(sp);
 		} else if (debug)
 			addlog("\n");
+	} else if ((ifp->if_flags & (IFF_AUTO | IFF_PASSIVE)) == 0 &&
+		   (sp->state[IDX_LCP] == STATE_INITIAL)) {
+			ifp->if_flags |= IFF_RUNNING;
+			lcp.Open(sp);
 	}
 
 	sppp_up_event(&lcp, sp);
@@ -1932,12 +1949,27 @@ sppp_lcp_down(struct sppp *sp)
 {
 	STDDCL;
 
+	sppp_down_event(&lcp, sp);
+
+	/*
+	 * If this is neither a dial-on-demand nor a passive
+	 * interface, simulate an ``ifconfig down'' action, so the
+	 * administrator can force a redial by another ``ifconfig
+	 * up''.  XXX For leased line operation, should we immediately
+	 * try to reopen the connection here?
+	 */
+	if ((ifp->if_flags & (IFF_AUTO | IFF_PASSIVE)) == 0) {
 		if (debug)
-		log(LOG_DEBUG, SPP_FMT "Down event (carrier loss)\n",
+			log(LOG_DEBUG, SPP_FMT "Down event (carrier loss), "
+			    "taking interface down.", SPP_ARGS(ifp));
+		if_down(ifp);
+	} else {
+		if (debug)
+			log(LOG_DEBUG, SPP_FMT "Down event (carrier loss)\n",
 			    SPP_ARGS(ifp));
-  	sppp_down_event(&lcp, sp);
+	}
 
- 	if ((ifp->if_flags & (IFF_AUTO | IFF_PASSIVE)) != 0)
+	if (sp->state[IDX_LCP] != STATE_INITIAL)
 		lcp.Close(sp);
  	sp->pp_flags &= ~PP_CALLIN;
 	ifp->if_flags &= ~IFF_RUNNING;
@@ -2003,11 +2035,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp_header *h, int len)
 		switch (*p) {
 		case LCP_OPT_MAGIC:
 			/* Magic number. */
-			if (len >= 6 && p[1] == 6)
-				continue;
-			if (debug)
-				addlog("[invalid] ");
-			break;
+			/* fall through, both are same length */
 		case LCP_OPT_ASYNC_MAP:
 			/* Async control character map. */
 			if (len >= 6 && p[1] == 6)
@@ -2061,8 +2089,8 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp_header *h, int len)
 	if (rlen) {
 		if (debug)
 			addlog(" send conf-rej\n");
-		sppp_cp_send (sp, PPP_LCP, CONF_REJ, h->ident, rlen, buf);
-		return 0;
+		sppp_cp_send(sp, PPP_LCP, CONF_REJ, h->ident, rlen, buf);
+		goto end;
 	} else if (debug)
 		addlog("\n");
 
@@ -2154,21 +2182,18 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp_header *h, int len)
 		rlen += p[1];
 	}
 	if (rlen) {
-		if (++sp->fail_counter[IDX_LCP] < sp->lcp.max_failure) {
+		if (++sp->fail_counter[IDX_LCP] >= sp->lcp.max_failure) {
 			if (debug)
-				addlog("send conf-nak\n");
-			sppp_cp_send (sp, PPP_LCP, CONF_NAK, h->ident, rlen, buf);
-			return 0;
+				addlog(" max_failure (%d) exceeded, "
+				       "send conf-rej\n",
+				       sp->lcp.max_failure);
+			sppp_cp_send(sp, PPP_LCP, CONF_REJ, h->ident, rlen, buf);
+		} else {
+			if (debug)
+				addlog(" send conf-nak\n");
+			sppp_cp_send(sp, PPP_LCP, CONF_NAK, h->ident, rlen, buf);
 		}
-		if (debug)
-			addlog("max_failure (%d) exceeded, closing\n",
-			       sp->lcp.max_failure);
-		if (sp->pp_loopcnt >= MAXALIVECNT)
-			printf (SPP_FMT "loopback\n", SPP_ARGS(ifp));
-		lcp.Close(sp);
-		sp->fail_counter[IDX_LCP] = 0;
-		sp->pp_loopcnt = 0;
-		return 0;
+		goto end;
 	} else {
 		if (debug)
 			addlog("send conf-ack\n");
@@ -2178,7 +2203,8 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp_header *h, int len)
 			      h->ident, origlen, h+1);
 	}
 
-	free (buf, M_TEMP);
+ end:
+	free(buf, M_TEMP);
 	return (rlen == 0);
 }
 
@@ -2376,6 +2402,10 @@ sppp_lcp_tlu(struct sppp *sp)
 		if (sp->lcp.protos & mask && ((cps[i])->flags & CP_LCP) == 0)
 			(cps[i])->Up(sp);
 
+	/* notify low-level driver of state change */
+	if (sp->pp_chg)
+		sp->pp_chg(sp, (int)sp->pp_phase);
+
 	if (sp->pp_phase == PHASE_NETWORK)
 		/* if no NCP is starting, close down */
 		sppp_lcp_check_and_close(sp);
@@ -2445,7 +2475,7 @@ sppp_lcp_scr(struct sppp *sp)
 	if (sp->lcp.opts & (1 << LCP_OPT_MAGIC)) {
 		if (! sp->lcp.magic)
 #if defined (__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)
-			sp->lcp.magic = random();
+			sp->lcp.magic = arc4random();
 #else
 			sp->lcp.magic = time.tv_sec + time.tv_usec;
 #endif
@@ -2655,8 +2685,8 @@ sppp_ipcp_RCR(struct sppp *sp, struct lcp_header *h, int len)
 	if (rlen) {
 		if (debug)
 			addlog(" send conf-rej\n");
-		sppp_cp_send (sp, PPP_IPCP, CONF_REJ, h->ident, rlen, buf);
-		return 0;
+		sppp_cp_send(sp, PPP_IPCP, CONF_REJ, h->ident, rlen, buf);
+		goto end;
 	} else if (debug)
 		addlog("\n");
 
@@ -2707,12 +2737,12 @@ sppp_ipcp_RCR(struct sppp *sp, struct lcp_header *h, int len)
 				else
 					addlog("%s [not agreed] ",
 					       sppp_dotted_quad(desiredaddr));
-
-				p[2] = hisaddr >> 24;
-				p[3] = hisaddr >> 16;
-				p[4] = hisaddr >> 8;
-				p[5] = hisaddr;
 			}
+
+			p[2] = hisaddr >> 24;
+			p[3] = hisaddr >> 16;
+			p[4] = hisaddr >> 8;
+			p[5] = hisaddr;
 			break;
 		}
 		/* Add the option to nak'ed list. */
@@ -2754,7 +2784,8 @@ sppp_ipcp_RCR(struct sppp *sp, struct lcp_header *h, int len)
 			      h->ident, origlen, h+1);
 	}
 
-	free (buf, M_TEMP);
+ end:
+	free(buf, M_TEMP);
 	return (rlen == 0);
 }
 
@@ -3330,7 +3361,7 @@ sppp_chap_tlu(struct sppp *sp)
 		 * Compute the re-challenge timeout.  This will yield
 		 * a number between 300 and 810 seconds.
 		 */
-		i = 300 + ((unsigned)(random() & 0xff00) >> 7);
+		i = 300 + (arc4random() & 0x01fe);
 
 #if defined (__FreeBSD__)
 		sp->ch[IDX_CHAP] = timeout(chap.TO, (void *)sp, i * hz);
@@ -3842,8 +3873,10 @@ sppp_keepalive(void *dummy)
 {
 	struct sppp *sp;
 	int s;
+	time_t now;
 
 	s = splimp();
+	now = mono_time.tv_sec;
 	for (sp=spppq; sp; sp=sp->pp_next) {
 		struct ifnet *ifp = &sp->pp_if;
 
@@ -3857,24 +3890,41 @@ sppp_keepalive(void *dummy)
 		    sp->pp_phase < PHASE_AUTHENTICATE)
 			continue;
 
-		if (sp->pp_alivecnt == MAXALIVECNT) {
+		/* No echo reply, but maybe user data passed through? */
+		if (!(sp->pp_flags & PP_CISCO) &&
+		    (now - sp->pp_last_receive) < NORECV_TIME) {
+			sp->pp_alivecnt = 0;
+			continue;
+		}
+
+		if (sp->pp_alivecnt >= MAXALIVECNT) {
 			/* No keepalive packets got.  Stop the interface. */
-			printf (SPP_FMT "down\n", SPP_ARGS(ifp));
- 			if (sp->pp_flags & PP_CISCO) {
 			if_down (ifp);
 			sppp_qflush (&sp->pp_cpq);
- 			} else {
-				/* Shut down the PPP link. */
- 				lcp.Close(sp);
+			if (! (sp->pp_flags & PP_CISCO)) {
+				printf (SPP_FMT "LCP keepalive timeout",
+				    SPP_ARGS(ifp));
+				sp->pp_alivecnt = 0;
+
+				/* we are down, close all open protocols */
+				lcp.Close(sp);
+
+				/* And now prepare LCP to reestablish the link, if configured to do so. */
+				sppp_cp_change_state(&lcp, sp, STATE_STOPPED);
+
+				/* Close connection imediatly, completition of this
+				 * will summon the magic needed to reestablish it. */
+				sp->pp_tlf(sp);
+				continue;
 			}
 		}
-		if (sp->pp_alivecnt <= MAXALIVECNT)
+		if (sp->pp_alivecnt < MAXALIVECNT)
 			++sp->pp_alivecnt;
 		if (sp->pp_flags & PP_CISCO)
 			sppp_cisco_send (sp, CISCO_KEEPALIVE_REQ, ++sp->pp_seq,
 				sp->pp_rseq);
 		else if (sp->pp_phase >= PHASE_AUTHENTICATE) {
-			long nmagic = htonl (sp->lcp.magic);
+			unsigned long nmagic = htonl (sp->lcp.magic);
 			sp->lcp.echoid = ++sp->pp_seq;
 			sppp_cp_send (sp, PPP_LCP, ECHO_REQ,
 				sp->lcp.echoid, 4, &nmagic);