SyncPPP IPCP/LCP loop problem and patch

SyncPPP IPCP/LCP loop problem and patch

[rjd]

I've hit a problem with the syncPPP module within Linux.

Under certain conditions (hard to quantify exactly, but try several 8Mbps
streams hitting a relatively slow, say 200MHz processor) the LCP/IPCP
negotiation hits the following loop.


    A side state           Packet               B side state

                                                ACK sent
                <--     LCP conf ACK
    Opened
                        IPCP conf REQ   -->

                <--     LCP conf REQ
    ACK sent
                        LCP conf ACK    -->
                                                Opened
                <--     IPCP conf REQ

                        LCP conf REQ    -->
                                                ACK sent
                <--     LCP conf ACK
    Opened
                        IPCP conf REQ   -->

                <--     LCP conf REQ
    ACK sent
                        LCP conf ACK    -->
                                                Opened
                <--     IPCP conf REQ

                        LCP conf REQ    -->
                                                ACK sent
    ...


Basically one end has reached the IPCP level before the other. When it sees
an incoming LCP packet the RFC1661 state machine drops back to LCP but the
responses generated take the other end up to the IPCP level. The result is
an endless storm of small packets back and forth generating "IPCP when still
waiting LCP finish" messages.

Flip-flopping between IPCP and LCP like this seems to defeat the normal LCP
timeouts. I've searched the archives and found several solutions none of
which I found particularly satisfying. The best was "the driver will
eventually drop a packet breaking the loop", sorry but I try not to drop
packets in my drivers :-) Most of the rest shortened LCP timeouts or did
something else that modified the timing and I guess moved the problem for a
particular system, none of them seemed general purpose.


My solution in the patch that follows is to detect the flip-flop using a
counter and then after three occurrences with no genuine IPCP traffic to
modify behavior on receipt of the LCP conf REQ. After three attempts we
acknowledge the LCP conf REQ but stay in the opened state rather than
dropping back and restarting our own LCP negotiation. This is non-RFC1661
behavior unless you consider it part of the general loop avoidance directive.

I've tested this patch against an unmodified Linux system (where I first saw
the problem), a modified system (would be no good if it didn't work against
itself), NT RRAS and a small Cisco router. Of course I can't hit all timing
variants but it looks solid to me.


--- linux/include/net/syncppp.h.orig	Mon May 21 11:01:29 2001
+++ linux/include/net/syncppp.h	Mon May 21 11:01:28 2001
@@ -48,6 +48,7 @@
 	struct timer_list	pp_timer;
 	struct net_device	*pp_if;
 	char		pp_link_state;	/* Link status */
+	char		ipcp_b4_lcp;	/* IPCP up error counter */
 };
 
 struct ppp_device
--- linux/drivers/net/wan/syncppp.c.orig	Mon May 21 11:01:28 2001
+++ linux/drivers/net/wan/syncppp.c	Mon May 21 11:01:25 2001
@@ -262,10 +262,14 @@
 			kfree_skb(skb);
 			return;
 		case PPP_IPCP:
-			if (sp->lcp.state == LCP_STATE_OPENED)
+			if (sp->lcp.state == LCP_STATE_OPENED) {
+				sp->ipcp_b4_lcp = 0;
 				sppp_ipcp_input (sp, skb);
-			else
+			}
+			else {
 				printk(KERN_DEBUG "IPCP when still waiting LCP finish.\n");
+				sp->ipcp_b4_lcp++;
+			}
 			kfree_skb(skb);
 			return;
 		case PPP_IP:
@@ -529,6 +533,16 @@
 			sppp_ipcp_open (sp);
 			break;
 		case LCP_STATE_OPENED:
+			/* If it looks like we're looping in a IPCP<->LCP flip-
+			 * flop. ACK this one and stay open. Warning this is
+			 * non-RFC1661 behaviour.
+			 */
+			if (sp->ipcp_b4_lcp > 3) {
+				printk (KERN_DEBUG "IPCP<->LCP loop avoidance\n");
+				sp->ipcp_b4_lcp = 0;
+				break;
+			}
+
 			/* Remote magic changed -- close session. */
 			sp->lcp.state = LCP_STATE_CLOSED;
 			sp->ipcp.state = IPCP_STATE_CLOSED;
-- 
        Bob Dunlop                      FarSite Communications
        rjd@xyzzy.clara.co.uk           bob.dunlop@farsite.co.uk
        www.xyzzy.clara.co.uk           www.farsite.co.uk

Re: SyncPPP IPCP/LCP loop problem and patch

[Paul Mackerras]

rjd@xyzzy.clara.co.uk writes:

