accel-ppp-users Mailing List for accel-ppp

> Hi,
> 
> Here are two fixups, for master, fixing issues appearing when using
> accel-ppp as LAC (L2TP).
> 
> The first patch allows to initiate tunnels without having to declare
> the peer in [iprange]. This is based on the idea that the iprange
> module has been made to filter incomming connections, not to restrict
> tunnel creation by the administrator.
> 
> The second patch removes an incorrect AVP when sending ICCN messages.
> Thanks to commit 21d36d "l2tp: introduce data packet sequencing and
> reorder timeout support", accel-ppp is now more strict when parsing
> ICCN messages, and won't accept such unexpected mandatory AVPs anymore.
> So this patch is needed in order to let an accel-ppp LAC communicate
> with a recent accel-ppp LNS.
> 
> Best regards,
> 
> Guillaume
> 
> 
> Guillaume Nault (2):
>   l2tp: No iprange check for locally created tunnels
>   l2tp: Don't send Assigned Sesssion ID in ICCN
> 
>  accel-pppd/ctrl/l2tp/l2tp.c | 12 ------------
>  1 file changed, 12 deletions(-)

Applied, thanks.

The Assigned Session ID AVP should only be sent in ICRQ, ICRP, OCRQ,
OCRP and CDN messages. An LNS receiving an ICCN with Assigned
Session ID may reject the message because of this unexpected
mandatory AVP.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/ctrl/l2tp/l2tp.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/accel-pppd/ctrl/l2tp/l2tp.c b/accel-pppd/ctrl/l2tp/l2tp.c
index e018e5b..3b2e3f2 100644
--- a/accel-pppd/ctrl/l2tp/l2tp.c
+++ b/accel-pppd/ctrl/l2tp/l2tp.c
@@ -1822,12 +1822,6 @@ static int l2tp_send_ICCN(struct l2tp_sess_t *sess)
 		return -1;
 	}
 
