Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 27 Dec 2011 23:22:38 +0400
From:      Gleb Smirnoff <glebius@freebsd.org>
To:        Sami Halabi <sodynet1@gmail.com>
Cc:        net@freebsd.org
Subject:   Re: ng_mppc_decompress: too many (4094) packets dropped, disabling node
Message-ID:  <20111227192238.GV8035@glebius.int.ru>
In-Reply-To: <CAEW%2BogY_iHUb=n=G45d5U_r5XfD39YDwgNkowu1QN%2BeWL5K5Fw@mail.gmail.com>
References:  <CAEW%2Bogbn6jizawgLCHCcTLMSmdjCKFvPkJa33jrJ5AAnjww=fg@mail.gmail.com> <20111227044754.GK8035@FreeBSD.org> <CAEW%2BogY_iHUb=n=G45d5U_r5XfD39YDwgNkowu1QN%2BeWL5K5Fw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help 

--wq9mPyueHGvFACwf
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline

On Tue, Dec 27, 2011 at 09:44:23AM +0200, Sami Halabi wrote:
S> >1) Is the number always 4094?
S> 
S> No, i see 4092, 4093 also:
S> Dec 24 09:17:04 mpd2 kernel: ng_mppc_decompress: too many (4092) packets
S> dropped
S> , disabling node 0xffffff003051e400!
S> Dec 24 09:17:04 mpd2 kernel:
S> Dec 24 14:22:45 mpd2 kernel: ng_mppc_decompress: too many (4093) packets
S> dropped
S> , disabling node 0xffffff003d53db00!
S> Dec 24 14:22:45 mpd2 kernel:
S> Dec 24 19:28:45 mpd2 kernel: ng_mppc_decompress: too many (4092) packets
S> dropped
S> , disabling node 0xffffff00304e8500!

Well, here is my histrogram of probability:

  38 4094
   5 4093
   3 4095
   1 4092
   1 4091
   1 4087
   1 4083
   1 3275
   1 2173
   1 2172
   1 2171
   1 2170
   1 2169
   1 2137
   1 2135
   1 2132
   1 2122
   1 2121
   1 2120
   1 1130
   1 1013

I believe that problem is caused by re-ordering of packets that may happen
on the Internet. We definitely didn't lose 4094 packets so often.

Shift of 4095 means that we received a packet that should be the previous
one. Shift of 4094 means that we received a packet, that should have been
two packets ago. I have no idea why this condition is much more probable :(

Today I thought of some patch that would detect and fix reordering, but
failed to find any elegant way on fixing this MPPE poor design.

So, I have decided to remove the protection at all. The decision is based on
the following facts:

1) Our current limit of 1000 isn't by an order of magnitude greater than
   maximum possible rekeying number - 4095. So, the DoS protection is quite
   not really a noticable one.
2) Since ng_mppc was developed CPUs got faster by more than an order of
   magnitude.
3) Linux implementations do as much rekeying as needed.
4) It looks like Windows does too. Not very clear from the article, but
   out of ordering is mentioned here:

   http://technet.microsoft.com/en-us/library/cc958061.aspx

I suggest the attached patch. Can you please test it for a period
of time and report how it goes?

I am going to try it, too.

-- 
Totus tuus, Glebius.

--wq9mPyueHGvFACwf
Content-Type: text/x-diff; charset=koi8-r
Content-Disposition: attachment; filename="ng_mppc.c.diff"

Index: ng_mppc.c
===================================================================
--- ng_mppc.c	(revision 228916)
+++ ng_mppc.c	(working copy)
@@ -98,15 +98,6 @@
 /* Key length */
 #define KEYLEN(b)		(((b) & MPPE_128) ? 16 : 8)
 
-/*
- * When packets are lost with MPPE, we may have to re-key arbitrarily
- * many times to 'catch up' to the new jumped-ahead sequence number.
- * Since this can be expensive, we pose a limit on how many re-keyings
- * we will do at one time to avoid a possible D.O.S. vulnerability.
- * This should instead be a configurable parameter.
- */
-#define MPPE_MAX_REKEY		1000
-
 /* MPPC packet header bits */
 #define MPPC_FLAG_FLUSHED	0x8000		/* xmitter reset state */
 #define MPPC_FLAG_RESTART	0x4000		/* compress history restart */
@@ -641,20 +632,13 @@
 #endif
 #ifdef NETGRAPH_MPPC_ENCRYPTION
 		if ((d->cfg.bits & MPPE_BITS) != 0) {
-			u_int rekey;
 
-			/* How many times are we going to have to re-key? */
-			rekey = ((d->cfg.bits & MPPE_STATELESS) != 0) ?
-			    numLost : (numLost / (MPPE_UPDATE_MASK + 1));
-			if (rekey > MPPE_MAX_REKEY) {
-				log(LOG_ERR, "%s: too many (%d) packets"
-				    " dropped, disabling node %p!",
-				    __func__, numLost, node);
-				priv->recv.cfg.enable = 0;
-				goto failed;
-			}
-
-			/* Re-key as necessary to catch up to peer */
+			/*
+			 * When packets are lost or re-ordered with MPPE,
+			 * we may have to re-key up to 0xfff times to 'catch
+			 * up' to the new jumped-ahead sequence number. Yep,
+			 * this is heavy, but what else can we do?
+			 */
 			while (d->cc != cc) {
 				if ((d->cfg.bits & MPPE_STATELESS) != 0
 				    || (d->cc & MPPE_UPDATE_MASK)

--wq9mPyueHGvFACwf--

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111227192238.GV8035>