> I've hit a problem with the syncPPP module within Linux.
> 
> Under certain conditions (hard to quantify exactly, but try several 8Mbps
> streams hitting a relatively slow, say 200MHz processor) the LCP/IPCP
> negotiation hits the following loop.

[snip]

> My solution in the patch that follows is to detect the flip-flop using a
> counter and then after three occurrences with no genuine IPCP traffic to
> modify behavior on receipt of the LCP conf REQ. After three attempts we
> acknowledge the LCP conf REQ but stay in the opened state rather than
> dropping back and restarting our own LCP negotiation. This is non-RFC1661
> behavior unless you consider it part of the general loop avoidance directive.

Seems to me that when you get the conf-request in opened state, you
should send your conf-request before sending the conf-ack to the
peer's conf-request.  I think this would short-circuit the loop (I
could be wrong though, it's getting late).

That behaviour would be in line with the FSM in rfc1661, where the
action for event RCR+ in Opened state is "tld,scr,sca/8", i.e. the one
action involves sending both the conf-request and the conf-ack.  It is
debatable to what extent that specifies the order of the messages but
it does list the conf-request first FWIW.

Paul.

Re: SyncPPP IPCP/LCP loop problem and patch

[rjd]

Paul Mackerras wrote:
> 
> rjd@xyzzy.clara.co.uk writes:
> > I've hit a problem with the syncPPP module within Linux.
> 
> Seems to me that when you get the conf-request in opened state, you
> should send your conf-request before sending the conf-ack to the
> peer's conf-request.  I think this would short-circuit the loop (I
> could be wrong though, it's getting late).

Thanks but I've already tried that. You get a slightly different pattern
to the loop but it still loops.

> That behaviour would be in line with the FSM in rfc1661, where the
> action for event RCR+ in Opened state is "tld,scr,sca/8", i.e. the one
> action involves sending both the conf-request and the conf-ack.  It is
> debatable to what extent that specifies the order of the messages but
> it does list the conf-request first FWIW.

RFC1661 states: multiple actions may be implemented in any convenient order.
So if it had worked we'd still have been conformant.


I've also tried dropping selected packets when I see the condition, simulating
a noisy line or buggy driver whilst keeping the PPP state machine conformant.
Problem is you need to generate the conf-ack to get the remote end into the
LCP opened state whilst not dropping out of it yourself.

-- 
        Bob Dunlop                      FarSite Communications
        rjd@xyzzy.clara.co.uk           bob.dunlop@farsite.co.uk
        www.xyzzy.clara.co.uk           www.farsite.co.uk

Re: SyncPPP IPCP/LCP loop problem and patch

[Paul Fulghum]

> > Seems to me that when you get the conf-request in opened state, you
> > should send your conf-request before sending the conf-ack to the
> > peer's conf-request.  I think this would short-circuit the loop (I
> > could be wrong though, it's getting late).
> 
> Thanks but I've already tried that. You get a slightly different pattern
> to the loop but it still loops.

What does the loop look like when the cfg-req is sent 1st?

Paul Fulghum paulkf@microgate.com
Microgate Corporation www.microgate.com

SyncPPP IPCP/LCP loop problem and patch. Take 2

[rjd]

Paul Fulghum wrote:
> > 
> > Thanks but I've already tried that. You get a slightly different pattern
> > to the loop but it still loops.
> 
> What does the loop look like when the cfg-req is sent 1st?

Thanks for making me go back and look at it again guys. The reordering of
REQ and ACK does work to break the loop. Provided you apply the patch to
both ends of the link.

This is a much smaller patch, remains RFC1661 conformant and works against
all the platforms I've been able to test. Against an unpatched Linux it
still loops but no worse than before.


--- syncppp.c-orig	Mon May 21 10:35:59 2001
+++ syncppp.c	Thu May 24 15:55:55 2001
@@ -517,8 +517,10 @@
 		}
 		/* Send Configure-Ack packet. */
 		sp->pp_loopcnt = 0;
-		sppp_cp_send (sp, PPP_LCP, LCP_CONF_ACK,
-				h->ident, len-4, h+1);
+		if (sp->lcp.state != LCP_STATE_OPENED) {
+			sppp_cp_send (sp, PPP_LCP, LCP_CONF_ACK,
+					h->ident, len-4, h+1);
+		}
 		/* Change the state. */
 		switch (sp->lcp.state) {
 		case LCP_STATE_CLOSED:
@@ -534,7 +536,9 @@
 			sp->ipcp.state = IPCP_STATE_CLOSED;
 			/* Initiate renegotiation. */
 			sppp_lcp_open (sp);
-			/* An ACK has already been sent. */
+			/* Send ACK after our REQ in attempt to break loop */
+			sppp_cp_send (sp, PPP_LCP, LCP_CONF_ACK,
+					h->ident, len-4, h+1);
 			sp->lcp.state = LCP_STATE_ACK_SENT;
 			break;
 		}




