Skip site navigation (1)Skip section navigation (2)
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>