From owner-freebsd-ports-bugs@FreeBSD.ORG Sat Dec 8 08:30:01 2012 Return-Path: Delivered-To: freebsd-ports-bugs@smarthost.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 99987718 for ; Sat, 8 Dec 2012 08:30:01 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) by mx1.freebsd.org (Postfix) with ESMTP id 7D9458FC12 for ; Sat, 8 Dec 2012 08:30:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.5/8.14.5) with ESMTP id qB88U11M003234 for ; Sat, 8 Dec 2012 08:30:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.5/8.14.5/Submit) id qB88U1d2003233; Sat, 8 Dec 2012 08:30:01 GMT (envelope-from gnats) Date: Sat, 8 Dec 2012 08:30:01 GMT Message-Id: <201212080830.qB88U1d2003233@freefall.freebsd.org> To: freebsd-ports-bugs@FreeBSD.org Cc: From: Brett Glass Subject: Re: ports/173533: mpd5 PPTP server race condition with some clients X-BeenThere: freebsd-ports-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: Brett Glass List-Id: Ports bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 08 Dec 2012 08:30:01 -0000 The following reply was made to PR ports/173533; it has been noted by GNATS. From: Brett Glass To: bug-followup@FreeBSD.org, freebsd-prs@brettglass.com Cc: Subject: Re: ports/173533: mpd5 PPTP server race condition with some clients Date: Sat, 08 Dec 2012 01:26:56 -0700 Discovered some errors in my first patch; also realized that it wasn't in the usual format. So, I'm re-sending with corrections. Because altering the PPP state machine would technically violate the RFC (even though one might argue that the RFC is flawed because it does not provide for a delay), I've developed patches that do not alter the state machine code. Rather, the code in link.c is modified so that a delay can be introduced before the finite state machine which performs the LCP negotation is started. A new command, "link set lcp-delay {msec]", is added to set this delay, which is 50 ms by default and can be set to any value from 0 to 30000. (Experimentation has shown that the default of 50 ms is enough time for the routers we have tested to set up a PPTP, PPPoE, or L2TP call, so the problem we experienced is fixed by default. However, a longer delay may be desired in some cases.) If the peer attempts to initiate negotiations during the delay, the request is ignored but no harm is done. patch (mpdpatch.c) follows: *** link.h.orig Thu Dec 6 23:11:52 2012 --- link.h Sat Dec 8 01:14:03 2012 *************** *** 34,43 **** --- 34,47 ---- /* Default latency and bandwidth */ #define LINK_DEFAULT_BANDWIDTH 64000 /* 64k */ #define LINK_DEFAULT_LATENCY 2000 /* 2ms */ + /* Default delay before starting LCP configuration negotiation */ + #define LINK_DEFAULT_LCP_DELAY 50 /* 50 ms */ + #define LINK_MAX_LCP_DELAY 30000 /* 30 seconds */ + enum { LINK_ACTION_FORWARD, LINK_ACTION_BUNDLE, LINK_ACTION_DROP }; *************** *** 78,87 **** --- 82,92 ---- struct linkconf { u_short mtu; /* Initial MTU value */ u_short mru; /* Initial MRU value */ uint16_t mrru; /* Initial MRRU value */ uint32_t accmap; /* Initial ACCMAP value */ + uint16_t lcp_delay; /* Delay before initiating LCP */ short retry_timeout; /* FSM timeout for retries */ short max_redial; /* Max failed connect attempts */ short redial_delay; /* Failed connect retry time */ char *ident; /* LCP ident string */ struct optinfo options; /* Configured options */ *************** *** 148,157 **** --- 153,163 ---- PhysType type; /* Device type descriptor */ void *info; /* Type specific info */ MsgHandler pmsgs; /* Message channel */ struct pppTimer openTimer; /* Open retry timer */ + struct pppTimer lcpDelayTimer; /* LCP delay timer */ }; /* * VARIABLES *************** *** 197,202 **** const char *arg, ...); extern const char *LinkMatchAction(Link l, int stage, char *login); #endif - --- 203,207 ---- *** link.c.orig Thu Dec 6 23:11:46 2012 --- link.c Sat Dec 8 01:17:38 2012 *************** *** 41,50 **** --- 41,51 ---- SET_FSM_RETRY, SET_MAX_RETRY, SET_RETRY_DELAY, SET_MAX_CHILDREN, SET_KEEPALIVE, + SET_LCP_DELAY, SET_IDENT, SET_ACCEPT, SET_DENY, SET_ENABLE, SET_DISABLE, *************** *** 60,69 **** --- 61,71 ---- static int LinkSetCommand(Context ctx, int ac, char *av[], void *arg); static void LinkMsg(int type, void *cookie); static void LinkNgDataEvent(int type, void *cookie); static void LinkReopenTimeout(void *arg); + static void LinkLcpDelayTimeout(void *arg); /* * GLOBAL VARIABLES */ *************** *** 104,113 **** --- 106,117 ---- LinkSetCommand, NULL, 2, (void *) SET_MAX_CHILDREN }, { "keep-alive {secs} {max}", "LCP echo keep-alives", LinkSetCommand, NULL, 2, (void *) SET_KEEPALIVE }, { "ident {string}", "LCP ident string", LinkSetCommand, NULL, 2, (void *) SET_IDENT }, + { "lcp-delay {msec}", "Delay before LCP start", + LinkSetCommand, NULL, 2, (void *) SET_LCP_DELAY }, { "accept {opt ...}", "Accept option", LinkSetCommand, NULL, 2, (void *) SET_ACCEPT }, { "deny {opt ...}", "Deny option", LinkSetCommand, NULL, 2, (void *) SET_DENY }, { "enable {opt ...}", "Enable option", *************** *** 244,253 **** --- 248,278 ---- Log(LG_LINK, ("[%s] Link: UP event", l->name)); l->originate = PhysGetOriginate(l); Log(LG_PHYS2, ("[%s] Link: origination is %s", l->name, LINK_ORIGINATION(l->originate))); + + if (l->conf.lcp_delay == 0) { + LcpUp(l); + } else { + TimerStop(&l->lcpDelayTimer); + TimerInit(&l->lcpDelayTimer, "LcpOpen", + l->conf.lcp_delay, LinkLcpDelayTimeout, l); + TimerStart(&l->lcpDelayTimer); + Log(LG_LINK, ("[%s] Link: delaying %d ms before initiating LCP negotiation", + l->name, l->conf.lcp_delay)); + } + } + + static void + LinkLcpDelayTimeout(void *arg) + { + Link const l = (Link)arg; + + if (gShutdownInProgress) + return; + LcpUp(l); } /* * LinkDown() *************** *** 427,436 **** --- 452,462 ---- l->latency = LINK_DEFAULT_LATENCY; l->upReason = NULL; l->upReasonValid = 0; l->downReason = NULL; l->downReasonValid = 0; + l->conf.lcp_delay = LINK_DEFAULT_LCP_DELAY; Disable(&l->conf.options, LINK_CONF_CHAPMD5); Accept(&l->conf.options, LINK_CONF_CHAPMD5); Disable(&l->conf.options, LINK_CONF_CHAPMSv1); *************** *** 1260,1269 **** --- 1286,1296 ---- if (l->lcp.fsm.conf.echo_int == 0) Printf("disabled\r\n"); else Printf("every %d secs, timeout %d\r\n", l->lcp.fsm.conf.echo_int, l->lcp.fsm.conf.echo_max); + Printf("\tLCP delay : %d msec\r\n", l->conf.lcp_delay); Printf("\tIdent string : \"%s\"\r\n", l->conf.ident ? l->conf.ident : ""); if (l->tmpl) Printf("\tMax children : %d\r\n", l->conf.max_children); Printf("Link incoming actions:\r\n"); SLIST_FOREACH(a, &l->actions, next) { *************** *** 1563,1572 **** --- 1590,1611 ---- return(-1); l->lcp.fsm.conf.echo_int = atoi(av[0]); l->lcp.fsm.conf.echo_max = atoi(av[1]); break; + case SET_LCP_DELAY: + if (ac != 1) + return(-1); + val = atoi(*av); + if (val < 0) + Error("lcp-delay cannot be negative"); + else if (val > LINK_MAX_LCP_DELAY) + Error("max lcp-delay is %d", LINK_MAX_LCP_DELAY); + else + l->conf.lcp_delay = val; + break; + case SET_IDENT: if (ac != 1) return(-1); if (l->conf.ident != NULL) { Freee(l->conf.ident); *************** *** 1616,1621 **** assert(0); } return(0); } - --- 1655,1659 ----