A colleage has suggested that we should apply the same ordering in the NAK
path and for similar code in the stopped state.  Since I've not seen loops in
these areas I'm loath to make changes without the ability to test.
-- 
        Bob Dunlop                      FarSite Communications
        rjd@xyzzy.clara.co.uk           bob.dunlop@farsite.co.uk
        www.xyzzy.clara.co.uk           www.farsite.co.uk

Re: SyncPPP IPCP/LCP loop problem and patch

[Paul Fulghum]

> > Seems to me that when you get the conf-request in opened state, you
> > should send your conf-request before sending the conf-ack to the
> > peer's conf-request.  I think this would short-circuit the loop (I
> > could be wrong though, it's getting late).
> 
> Thanks but I've already tried that. You get a slightly different pattern
> to the loop but it still loops.

I'm just wondering if the loop that results when
the cfg-req is sent 1st might be a results of 
syncppp not processing a cfg-ack properly when
in the opened state.

RFC1661 state table shows a transition to req-sent
from opened when a (properly formated with 
correct sequence ID) cfg-ack is received.

Syncppp does not do this (from sppp_lcp_input):

 case LCP_CONF_ACK:
  if (h->ident != sp->lcp.confid)
   break;
  sppp_clear_timeout (sp);
  if ((sp->pp_link_state != SPPP_LINK_UP) &&
      (dev->flags & IFF_UP)) {
   /* Coming out of loopback mode. */
   sp->pp_link_state=SPPP_LINK_UP;
   printk (KERN_INFO "%s: protocol up\n", dev->name);
  }
  switch (sp->lcp.state) {
  case LCP_STATE_CLOSED:
   sp->lcp.state = LCP_STATE_ACK_RCVD;
   sppp_set_timeout (sp, 5);
   break;
  case LCP_STATE_ACK_SENT:
   sp->lcp.state = LCP_STATE_OPENED;
   sppp_ipcp_open (sp);
   break;
  }
  break;

Maybe adding:

  case LCP_STATE_OPENED:
   sppp_lcp_open (sp);
   sp->ipcp.state = IPCP_STATE_CLOSED;
   sp->lcp.state = LCP_STATE_REQ_SENT;
   break;

to the above switch statement in addition to
the cfg-req 1st change would cure both loops.

Paul Fulghum paulkf@microgate.com
Microgate Corporation www.microgate.com

Re: SyncPPP IPCP/LCP loop problem and patch

[rjd]

Paul Fulghum wrote:
> 
> RFC1661 state table shows a transition to req-sent
> from opened when a (properly formated with 
> correct sequence ID) cfg-ack is received.
> 
> Syncppp does not do this (from sppp_lcp_input):
...
> Maybe adding:
> 
>   case LCP_STATE_OPENED:
>    sppp_lcp_open (sp);
>    sp->ipcp.state = IPCP_STATE_CLOSED;
>    sp->lcp.state = LCP_STATE_REQ_SENT;
>    break;

Thanks for the suggestion. I tried it and found that Linux syncppp does not
have a LCP_STATE_REQ_SENT the nearest alternate being LCP_STATE_CLOSED.
Looking at the RFC these are by no means compatable. Adding the extra state
and coding the transitions would not be difficult but as I've looked at it
I've seen more and more ommissions in the code. The magic number is not
randomised very well, jiffies is not a great random number :-)  Parameter
negotiation only takes place for the magic number plus dummys for MRU and
ACCM, I'm not even sure that enforcing and ACCM of all zeros is required.
If the remote end sends us a config request without a magic number we end
up testing an uninitialised variable, and so on.

Who's the owner for syncppp.c ?  I might be able to put some time in on
it, but would hate to be sending patches into empty space.

-- 
        Bob Dunlop                      FarSite Communications
        rjd@xyzzy.clara.co.uk           bob.dunlop@farsite.co.uk
        www.xyzzy.clara.co.uk           www.farsite.co.uk

Re: SyncPPP IPCP/LCP loop problem and patch

[Paul Fulghum]

From: <rjd@xyzzy.clara.co.uk>
> Linux syncppp does not have a LCP_STATE_REQ_SENT ...
[snip]
> Who's the owner for syncppp.c ?  I might be able to put some time in on
> it, but would hate to be sending patches into empty space.

