Date: Sat, 8 Dec 2012 08:30:01 GMT From: Brett Glass <freebsd-prs@brettglass.com> To: freebsd-ports-bugs@FreeBSD.org Subject: Re: ports/173533: mpd5 PPTP server race condition with some clients Message-ID: <201212080830.qB88U1d2003233@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR ports/173533; it has been noted by GNATS.
From: Brett Glass <freebsd-prs@brettglass.com>
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 ----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201212080830.qB88U1d2003233>