[PATCH] ppp: implement MRU option

[PATCH] ppp: implement MRU option

[Kristen Carlson Accardi]

[-- Attachment #1: Type: text/plain, Size: 4364 bytes --]

If the peer requests a MRU option, set the mtu for the network
phase.  When we are in link establishment phase, we should
continue to behave as if no option has been set and the peer
should use the default MRU.

This option is required for the Huawei E160G modem.
---
 gatchat/gatppp.c  |   16 ++++++++++++++++
 gatchat/ppp.h     |    2 ++
 gatchat/ppp_lcp.c |    4 ++++
 gatchat/ppp_net.c |   13 +++++++++++--
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index e1e49e6..b7834e3 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -40,6 +40,7 @@
 #include "ppp.h"
 
 #define DEFAULT_MRU	1500
+#define DEFAULT_MTU	1500
 
 #define BUFFERSZ	(DEFAULT_MRU * 2)
 
@@ -58,6 +59,7 @@ struct _GAtPPP {
 	guint8 buffer[BUFFERSZ];
 	int index;
 	gint mru;
+	gint mtu;
 	char username[256];
 	char password[256];
 	guint32 xmit_accm[8];
@@ -407,6 +409,8 @@ void ppp_net_up_notify(GAtPPP *ppp, const char *ip,
 {
 	ppp->net = ppp_net_new(ppp);
 
+	ppp_net_set_mtu(ppp->net, ppp->mtu);
+
 	if (ppp->connect_cb == NULL)
 		return;
 
@@ -435,6 +439,17 @@ void ppp_set_xmit_accm(GAtPPP *ppp, guint32 accm)
 	ppp->xmit_accm[0] = accm;
 }
 
+/*
+ * The only time we use other than default MTU is when we are in
+ * the network phase.
+ */
+void ppp_set_mtu(GAtPPP *ppp, const guint8 *data)
+{
+	guint16 mtu = get_host_short(data);
+
+	ppp->mtu = mtu;
+}
+
 /* Administrative Open */
 void g_at_ppp_open(GAtPPP *ppp)
 {
@@ -576,6 +591,7 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem)
 
 	/* set options to defaults */
 	ppp->mru = DEFAULT_MRU;
+	ppp->mtu = DEFAULT_MTU;
 	ppp->recv_accm = ~0U;
 	ppp->xmit_accm[0] = ~0U;
 	ppp->xmit_accm[3] = 0x60000000; /* 0x7d, 0x7e */
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index a8a0486..07483a9 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -103,6 +103,7 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp);
 const char *ppp_net_get_interface(struct ppp_net *net);
 void ppp_net_process_packet(struct ppp_net *net, guint8 *packet);
 void ppp_net_free(struct ppp_net *net);
+void ppp_net_set_mtu(struct ppp_net *net, guint16 mtu);
 
 /* PPP functions related to main GAtPPP object */
 void ppp_debug(GAtPPP *ppp, const char *str);
@@ -115,3 +116,4 @@ void ppp_net_up_notify(GAtPPP *ppp, const char *ip,
 void ppp_net_down_notify(GAtPPP *ppp);
 void ppp_set_recv_accm(GAtPPP *ppp, guint32 accm);
 void ppp_set_xmit_accm(GAtPPP *ppp, guint32 accm);
+void ppp_set_mtu(GAtPPP *ppp, const guint8 *data);
diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index 5cf5656..8639c6c 100644
--- a/gatchat/ppp_lcp.c
+++ b/gatchat/ppp_lcp.c
@@ -197,6 +197,7 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
 		case ACCM:
 		case PFC:
 		case ACFC:
+		case MRU:
 			break;
 
 		case MAGIC_NUMBER:
@@ -226,6 +227,9 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
 		case AUTH_PROTO:
 			ppp_set_auth(ppp, ppp_option_iter_get_data(&iter));
 			break;
+		case MRU:
+			ppp_set_mtu(ppp, ppp_option_iter_get_data(&iter));
+			break;
 		case MAGIC_NUMBER:
 		case PFC:
 		case ACFC:
diff --git a/gatchat/ppp_net.c b/gatchat/ppp_net.c
index 325e859..c1f2eb4 100644
--- a/gatchat/ppp_net.c
+++ b/gatchat/ppp_net.c
@@ -38,7 +38,6 @@
 #include "gatppp.h"
 #include "ppp.h"
 
-/* XXX should be maximum IP Packet size */
 #define MAX_PACKET 1500
 
 struct ppp_net {
@@ -46,8 +45,17 @@ struct ppp_net {
 	char *if_name;
 	GIOChannel *channel;
 	gint watch;
+	gint mtu;
 };
 
+void ppp_net_set_mtu(struct ppp_net *net, guint16 mtu)
+{
+	if (net == NULL)
+		return;
+
+	net->mtu = mtu;
+}
+
 void ppp_net_process_packet(struct ppp_net *net, guint8 *packet)
 {
 	GError *error = NULL;
@@ -80,7 +88,7 @@ static gboolean ppp_net_callback(GIOChannel *channel, GIOCondition cond,
 
 	if (cond & G_IO_IN) {
 		/* leave space to add PPP protocol field */
-		status = g_io_channel_read_chars(channel, buf + 2, MAX_PACKET,
+		status = g_io_channel_read_chars(channel, buf + 2, net->mtu,
 				&bytes_read, &error);
 		if (bytes_read > 0) {
 			ppp->proto = htons(PPP_IP_PROTO);
@@ -140,6 +148,7 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp)
 			ppp_net_callback, net);
 	net->ppp = ppp;
 
+	net->mtu = MAX_PACKET;
 	return net;
 
 error:
-- 
1.6.6.1

Re: [PATCH] ppp: implement MRU option

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

Hi Kristen,

> If the peer requests a MRU option, set the mtu for the network
> phase.  When we are in link establishment phase, we should
> continue to behave as if no option has been set and the peer
> should use the default MRU.
> 
> This option is required for the Huawei E160G modem.

patch has been applied. Thanks.

Regards

Marcel

Re: [PATCH] ppp: implement MRU option

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

Hi Kristen,

> If the peer requests a MRU option, set the mtu for the network
> phase.  When we are in link establishment phase, we should
> continue to behave as if no option has been set and the peer
> should use the default MRU.
> 
> This option is required for the Huawei E160G modem.

can you also send a pppdump from this modem setup with a few IP packets
in for reference. I like to see what the Huawei modem does on LCP or
IPCP setup.

Regards

Marcel

Re: [PATCH] ppp: implement MRU option

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

Hi Kristen,

> @@ -80,7 +88,7 @@ static gboolean ppp_net_callback(GIOChannel *channel,
>  GIOCondition cond,
> 
>  	if (cond & G_IO_IN) {
>  		/* leave space to add PPP protocol field */
> -		status = g_io_channel_read_chars(channel, buf + 2, MAX_PACKET,
> +		status = g_io_channel_read_chars(channel, buf + 2, net->mtu,
>  				&bytes_read, &error);
>  		if (bytes_read > 0) {
>  			ppp->proto = htons(PPP_IP_PROTO);

So I actually have a concern here.  Quoting RFC 1332 Section 2.1:
"Exactly one IP packet is encapsulated in the Information field of PPP
   Data Link Layer frames where the Protocol field indicates type hex
   0021 (Internet Protocol)."

However, you're not changing the physical MTU of the TUN/TAP interface, so in 
effect you are mangling the packets that are larger than net->mtu.  These 
packets will most likely simply be discarded leading to lots of fun.

Comments?

Regards,
-Denis

Re: [PATCH] ppp: implement MRU option

[Kristen Carlson Accardi]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

On Thu, 22 Apr 2010 15:16:58 -0500
Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Kristen,
> 
> > @@ -80,7 +88,7 @@ static gboolean ppp_net_callback(GIOChannel *channel,
> >  GIOCondition cond,
> > 
> >  	if (cond & G_IO_IN) {
> >  		/* leave space to add PPP protocol field */
> > -		status = g_io_channel_read_chars(channel, buf + 2, MAX_PACKET,
> > +		status = g_io_channel_read_chars(channel, buf + 2, net->mtu,
> >  				&bytes_read, &error);
> >  		if (bytes_read > 0) {
> >  			ppp->proto = htons(PPP_IP_PROTO);
> 
> So I actually have a concern here.  Quoting RFC 1332 Section 2.1:
> "Exactly one IP packet is encapsulated in the Information field of PPP
>    Data Link Layer frames where the Protocol field indicates type hex
>    0021 (Internet Protocol)."
> 
> However, you're not changing the physical MTU of the TUN/TAP interface, so in 
> effect you are mangling the packets that are larger than net->mtu.  These 
> packets will most likely simply be discarded leading to lots of fun.

We can add an ioctl to change the MTU during ppp_net_set_mtu(), which
should hopefully resolve this.

Re: [PATCH] ppp: implement MRU option

[Kristen Carlson Accardi]

[-- Attachment #1: Type: text/plain, Size: 1439 bytes --]

On Thu, 22 Apr 2010 15:51:26 -0500
Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Kristen,
> 
> > On Thu, 22 Apr 2010 15:16:58 -0500
> > 
> > Denis Kenzior <denkenz@gmail.com> wrote:
> > > Hi Kristen,
> > >
> > > > @@ -80,7 +88,7 @@ static gboolean ppp_net_callback(GIOChannel *channel,
> > > >  GIOCondition cond,
> > > >
> > > >  	if (cond & G_IO_IN) {
> > > >  		/* leave space to add PPP protocol field */
> > > > -		status = g_io_channel_read_chars(channel, buf + 2, MAX_PACKET,
> > > > +		status = g_io_channel_read_chars(channel, buf + 2, net->mtu,
> > > >  				&bytes_read, &error);
> > > >  		if (bytes_read > 0) {
> > > >  			ppp->proto = htons(PPP_IP_PROTO);
> > >
> > > So I actually have a concern here.  Quoting RFC 1332 Section 2.1:
> > > "Exactly one IP packet is encapsulated in the Information field of PPP
> > >    Data Link Layer frames where the Protocol field indicates type hex
> > >    0021 (Internet Protocol)."
> > >
> > > However, you're not changing the physical MTU of the TUN/TAP interface,
> > > so in effect you are mangling the packets that are larger than net->mtu. 
> > > These packets will most likely simply be discarded leading to lots of
> > > fun.
> > 
> > We can add an ioctl to change the MTU during ppp_net_set_mtu(), which
> > should hopefully resolve this.
> > 
> 
> Yep, my thinking as well.  Can you send a patch?
> 
> Thanks,
> -Denis

yes, will do.

Re: [PATCH] ppp: implement MRU option

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

Hi Kristen,

> On Thu, 22 Apr 2010 15:16:58 -0500
> 
> Denis Kenzior <denkenz@gmail.com> wrote:
> > Hi Kristen,
> >
> > > @@ -80,7 +88,7 @@ static gboolean ppp_net_callback(GIOChannel *channel,
> > >  GIOCondition cond,
> > >
> > >  	if (cond & G_IO_IN) {
> > >  		/* leave space to add PPP protocol field */
> > > -		status = g_io_channel_read_chars(channel, buf + 2, MAX_PACKET,
> > > +		status = g_io_channel_read_chars(channel, buf + 2, net->mtu,
> > >  				&bytes_read, &error);
> > >  		if (bytes_read > 0) {
> > >  			ppp->proto = htons(PPP_IP_PROTO);
> >
> > So I actually have a concern here.  Quoting RFC 1332 Section 2.1:
> > "Exactly one IP packet is encapsulated in the Information field of PPP
> >    Data Link Layer frames where the Protocol field indicates type hex
> >    0021 (Internet Protocol)."
> >
> > However, you're not changing the physical MTU of the TUN/TAP interface,
> > so in effect you are mangling the packets that are larger than net->mtu. 
> > These packets will most likely simply be discarded leading to lots of
> > fun.
> 
> We can add an ioctl to change the MTU during ppp_net_set_mtu(), which
> should hopefully resolve this.
> 

Yep, my thinking as well.  Can you send a patch?

Thanks,
-Denis