I'm not sure anyone is willing to claim ownership :)

I did not realize that syncppp does not implement
all the RFC1661 states. That's simply broken :(
A proper state machine implementation would be nice.

On the other hand, it works in a minimal way
for most people and it's supposed to be folded
into the generic PPP implementation someday.
So there's not much point in trying to overhaul the code.

Paul Fulghum paulkf@microgate.com
Microgate Corporation www.microgate.com

Re: SyncPPP IPCP/LCP loop problem and patch

[Alan Cox]

> I did not realize that syncppp does not implement
> all the RFC1661 states. That's simply broken :(
> A proper state machine implementation would be nice.

syncppp.c -predates- RFC1661 I believe.

> On the other hand, it works in a minimal way
> for most people and it's supposed to be folded
> into the generic PPP implementation someday.
> So there's not much point in trying to overhaul the code.

I had hoped for 2.4 to use generic ppp with it. That might be the more
productive way to attack the problem

SyncPPP Generic PPP merge

[Paul Fulghum]

From: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
> I had hoped for 2.4 to use generic ppp with it. That might be the more
> productive way to attack the problem.

Generic PPP requires the user mode pppd to handle
the LCP and NCPs, while syncppp implements these in
the kernel.

Instead of using ifconfig to bring an interface
up or down, the user must now work with pppd. And the net
device naming changes (allocated by ppp_generic.c instead
of using the net device allocated by low level driver).

I have no problem with this, but some people might
not be happy with the change.

Is the plan to *replace* the PPP code in syncppp
(hopefully in a way that is invisible to the
low level drivers)?

Or is it to *add* generic PPP support to syncppp,
leaving (at least temporarily) the existing PPP 
capability in syncppp for compatibility?
(implying a new syncppp flag USE_GENERIC_PPP?)

Paul Fulghum paulkf@microgate.com
Microgate Corporation www.microgate.com

Re: SyncPPP Generic PPP merge

[Alan Cox]

> Instead of using ifconfig to bring an interface
> up or down, the user must now work with pppd. And the net
> device naming changes (allocated by ppp_generic.c instead
> of using the net device allocated by low level driver).

I suspect that bit can be fixed if need be. Its nice to keep a constant 
naming between cisco/ppp modes. cisco/ppp autodetect is also possible and would
be rather nice to support 

> Or is it to *add* generic PPP support to syncppp,
> leaving (at least temporarily) the existing PPP 
> capability in syncppp for compatibility?
> (implying a new syncppp flag USE_GENERIC_PPP?)

Assuming this is a 'when 2.5 starts' discussion I'd like initially to keep the
syncppp api is but the pppd code going via generic ppp - and yes it would
break configs.

Clearly thats not 2.4 acceptable

Re: SyncPPP Generic PPP merge

[Jeff Mcadams]

Also sprach Alan Cox
>> Instead of using ifconfig to bring an interface up or down, the user
>> must now work with pppd. And the net device naming changes (allocated
>> by ppp_generic.c instead of using the net device allocated by low
>> level driver).

>I suspect that bit can be fixed if need be. Its nice to keep a constant
>naming between cisco/ppp modes. cisco/ppp autodetect is also possible
>and would be rather nice to support 

Indeed.  And let me just throw out another thought.  A clean abstraction
of the various portions of the PPP functionality is beneficial in other
ways.  My personal pet project being to add L2TP support to the kernel
eventually.  A good abstraction of the framing capabilities and basic
PPP processing would be rather useful in that project.

>> Or is it to *add* generic PPP support to syncppp, leaving (at least
>> temporarily) the existing PPP capability in syncppp for
>> compatibility?  (implying a new syncppp flag USE_GENERIC_PPP?)

>Assuming this is a 'when 2.5 starts' discussion I'd like initially to
>keep the syncppp api is but the pppd code going via generic ppp - and
>yes it would break configs.

>Clearly thats not 2.4 acceptable

I would agree that such a project would be 2.5 material.

I'll try to keep up with things on the list, but if this goes off-list,
I would appreciate being kept in the loop if possible.  :)  Thanks!
-- 
Jeff McAdams                            Email: jeffm@iglou.com
Head Network Administrator              Voice: (502) 966-3848
IgLou Internet Services                        (800) 436-4456

Re: SyncPPP Generic PPP merge

[Paul Mackerras]

Jeff Mcadams writes:

> Indeed.  And let me just throw out another thought.  A clean abstraction
> of the various portions of the PPP functionality is beneficial in other
> ways.  My personal pet project being to add L2TP support to the kernel
> eventually.  A good abstraction of the framing capabilities and basic
> PPP processing would be rather useful in that project.

