From owner-freebsd-hackers Thu Jan 11 21:22:19 1996 Return-Path: owner-hackers Received: (from root@localhost) by freefall.freebsd.org (8.7.3/8.7.3) id VAA28998 for hackers-outgoing; Thu, 11 Jan 1996 21:22:19 -0800 (PST) Received: from irbs.irbs.com (irbs.com [199.182.75.129]) by freefall.freebsd.org (8.7.3/8.7.3) with SMTP id VAA28989 for ; Thu, 11 Jan 1996 21:22:09 -0800 (PST) Received: (from jc@localhost) by irbs.irbs.com (8.6.12/8.6.6) id AAA27898 for freebsd-hackers@FreeBSD.org; Fri, 12 Jan 1996 00:21:36 -0500 From: John Capo Message-Id: <199601120521.AAA27898@irbs.irbs.com> Subject: Re: pppd vs ijppp (Bug Fixes) To: freebsd-hackers@FreeBSD.org Date: Fri, 12 Jan 1996 00:21:36 -0500 (EST) In-Reply-To: from "Michael C. Newell" at Jan 11, 96 11:06:53 pm X-Mailer: ELM [version 2.4 PL24] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-hackers@FreeBSD.org Precedence: bulk Two features iijppp has over kernel ppp that I like are predictor1 compression and demand dialing. Here are a few bug fixes. I expanded the priority queueing scheme and discovered it was broken due to the assignment at ip.c line 300. All packets were being queued at the same priority. Fixing priority queueing broke predictor1 compression. Packets were compressed before being queued and predictor1 worked as long as the packets were popped off the queue in the same order they were pushed onto the queue. There were a few byte order problems in IP header tests also. There is a recursion problem in SendLqrReport(). LcpClose() is called when "Too many echo packets are lost" which winds up in SendLqrReport() again. I believe the original intention was to just stop the LQR timer with the call to StopLqr() but the side effects hurt. #0 0xa2da in SendLqrReport () at lqr.c:121 #1 0xa5e5 in StopLqr (method=1) at lqr.c:227 #2 0x97d1 in LcpLayerDown (fp=0x17088) at lcp.c:375 #3 0x6046 in FsmClose (fp=0x17088) at fsm.c:185 #4 0x985d in LcpClose () at lcp.c:405 #5 0xa2da in SendLqrReport () at lqr.c:121 #6 0xa5e5 in StopLqr (method=1) at lqr.c:227 #7 0x97d1 in LcpLayerDown (fp=0x17088) at lcp.c:375 #8 0x6046 in FsmClose (fp=0x17088) at fsm.c:185 #9 0x985d in LcpClose () at lcp.c:405 #10 0xa2da in SendLqrReport () at lqr.c:121 #11 0xa5e5 in StopLqr (method=1) at lqr.c:227 #12 0x97d1 in LcpLayerDown (fp=0x17088) at lcp.c:375 #13 0x5fb6 in FsmDown (fp=0x17088) at fsm.c:164 #14 0x9829 in LcpDown () at lcp.c:391 #15 0xcfe3 in DownConnection () at modem.c:212 There is a send-pr black hole report on the recursion bug. I suspect another recursion problem somewhere that does not spit out a message. Symptoms are illegal instructions and a completely trashed core dump, 90% 0's. These patches were generated against supped -stable with some massaging. I may have ommitted something. I have Nate's "ddial" patch working too but it is not included here. Email if you are interested. John Capo jc@irbs.com IRBS Engineering High performance FreeBSD systems (305) 792-9551 Unix/Internet Consulting - ISP Solutions diff -c ../ppp/chap.c ./chap.c *** ../ppp/chap.c Mon May 29 23:50:28 1995 --- ./chap.c Thu Jan 11 21:55:38 1996 *************** *** 62,68 **** DumpBp(bp); #endif LogPrintf(LOG_LCP, "ChapOutput: %s\n", chapcodes[code]); ! HdlcOutput(PRI_NORMAL, PROTO_CHAP, bp); } --- 62,68 ---- DumpBp(bp); #endif LogPrintf(LOG_LCP, "ChapOutput: %s\n", chapcodes[code]); ! HdlcOutput(PRI_LINK, PROTO_CHAP, bp); } diff -c ../ppp/fsm.c ./fsm.c *** ../ppp/fsm.c Fri Oct 6 11:10:02 1995 --- ./fsm.c Thu Jan 11 21:55:39 1996 *************** *** 86,92 **** #ifdef DEBUG DumpBp(bp); #endif ! HdlcOutput(PRI_NORMAL, fp->proto, bp); } void --- 86,92 ---- #ifdef DEBUG DumpBp(bp); #endif ! HdlcOutput(PRI_LINK, fp->proto, bp); } void diff -c ../ppp/hdlc.h ./hdlc.h *** ../ppp/hdlc.h Sun Feb 26 07:17:31 1995 --- ./hdlc.h Thu Jan 11 21:55:39 1996 *************** *** 45,53 **** /* * Output priority */ #define PRI_NORMAL 0 /* Normal priority */ ! #define PRI_FAST 1 /* Fast (interructive) */ ! #define PRI_URGENT 2 /* Urgent (LQR packets) */ unsigned char EscMap[33]; --- 45,58 ---- /* * Output priority */ + /* PRI_NORMAL and PRI_FAST have meaning only on the IP queue. + * All IP frames have the same priority once they are compressed. + * IP frames stay on the IP queue till they can be sent on the + * link. They are compressed at that time. + */ #define PRI_NORMAL 0 /* Normal priority */ ! #define PRI_FAST 1 /* Fast (interractive) */ ! #define PRI_LINK 1 /* Urgent (LQR packets) */ unsigned char EscMap[33]; diff -c ../ppp/ip.c ./ip.c *** ../ppp/ip.c Fri Oct 6 11:10:03 1995 --- ./ip.c Thu Jan 11 22:14:50 1996 *************** *** 35,41 **** #include "filter.h" extern void SendPppFrame(); - extern int PacketCheck(); extern void LcpClose(); static struct pppTimer IdleTimer; --- 35,40 ---- *************** *** 80,90 **** } } ! static u_short interactive_ports[8] = { ! 0, 513, 0, 0, 0, 21, 0, 23, }; ! #define INTERACTIVE(p) (interactive_ports[(p) & 7] == (p)) static char *TcpFlags[] = { "FIN", "SYN", "RST", "PSH", "ACK", "URG", --- 79,90 ---- } } ! static u_short interactive_ports[32] = { ! 544, 513, 514, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ! 0, 0, 0, 0, 0, 21, 22, 23, 0, 0, 0, 0, 0, 0, 0, 543, }; ! #define INTERACTIVE(p) (interactive_ports[(p) & 0x1F] == (p)) static char *TcpFlags[] = { "FIN", "SYN", "RST", "PSH", "ACK", "URG", *************** *** 133,139 **** if (fp->action) { /* permit fragments on in and out filter */ if ((direction == FL_IN || direction == FL_OUT) && ! (pip->ip_off & IP_OFFMASK) != 0) { return(A_PERMIT); } #ifdef DEBUG --- 133,139 ---- if (fp->action) { /* permit fragments on in and out filter */ if ((direction == FL_IN || direction == FL_OUT) && ! (ntohs(pip->ip_off) & IP_OFFMASK) != 0) { return(A_PERMIT); } #ifdef DEBUG *************** *** 217,223 **** if (pip->ip_p != IPPROTO_ICMP) { bp = mballoc(cnt, MB_IPIN); bcopy(ptr, MBUF_CTOP(bp), cnt); ! SendPppFrame(PRI_URGENT, bp); RestartIdleTimer(); ipOutOctets += cnt; } --- 217,223 ---- if (pip->ip_p != IPPROTO_ICMP) { bp = mballoc(cnt, MB_IPIN); bcopy(ptr, MBUF_CTOP(bp), cnt); ! SendPppFrame(bp); RestartIdleTimer(); ipOutOctets += cnt; } *************** *** 269,275 **** th = (struct tcphdr *)ptop; if (pip->ip_tos == IPTOS_LOWDELAY) pri = PRI_FAST; ! else if (pip->ip_off == 0) { if (INTERACTIVE(ntohs(th->th_sport)) || INTERACTIVE(ntohs(th->th_dport))) pri = PRI_FAST; } --- 269,275 ---- th = (struct tcphdr *)ptop; if (pip->ip_tos == IPTOS_LOWDELAY) pri = PRI_FAST; ! else if ((ntohs(pip->ip_off) & IP_OFFMASK) == 0) { if (INTERACTIVE(ntohs(th->th_sport)) || INTERACTIVE(ntohs(th->th_dport))) pri = PRI_FAST; } *************** *** 297,304 **** } break; } ! pri = FilterCheck(pip, direction); ! if (pri & A_DENY) { #ifdef DEBUG logprintf("blocked.\n"); #endif --- 297,304 ---- } break; } ! ! if ((FilterCheck(pip, direction) & A_DENY)) { #ifdef DEBUG logprintf("blocked.\n"); #endif *************** *** 348,375 **** RestartIdleTimer(); } ! void ! IpOutput(ptr, cnt) ! u_char *ptr; /* IN: Pointer to IP packet */ ! int cnt; /* IN: Length of packet */ ! { ! struct mbuf *bp; ! int pri; ! ! if (IpcpFsm.state != ST_OPENED) ! return; ! ! pri = PacketCheck(ptr, cnt, FL_OUT); ! if (pri >= 0) { ! bp = mballoc(cnt, MB_IPIN); ! bcopy(ptr, MBUF_CTOP(bp), cnt); ! SendPppFrame(pri, bp); ! RestartIdleTimer(); ! ipOutOctets += cnt; ! } ! } ! ! static struct mqueue IpOutputQueues[PRI_URGENT+1]; void IpEnqueue(pri, ptr, count) --- 348,354 ---- RestartIdleTimer(); } ! static struct mqueue IpOutputQueues[PRI_FAST+1]; void IpEnqueue(pri, ptr, count) *************** *** 389,395 **** { struct mqueue *queue; int exist = FALSE; ! for (queue = &IpOutputQueues[PRI_URGENT]; queue >= IpOutputQueues; queue--) { if ( queue->qlen > 0 ) { exist = TRUE; break; --- 368,374 ---- { struct mqueue *queue; int exist = FALSE; ! for (queue = &IpOutputQueues[PRI_FAST]; queue >= IpOutputQueues; queue--) { if ( queue->qlen > 0 ) { exist = TRUE; break; *************** *** 407,421 **** if (IpcpFsm.state != ST_OPENED) return; ! pri = PRI_URGENT; ! for (queue = &IpOutputQueues[PRI_URGENT]; queue >= IpOutputQueues; queue--) { if (queue->top) { bp = Dequeue(queue); if (bp) { cnt = plength(bp); ! SendPppFrame(pri, bp); RestartIdleTimer(); ipOutOctets += cnt; } } pri--; --- 386,401 ---- if (IpcpFsm.state != ST_OPENED) return; ! pri = PRI_FAST; ! for (queue = &IpOutputQueues[PRI_FAST]; queue >= IpOutputQueues; queue--) { if (queue->top) { bp = Dequeue(queue); if (bp) { cnt = plength(bp); ! SendPppFrame(bp); RestartIdleTimer(); ipOutOctets += cnt; + break; } } pri--; diff -c ../ppp/lcp.c ./lcp.c *** ../ppp/lcp.c Fri Oct 6 11:10:05 1995 --- ./lcp.c Thu Jan 11 21:55:40 1996 *************** *** 327,332 **** --- 327,333 ---- StopIdleTimer(); StopTimer(&AuthPapInfo.authtimer); StopTimer(&AuthChapInfo.authtimer); + StopLqrTimer(); } static void *************** *** 372,378 **** { LogPrintf(LOG_LCP, "%s: LayerDown\n", fp->name); StopAllTimers(); - StopLqr( LQM_LQR ); OsLinkdown(); NewPhase(PHASE_TERMINATE); } --- 373,378 ---- diff -c ../ppp/lqr.c ./lqr.c *** ../ppp/lqr.c Mon May 29 23:50:44 1995 --- ./lqr.c Thu Jan 11 21:55:40 1996 *************** *** 107,125 **** /* * XXX: Should implement LQM strategy */ ! LogPrintf(LOG_PHASE, "** Too many ECHO packets are lost. **\n"); LcpClose(); - Cleanup(EX_ERRDEAD); } else { bp = mballoc(sizeof(struct lqrdata), MB_LQR); ! HdlcOutput(PRI_URGENT, PROTO_LQR, bp); lqrsendcnt++; } } else if (lqmmethod & LQM_ECHO) { if (echoseq - gotseq > 5) { ! LogPrintf(LOG_PHASE, "** Too many ECHO packets are lost. **\n"); LcpClose(); - Cleanup(EX_ERRDEAD); } else SendEchoReq(); } --- 107,125 ---- /* * XXX: Should implement LQM strategy */ ! LogPrintf(LOG_PHASE, "** 1 Too many ECHO packets are lost. **\n"); ! lqmmethod = 0; /* Prevent rcursion via LcpClose() */ LcpClose(); } else { bp = mballoc(sizeof(struct lqrdata), MB_LQR); ! HdlcOutput(PRI_LINK, PROTO_LQR, bp); lqrsendcnt++; } } else if (lqmmethod & LQM_ECHO) { if (echoseq - gotseq > 5) { ! LogPrintf(LOG_PHASE, "** 2 Too many ECHO packets are lost. **\n"); ! lqmmethod = 0; /* Prevent rcursion via LcpClose() */ LcpClose(); } else SendEchoReq(); } *************** *** 210,215 **** --- 210,221 ---- } else { LogPrintf(LOG_LQM, "LQR is not activated.\n"); } + } + + void + StopLqrTimer(void) + { + StopTimer(&LqrTimer); } void diff -c ../ppp/main.c ./main.c *** ../ppp/main.c Fri Oct 6 11:10:07 1995 --- ./main.c Thu Jan 11 22:01:03 1996 *************** *** 611,618 **** dial_up = FALSE; /* XXXX */ for (;;) { - if ( modem ) - IpStartOutput(); FD_ZERO(&rfds); FD_ZERO(&wfds); FD_ZERO(&efds); /* --- 611,616 ---- *************** *** 641,646 **** --- 639,650 ---- } } qlen = ModemQlen(); + + if (qlen == 0) { + IpStartOutput(); + qlen = ModemQlen(); + } + if (modem) { FD_SET(modem, &rfds); FD_SET(modem, &efds); diff -c ../ppp/modem.c ./modem.c *** ../ppp/modem.c Fri Oct 6 11:10:09 1995 --- ./modem.c Thu Jan 11 22:14:59 1996 *************** *** 52,60 **** #define Online (mbits & TIOCM_CD) static struct mbuf *modemout; ! static struct mqueue OutputQueues[PRI_URGENT+1]; static int dev_is_modem; void Enqueue(queue, bp) struct mqueue *queue; --- 52,62 ---- #define Online (mbits & TIOCM_CD) static struct mbuf *modemout; ! static struct mqueue OutputQueues[PRI_LINK+1]; static int dev_is_modem; + #undef QDEBUG + void Enqueue(queue, bp) struct mqueue *queue; *************** *** 624,629 **** --- 626,635 ---- bp = mballoc(count, MB_MODEM); bcopy(ptr, MBUF_CTOP(bp), count); + + /* Should be NORMAL and LINK only. + * All IP frames get here marked NORMAL. + */ Enqueue(&OutputQueues[pri], bp); } *************** *** 649,655 **** int len = 0; int i; ! for ( i = PRI_NORMAL; i <= PRI_URGENT; i ++ ) { queue = &OutputQueues[i]; len += queue->qlen; } --- 655,661 ---- int len = 0; int i; ! for ( i = PRI_NORMAL; i <= PRI_LINK; i ++ ) { queue = &OutputQueues[i]; len += queue->qlen; } *************** *** 664,676 **** struct mqueue *queue; int nb, nw, i; if (modemout == NULL) { ! i = 0; ! for (queue = &OutputQueues[PRI_URGENT]; queue >= OutputQueues; queue--) { if (queue->top) { modemout = Dequeue(queue); #ifdef QDEBUG ! if (i < 2) { struct mqueue *q; q = &OutputQueues[0]; --- 670,684 ---- struct mqueue *queue; int nb, nw, i; + if (modemout == NULL && ModemQlen() == 0) + IpStartOutput(); if (modemout == NULL) { ! i = PRI_LINK; ! for (queue = &OutputQueues[PRI_LINK]; queue >= OutputQueues; queue--) { if (queue->top) { modemout = Dequeue(queue); #ifdef QDEBUG ! if (i > PRI_NORMAL) { struct mqueue *q; q = &OutputQueues[0]; *************** *** 680,692 **** #endif break; } ! i++; } } if (modemout) { nb = modemout->cnt; if (nb > 1600) nb = 1600; ! if (fd == 0) fd = 1; nw = write(fd, MBUF_CTOP(modemout), nb); #ifdef QDEBUG logprintf("wrote: %d(%d)\n", nw, nb); --- 688,700 ---- #endif break; } ! i--; } } if (modemout) { nb = modemout->cnt; if (nb > 1600) nb = 1600; ! if (fd == 0) fd = 1; /* XXX WTFO! This is bogus */ nw = write(fd, MBUF_CTOP(modemout), nb); #ifdef QDEBUG logprintf("wrote: %d(%d)\n", nw, nb); diff -c ../ppp/pap.c ./pap.c *** ../ppp/pap.c Mon May 29 23:50:53 1995 --- ./pap.c Thu Jan 11 21:55:40 1996 *************** *** 67,73 **** *cp++ = keylen; bcopy(VarAuthKey, cp, keylen); ! HdlcOutput(PRI_NORMAL, PROTO_PAP, bp); } static void --- 67,73 ---- *cp++ = keylen; bcopy(VarAuthKey, cp, keylen); ! HdlcOutput(PRI_LINK, PROTO_PAP, bp); } static void *************** *** 92,98 **** *cp++ = mlen; bcopy(message, cp, mlen); LogPrintf(LOG_PHASE, "PapOutput: %s\n", papcodes[code]); ! HdlcOutput(PRI_NORMAL, PROTO_PAP, bp); } /* --- 92,98 ---- *cp++ = mlen; bcopy(message, cp, mlen); LogPrintf(LOG_PHASE, "PapOutput: %s\n", papcodes[code]); ! HdlcOutput(PRI_LINK, PROTO_PAP, bp); } /* diff -c ../ppp/pred.c ./pred.c *** ../ppp/pred.c Mon May 29 23:50:55 1995 --- ./pred.c Thu Jan 11 22:15:19 1996 *************** *** 153,159 **** *wp++ = fcs & 0377; *wp++ = fcs >> 8; mwp->cnt = wp - MBUF_CTOP(mwp); ! HdlcOutput(pri, PROTO_COMPD, mwp); } void --- 153,159 ---- *wp++ = fcs & 0377; *wp++ = fcs >> 8; mwp->cnt = wp - MBUF_CTOP(mwp); ! HdlcOutput(PRI_NORMAL, PROTO_COMPD, mwp); } void *************** *** 180,187 **** --- 180,189 ---- CcpInfo.compin += olen; len &= 0x7fff; if (len != len1) { /* Error is detected. Send reset request */ + LogPrintf(LOG_LCP, "%s: Length Error\n", CcpFsm.name); CcpSendResetReq(&CcpFsm); pfree(bp); + pfree(wp); return; } cp += olen - 4; *************** *** 196,202 **** *pp++ = *cp++; fcs = HdlcFcs(INITFCS, bufp, wp->cnt = pp - bufp); #ifdef DEBUG ! logprintf("fcs = %04x (%s), len = %x, olen = %x\n", fcs, (fcs == GOODFCS)? "good" : "bad", len, olen); #endif if (fcs == GOODFCS) { --- 198,205 ---- *pp++ = *cp++; fcs = HdlcFcs(INITFCS, bufp, wp->cnt = pp - bufp); #ifdef DEBUG ! if (fcs != GOODFCS) ! logprintf("fcs = 0x%04x (%s), len = 0x%x, olen = 0x%x\n", fcs, (fcs == GOODFCS)? "good" : "bad", len, olen); #endif if (fcs == GOODFCS) { *************** *** 213,218 **** --- 216,226 ---- proto = (proto << 8) | *pp++; } DecodePacket(proto, wp); + } + else + { + LogDumpBp(LOG_HDLC, "Bad FCS", wp); + pfree(wp); } pfree(bp); } diff -c ../ppp/vars.c ./vars.c *** ../ppp/vars.c Fri Oct 6 11:10:11 1995 --- ./vars.c Thu Jan 11 21:55:41 1996 *************** *** 29,35 **** #include "defs.h" char VarVersion[] = "Version 0.94"; ! char VarLocalVersion[] = "$Date: 1995/10/06 11:24:47 $"; /* * Order of conf option is important. See vars.h. --- 29,35 ---- #include "defs.h" char VarVersion[] = "Version 0.94"; ! char VarLocalVersion[] = "$Date: 1995/10/08 14:57:31 $"; /* * Order of conf option is important. See vars.h. *************** *** 48,54 **** struct pppvars pppVars = { DEF_MRU, 0, MODEM_SPEED, CS8, 180, 30, 3, ! MODEM_DEV, OPEN_PASSIVE, LOCAL_NO_AUTH, }; int --- 48,54 ---- struct pppvars pppVars = { DEF_MRU, 0, MODEM_SPEED, CS8, 180, 30, 3, ! REDIAL_PERIOD, 1, MODEM_DEV, OPEN_PASSIVE, LOCAL_NO_AUTH, }; int diff -c ../ppp/vjcomp.c ./vjcomp.c *** ../ppp/vjcomp.c Mon May 29 23:51:02 1995 --- ./vjcomp.c Thu Jan 11 21:55:41 1996 *************** *** 40,47 **** } void ! SendPppFrame(pri, bp) ! int pri; struct mbuf *bp; { int type; --- 40,46 ---- } void ! SendPppFrame(bp) struct mbuf *bp; { int type; *************** *** 74,80 **** } } else proto = PROTO_IP; ! HdlcOutput(pri, proto, bp); } static struct mbuf * --- 73,79 ---- } } else proto = PROTO_IP; ! HdlcOutput(PRI_NORMAL, proto, bp); } static struct mbuf *