Bug 182212 - [patch] [ng_mppc] ng_mppc(4) blocks on network errors unconditionaly
Summary: [patch] [ng_mppc] ng_mppc(4) blocks on network errors unconditionaly
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 8.4-STABLE
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-net (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-18 20:30 UTC by Eugene Grosbein
Modified: 2014-06-05 06:02 UTC (History)
1 user (show)

See Also:


Attachments
file.diff (2.16 KB, patch)
2013-09-18 20:30 UTC, Eugene Grosbein
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Grosbein 2013-09-18 20:30:00 UTC
	Netgraph node ng_mppc(4) is used by mpd daemon to implement
	MPPC/MPPE for Compression Control Protocol (CCP) used with various
	PPP tunnels (pptp/l2tp/etc.)

	This node blocks itself when "too many" packets got dropped in between
	PPP peers and MPPE should do "re-keying". The threshold is hardcoded
	in its code. The code considers "re-keying" as too CPU intensive task.
	Thus, it tries to protect the box from DoS.
	This code dates back to year 2000.

	When such event occcurs, PPP tunnel hangs: CCP does not get reset,
	IP packets cannot pass tunnel anymore but system interface "looks fine".

	These days mpd runs on multi-MHZ and GHZ boxes and may be used
	not for BRAS'es with lots of tunnels but to form VPN between a pair
	of routers. In the latter case, manual reset of mpd link is required
	to revive VPN tunnel.

Fix: Let's system administrator decides if such behavour is needed.
	The following patch introduces new sysctl subtree "net.graph.mppe"
	with three read/write sysctls (each one is loader tunnable too):

net.graph.mppe.block_on_max_rekey - non-zero value means current behavour,
zero disables node block;

net.graph.mppe.mppe_log_max_rekey - non-zero value permits to write messages
to the log to notify of described event;

net.graph.mppe.max_rekey - allows to change the threshold.

	By default, node block is prohibited and mpd just resets CCP
	and tunnel continues to work. Mpd writes a line like this to its log:

CCP: SendResetReq #3 link 0 (Opened)
	
	At the another side, next line is written to mpd log:

CCP: rec'd Reset Request #3 (Opened)
How-To-Repeat: 	
	Run PPtP or L2TP tunnel between two mpd servers over WAN link
	with non-zero amount of packet drops and/or packet rearrangements
	having MPPC/MPPE enabled. Soon, you will get a message in the dmesg
	buffer similar to:

ng_mppc_decompress: too many (4094) packets dropped, disabling node 0xc7020900!

	Then tunnel just hangs until manually restarted as LCP echos
	cannot detect this problem.
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2013-09-19 03:35:33 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-net

Over to maintainer(s).
Comment 2 dfilter service freebsd_committer freebsd_triage 2014-05-22 08:27:08 UTC
Author: mav
Date: Thu May 22 07:27:04 2014
New Revision: 266538
URL: http://svnweb.freebsd.org/changeset/base/266538

Log:
  Make ng_mppc to not disable the node in case of multiple packet loss.
  Quite often it can be just packet reorder, and killing link in such case
  is inconvenient.  Add few sysctl's to control that behavior.
  
  PR:		kern/182212
  Submitted by:	Eugene Grosbein <egrosbein@rdtc.ru>
  MFC after:	2 weeks

Modified:
  head/sys/netgraph/ng_mppc.c

Modified: head/sys/netgraph/ng_mppc.c
==============================================================================
--- head/sys/netgraph/ng_mppc.c	Thu May 22 07:25:36 2014	(r266537)
+++ head/sys/netgraph/ng_mppc.c	Thu May 22 07:27:04 2014	(r266538)
@@ -55,6 +55,7 @@
 #include <sys/malloc.h>
 #include <sys/endian.h>
 #include <sys/errno.h>
+#include <sys/sysctl.h>
 #include <sys/syslog.h>
 
 #include <netgraph/ng_message.h>
@@ -107,6 +108,23 @@ static MALLOC_DEFINE(M_NETGRAPH_MPPC, "n
  */
 #define MPPE_MAX_REKEY		1000
 
+SYSCTL_NODE(_net_graph, OID_AUTO, mppe, CTLFLAG_RW, 0, "MPPE");
+
+static int mppe_block_on_max_rekey = 0;
+TUNABLE_INT("net.graph.mppe.block_on_max_rekey", &mppe_block_on_max_rekey);
+SYSCTL_INT(_net_graph_mppe, OID_AUTO, block_on_max_rekey, CTLFLAG_RW,
+    &mppe_block_on_max_rekey, 0, "Block node on max MPPE key re-calculations");
+
+static int mppe_log_max_rekey = 1;
+TUNABLE_INT("net.graph.mppe.log_max_rekey", &mppe_log_max_rekey);
+SYSCTL_INT(_net_graph_mppe, OID_AUTO, log_max_rekey, CTLFLAG_RW,
+    &mppe_log_max_rekey, 0, "Log max MPPE key re-calculations event");
+
+static int mppe_max_rekey = MPPE_MAX_REKEY;
+TUNABLE_INT("net.graph.mppe.max_rekey", &mppe_max_rekey);
+SYSCTL_INT(_net_graph_mppe, OID_AUTO, max_rekey, CTLFLAG_RW,
+    &mppe_max_rekey, 0, "Maximum number of MPPE key re-calculations");
+
 /* MPPC packet header bits */
 #define MPPC_FLAG_FLUSHED	0x8000		/* xmitter reset state */
 #define MPPC_FLAG_RESTART	0x4000		/* compress history restart */
@@ -646,12 +664,23 @@ ng_mppc_decompress(node_p node, struct m
 			/* 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);
+			if (rekey > mppe_max_rekey) {
+			    if (mppe_block_on_max_rekey) {
+				if (mppe_log_max_rekey) {
+				    log(LOG_ERR, "%s: too many (%d) packets"
+					" dropped, disabling node %p!\n",
+					__func__, numLost, node);
+				}
 				priv->recv.cfg.enable = 0;
 				goto failed;
+			    } else {
+				if (mppe_log_max_rekey) {
+				    log(LOG_ERR, "%s: %d packets"
+					" dropped, node %p\n",
+					__func__, numLost, node);
+				}
+				goto failed;
+			    }
 			}
 
 			/* Re-key as necessary to catch up to peer */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 3 Alexander Motin freebsd_committer freebsd_triage 2014-05-22 08:27:24 UTC
State Changed
From-To: open->patched

Patch committed to HEAD.
Comment 4 commit-hook freebsd_committer freebsd_triage 2014-06-05 05:37:25 UTC
A commit references this bug:

Author: mav
Date: Thu Jun  5 05:36:56 UTC 2014
New revision: 267093
URL: http://svnweb.freebsd.org/changeset/base/267093

Log:
  MFC r266538:
  Make ng_mppc to not disable the node in case of multiple packet loss.
  Quite often it can be just packet reorder, and killing link in such case
  is inconvenient.  Add few sysctl's to control that behavior.

  PR:		kern/182212
  Submitted by:	Eugene Grosbein <egrosbein@rdtc.ru>

Changes:
_U  stable/10/
  stable/10/sys/netgraph/ng_mppc.c
Comment 5 commit-hook freebsd_committer freebsd_triage 2014-06-05 05:59:27 UTC
A commit references this bug:

Author: mav
Date: Thu Jun  5 05:58:55 UTC 2014
New revision: 267094
URL: http://svnweb.freebsd.org/changeset/base/267094

Log:
  MFC r266538:
  Make ng_mppc to not disable the node in case of multiple packet loss.
  Quite often it can be just packet reorder, and killing link in such case
  is inconvenient.  Add few sysctl's to control that behavior.

  PR:		kern/182212
  Submitted by:	Eugene Grosbein <egrosbein@rdtc.ru>
  Approved by:	re (glebius)

Changes:
_U  stable/9/
_U  stable/9/sys/
  stable/9/sys/netgraph/ng_mppc.c
Comment 6 commit-hook freebsd_committer freebsd_triage 2014-06-05 06:00:28 UTC
A commit references this bug:

Author: mav
Date: Thu Jun  5 06:00:08 UTC 2014
New revision: 267095
URL: http://svnweb.freebsd.org/changeset/base/267095

Log:
  MFC r266538:
  Make ng_mppc to not disable the node in case of multiple packet loss.
  Quite often it can be just packet reorder, and killing link in such case
  is inconvenient.  Add few sysctl's to control that behavior.

  PR:		kern/182212
  Submitted by:	Eugene Grosbein <egrosbein@rdtc.ru>

Changes:
_U  stable/8/
_U  stable/8/sys/
_U  stable/8/sys/netgraph/
  stable/8/sys/netgraph/ng_mppc.c
Comment 7 Alexander Motin freebsd_committer freebsd_triage 2014-06-05 06:02:52 UTC
Patch merged sown to stable/8.