That is exactly what ppp_generic.c is intended to do - it abstracts
out the framing and encapsulation and low-level transport of PPP
frames into ppp "channels" (see for example ppp_async.c,
ppp_synctty.c) while ppp_generic.c does the basic PPP processing
(compression, multilink, handling the network interface device etc.).

You should be able to write an L2TP channel to work with ppp_generic -
all your code would need to know about is how to take a PPP frame and
encapsulate and send it, and how to receive and decapsulate PPP
frames.

[Note to myself: send in a Documentation/ppp_generic.txt which
describes the interface between ppp_generic.c and the channels.]

> I would agree that such a project would be 2.5 material.

Do it today if you like, I can't see that adding a new PPP channel
could break anything else, it would be like adding a new driver.

Paul.

Re: SyncPPP Generic PPP merge

[Jeff Mcadams]

Also sprach Paul Mackerras
>Jeff Mcadams writes:
>> Indeed.  And let me just throw out another thought.  A clean
>> abstraction of the various portions of the PPP functionality is
>> beneficial in other ways.  My personal pet project being to add L2TP
>> support to the kernel eventually.  A good abstraction of the framing
>> capabilities and basic PPP processing would be rather useful in that
>> project.

>That is exactly what ppp_generic.c is intended to do - it abstracts out
>the framing and encapsulation and low-level transport of PPP frames
>into ppp "channels" (see for example ppp_async.c, ppp_synctty.c) while
>ppp_generic.c does the basic PPP processing (compression, multilink,
>handling the network interface device etc.).

>You should be able to write an L2TP channel to work with ppp_generic -
>all your code would need to know about is how to take a PPP frame and
>encapsulate and send it, and how to receive and decapsulate PPP frames.

>[Note to myself: send in a Documentation/ppp_generic.txt which
>describes the interface between ppp_generic.c and the channels.]

That's all well and good...and useful...and I'll probably take advantage
of it...but I think you missed my point (although I have no doubt that
my point was not at all clear!)

L2TP potentially has to convert a PPP frame that is given to it to or
from async or sync framing.  ppp_async.c has a ppp_async_encode()
function that would be useful to me in L2TP...it'd kinda suck to have to
duplicate that functionality.  Although it looks like the corresponding
decoding funcationality is tied up in ppp_async_input() which kinda
sucks a bit as well.

Anyway...the idea being that it would be nice to be able to have easy
access to some of the functionality that's part of the channels
currently, but could conceivably be abstracted out even further.

The question, I guess, then becomes whether its worth it to do this
extra layer of abstraction and adding another layer of interaction
between modules for the benefit that it provides.  I certainly wouldn't
squabble if the general concensus was that it wasn't worth it.  :)

Oh, and before anyone points this out, yes, L2TP changing the framing of
the PPP frame above is definitely a violation of layering...if you'd
like, I can point out plenty of other places where L2TP violates
layering principles.  :)

>> I would agree that such a project would be 2.5 material.

>Do it today if you like, I can't see that adding a new PPP channel
>could break anything else, it would be like adding a new driver.

Well...L2TP support is 2.5 material as far as I'm concerned because I'm
not much of a programmer in general, and have never done any kernel
programming...so I'm still getting up to speed on things, and it'll
probably take me until fairly well into 2.5 to even really consider
unleashing a kernel-level creation of my own onto the world.  :)
-- 
Jeff McAdams                            Email: jeffm@iglou.com
Head Network Administrator              Voice: (502) 966-3848
IgLou Internet Services                        (800) 436-4456

Re: SyncPPP Generic PPP merge

[Paul Fulghum]

From: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
> I suspect that bit can be fixed if need be. Its nice to keep a constant
> naming between cisco/ppp modes. cisco/ppp autodetect is also possible and
would
> be rather nice to support

I can't see getting around moving the net device allocation out of each
low level driver and into the layer above it. That removes
duplicate code and allows syncppp to do the right thing (allocate it itself
for cisco or allow ppp_generic.c do it for PPP). This would make
net device naming consistancy easier.

The low level drivers should not need any awareness of the net device
(possibly other than suggesting a name) as all they do is send and
receive frames.

The rest of the syncppp API should be (re)usable.

> Assuming this is a 'when 2.5 starts' discussion I'd like initially to keep
the
> syncppp api is but the pppd code going via generic ppp - and yes it would
> break configs.
>
> Clearly thats not 2.4 acceptable

Agreed, but it's a good time to call the cats to the herding pen.

Paul Fulghum paulkf@microgate.com
Microgate Corporation http://www.microgate.com