-	if (l2tp_packet_add_int16(pack, Assigned_Session_ID,
-				  sess->sid, 1) < 0) {
-		log_session(log_error, sess, "impossible to send ICCN:"
-			    " adding data to packet failed\n");
-		goto out_err;
-	}
 	if (l2tp_packet_add_int32(pack, TX_Speed, 1000, 1) < 0) {
 		log_session(log_error, sess, "impossible to send ICCN:"
 			    " adding data to packet failed\n");
-- 
1.8.4.rc3

There's no need to check for iprange when creating a new tunnel
upon administrator's request.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/ctrl/l2tp/l2tp.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/accel-pppd/ctrl/l2tp/l2tp.c b/accel-pppd/ctrl/l2tp/l2tp.c
index e14bab8..e018e5b 100644
--- a/accel-pppd/ctrl/l2tp/l2tp.c
+++ b/accel-pppd/ctrl/l2tp/l2tp.c
@@ -3786,12 +3786,6 @@ static int l2tp_create_tunnel_exec(const char *cmd, char * const *fields,
 		return CLI_CMD_SYNTAX;
 	}
 
-	if (iprange_client_check(peer.sin_addr.s_addr) < 0) {
-		cli_sendv(client, "peer address %s out of IP range\r\n",
-			  fields[peer_indx]);
-		return CLI_CMD_INVAL;
-	}
-
 	conn = l2tp_tunnel_alloc(&peer, &host, 3, lns_mode, 0, hide_avps);
 	if (conn == NULL) {
 		cli_send(client, "tunnel allocation failed\r\n");
-- 
1.8.4.rc3

Hi,

Here are two fixups, for master, fixing issues appearing when using
accel-ppp as LAC (L2TP).

The first patch allows to initiate tunnels without having to declare
the peer in [iprange]. This is based on the idea that the iprange
module has been made to filter incomming connections, not to restrict
tunnel creation by the administrator.

The second patch removes an incorrect AVP when sending ICCN messages.
Thanks to commit 21d36d "l2tp: introduce data packet sequencing and
reorder timeout support", accel-ppp is now more strict when parsing
ICCN messages, and won't accept such unexpected mandatory AVPs anymore.
So this patch is needed in order to let an accel-ppp LAC communicate
with a recent accel-ppp LNS.

Best regards,

Guillaume


Guillaume Nault (2):
  l2tp: No iprange check for locally created tunnels
  l2tp: Don't send Assigned Sesssion ID in ICCN

 accel-pppd/ctrl/l2tp/l2tp.c | 12 ------------
 1 file changed, 12 deletions(-)

-- 
1.8.4.rc3

>Hi,
>
>Patch adds support of equence number in data packets & reordering timeout
>with corresponding "dataseq" & "reorder-timeout" config options.
>  dataseq=deny|allow|prefer|require
>  reorder-timeout=ms
>Both options apply to LNS & LAC sessions

Applied, thanks.

On Tue, Oct 01, 2013 at 05:11:07PM +0600, Vladislav Grishenko wrote:
> > However, "deny" has the same effect as "allow" when
> > applied to a LAC. There's nothing wrong with this as a LAC can only
> require
> > sequencing (and not forbid it), but it may be worth a note in the manpage.
> 
> Actually, things are a bit more complex:
> LNS behavior (default)
> 	LNS controls LAC, so LAC follows LNS point of view about data
> sequencing, and turns off/on its own data sequencing after receiving data
> packets from LNS.
> 	deny: send data packet without sequence numbers, no matter what LAC
> requested
> 	allow:  send data packet without sequence numbers by default, and -
> with sequence numbers if LAC requested so
> 	prefer: send data packet with sequence numbers, this forces LAC to
> enable data sequencing too. LNS drops data packets from LAC without seqs
> after this, it's kernel driver behavior.
> 	require: same behavior as prefer above, because incoming data
> packets from LAC are already checked, if LNS sends seqs.
> 
> LAC behavior (needs more cli support, I don't need it):
> 	deny: don't require data sequencing from LNS, but if LNS sends so,
> LAC mush turn it on. So, "deny" does nothing for LAC
> 	allow:  same, don't request and allow, the correct default behavior
> for LAC.
> 	prefer: send data packet packets with sequence numbers and request
> LNS to enable sequencing, but if LNS sends no seqs, LAC turns its own seqs
> off too.
> 	require: same as prefer, but check for incoming data sequence
> numbers. If LNS doesn't send them, just drop that packets. 
> 
> So, deny/allow/prefer/require is a sufficient set for both LNS/LAC, moreover
> accel-pppd uses always LNS behavior for incoming connections.
> 
Precisely, I just meant to make it clear for users.

> > It
> > may also be good to explicitely state that reordering only makes sense
> when
> > sequencing is active.
> Probably, I'm not expecting sequencing to be widely used. MS LNS doesn't
> support it, at least under Windows 2003 Server.
> 
Me neither, and most users probably don't even know about this protocol
feature. I guess most of them won't see the relationship between the
dataseq and the reorder-timeout options. So better help them understand
this kind of dependencies.

Anyway documentation is already fine IMHO. Improvements can always be sent
later on.

> > Sequencing seems to be broken in recent Linux versions (probably
> > somewhere between Linux-2.6.39 and Linux-3.2). On which version(s) have
> > you tested it?
> Tested with my 2.6.22 (custom), 2.6.32-debian, also refer recent commits in
> kernel git.
> 
Thanks, this confirms my firts thoughts. I have a small patch for fixing the
issue in Linux-3.11. Will probably send it upstream in a few days.

Regards,

Guillaume

> However, "deny" has the same effect as "allow" when
> applied to a LAC. There's nothing wrong with this as a LAC can only
require
> sequencing (and not forbid it), but it may be worth a note in the manpage.

Actually, things are a bit more complex:
LNS behavior (default)
	LNS controls LAC, so LAC follows LNS point of view about data
sequencing, and turns off/on its own data sequencing after receiving data
packets from LNS.
	deny: send data packet without sequence numbers, no matter what LAC
requested
	allow:  send data packet without sequence numbers by default, and -
with sequence numbers if LAC requested so
	prefer: send data packet with sequence numbers, this forces LAC to
enable data sequencing too. LNS drops data packets from LAC without seqs
after this, it's kernel driver behavior.
	require: same behavior as prefer above, because incoming data
packets from LAC are already checked, if LNS sends seqs.

LAC behavior (needs more cli support, I don't need it):
	deny: don't require data sequencing from LNS, but if LNS sends so,
LAC mush turn it on. So, "deny" does nothing for LAC
	allow:  same, don't request and allow, the correct default behavior
for LAC.
	prefer: send data packet packets with sequence numbers and request
LNS to enable sequencing, but if LNS sends no seqs, LAC turns its own seqs
off too.
	require: same as prefer, but check for incoming data sequence
numbers. If LNS doesn't send them, just drop that packets. 

So, deny/allow/prefer/require is a sufficient set for both LNS/LAC, moreover
accel-pppd uses always LNS behavior for incoming connections.

> It
> may also be good to explicitely state that reordering only makes sense
when
> sequencing is active.
Probably, I'm not expecting sequencing to be widely used. MS LNS doesn't
support it, at least under Windows 2003 Server.

> Sequencing seems to be broken in recent Linux versions (probably
> somewhere between Linux-2.6.39 and Linux-3.2). On which version(s) have
> you tested it?
Tested with my 2.6.22 (custom), 2.6.32-debian, also refer recent commits in
kernel git.

Best Regards, Vladislav Grishenko

> -----Original Message-----
> From: Guillaume Nault [mailto:g....@al...]
> Sent: Tuesday, October 01, 2013 4:31 PM
> To: Vladislav Grishenko
> Cc: acc...@li...
> Subject: Re: [Accel-ppp-users] [PATCH] l2tp: add data packet sequencing
and
> reorder timeout support
> 
> On Fri, Sep 27, 2013 at 02:28:28PM +0600, Vladislav Grishenko wrote:
> > Hi,
> >
> > Patch adds support of equence number in data packets & reordering
> > timeout with corresponding "dataseq" & "reorder-timeout" config options.
> >   dataseq=deny|allow|prefer|require
> >   reorder-timeout=ms
> > Both options apply to LNS & LAC sessions
> >
> Looks good to me. However, "deny" has the same effect as "allow" when
> applied to a LAC. There's nothing wrong with this as a LAC can only
require
> sequencing (and not forbid it), but it may be worth a note in the manpage.
It
> may also be good to explicitely state that reordering only makes sense
when
> sequencing is active.
> 
> Sequencing seems to be broken in recent Linux versions (probably
> somewhere between Linux-2.6.39 and Linux-3.2). On which version(s) have
> you tested it?
> 
> BTW, there's an extra ">" character in your patch which prevents correct
> application by git-am.
> 
> Nice patch, thanks for the feature.
> 
> Guillaume

On Fri, Sep 27, 2013 at 02:28:28PM +0600, Vladislav Grishenko wrote:
> Hi,
> 
> Patch adds support of equence number in data packets & reordering timeout
> with corresponding "dataseq" & "reorder-timeout" config options.
>   dataseq=deny|allow|prefer|require
>   reorder-timeout=ms
> Both options apply to LNS & LAC sessions
> 
Looks good to me. However, "deny" has the same effect as "allow" when
applied to a LAC. There's nothing wrong with this as a LAC can only
require sequencing (and not forbid it), but it may be worth a note in
the manpage. It may also be good to explicitely state that reordering
only makes sense when sequencing is active.

Sequencing seems to be broken in recent Linux versions (probably
somewhere between Linux-2.6.39 and Linux-3.2). On which version(s) have
you tested it?

BTW, there's an extra ">" character in your patch which prevents
correct application by git-am.

Nice patch, thanks for the feature.

Guillaume