[PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

An accept() call on a SCTPv6 socket that returns due to connection of
a IPv4 mapped peer will fill out the 'struct sockaddr' with a zero
IPv6 address instead of the IPv4 mapped address of the peer.

This is due to the v4mapped flag not getting copied into the new
socket on accept() as well as a missing check for INET6 socket type in
sctp_v4_to_sk_*addr().

Signed-off-by: Dave Johnson <djohnson+linux-kernel@sw.starentnetworks.com>
Cc: Srinivas Akkipeddi <sakkiped@starentnetworks.com>

===== net/sctp/ipv6.c 1.108 vs edited =====
--- 1.108/net/sctp/ipv6.c	2007-07-05 20:40:15 -04:00
+++ edited/net/sctp/ipv6.c	2007-07-25 16:30:41 -04:00
@@ -641,6 +641,8 @@
 	newsctp6sk = (struct sctp6_sock *)newsk;
 	inet_sk(newsk)->pinet6 = &newsctp6sk->inet6;
 
+	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
+
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
 
===== net/sctp/protocol.c 1.130 vs edited =====
--- 1.130/net/sctp/protocol.c	2007-05-04 16:36:30 -04:00
+++ edited/net/sctp/protocol.c	2007-07-25 16:28:21 -04:00
@@ -257,13 +257,28 @@
 /* Initialize sk->sk_rcv_saddr from sctp_addr. */
 static void sctp_v4_to_sk_saddr(union sctp_addr *addr, struct sock *sk)
 {
-	inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
+	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
+		inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
+		inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
+		inet6_sk(sk)->rcv_saddr.s6_addr32[2] = htonl(0x0000ffff);
+		inet6_sk(sk)->rcv_saddr.s6_addr32[3] =
+			addr->v4.sin_addr.s_addr;
+	} else {
+		inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
+	}
 }
 
 /* Initialize sk->sk_daddr from sctp_addr. */
 static void sctp_v4_to_sk_daddr(union sctp_addr *addr, struct sock *sk)
 {
-	inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
+	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
+		inet6_sk(sk)->daddr.s6_addr32[0] = 0;
+		inet6_sk(sk)->daddr.s6_addr32[1] = 0;
+		inet6_sk(sk)->daddr.s6_addr32[2] = htonl(0x0000ffff);
+		inet6_sk(sk)->daddr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
+	} else {
+		inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
+	}
 }
 
 /* Initialize a sctp_addr from an address parameter. */
@@ -557,6 +572,8 @@
 	newsk->sk_protocol = IPPROTO_SCTP;
 	newsk->sk_backlog_rcv = sk->sk_prot->backlog_rcv;
 	sock_reset_flag(newsk, SOCK_ZAPPED);
+
+	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
 
 	newinet = inet_sk(newsk);
 

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

Dave Johnson wrote:
> An accept() call on a SCTPv6 socket that returns due to connection of
> a IPv4 mapped peer will fill out the 'struct sockaddr' with a zero
> IPv6 address instead of the IPv4 mapped address of the peer.
> 
> This is due to the v4mapped flag not getting copied into the new
> socket on accept() as well as a missing check for INET6 socket type in
> sctp_v4_to_sk_*addr().
> 
> Signed-off-by: Dave Johnson <djohnson+linux-kernel@sw.starentnetworks.com>
> Cc: Srinivas Akkipeddi <sakkiped@starentnetworks.com>
> 
> ===== net/sctp/ipv6.c 1.108 vs edited =====
> --- 1.108/net/sctp/ipv6.c	2007-07-05 20:40:15 -04:00
> +++ edited/net/sctp/ipv6.c	2007-07-25 16:30:41 -04:00
> @@ -641,6 +641,8 @@
>  	newsctp6sk = (struct sctp6_sock *)newsk;
>  	inet_sk(newsk)->pinet6 = &newsctp6sk->inet6;
>  
> +	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
> +
>  	newinet = inet_sk(newsk);
>  	newnp = inet6_sk(newsk);
>  
> ===== net/sctp/protocol.c 1.130 vs edited =====
> --- 1.130/net/sctp/protocol.c	2007-05-04 16:36:30 -04:00
> +++ edited/net/sctp/protocol.c	2007-07-25 16:28:21 -04:00
> @@ -257,13 +257,28 @@
>  /* Initialize sk->sk_rcv_saddr from sctp_addr. */
>  static void sctp_v4_to_sk_saddr(union sctp_addr *addr, struct sock *sk)
>  {
> -	inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
> +	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[0] = 0;
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[1] = 0;
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[2] = htonl(0x0000ffff);
> +		inet6_sk(sk)->rcv_saddr.s6_addr32[3] =
> +			addr->v4.sin_addr.s_addr;
> +	} else {
> +		inet_sk(sk)->rcv_saddr = addr->v4.sin_addr.s_addr;
> +	}
>  }
>  
>  /* Initialize sk->sk_daddr from sctp_addr. */
>  static void sctp_v4_to_sk_daddr(union sctp_addr *addr, struct sock *sk)
>  {
> -	inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
> +	if ((sk->sk_family == PF_INET6) && (sctp_sk(sk)->v4mapped)) {
> +		inet6_sk(sk)->daddr.s6_addr32[0] = 0;
> +		inet6_sk(sk)->daddr.s6_addr32[1] = 0;
> +		inet6_sk(sk)->daddr.s6_addr32[2] = htonl(0x0000ffff);
> +		inet6_sk(sk)->daddr.s6_addr32[3] = addr->v4.sin_addr.s_addr;
> +	} else {
> +		inet_sk(sk)->daddr = addr->v4.sin_addr.s_addr;
> +	}
>  }
>  
>  /* Initialize a sctp_addr from an address parameter. */
> @@ -557,6 +572,8 @@
>  	newsk->sk_protocol = IPPROTO_SCTP;
>  	newsk->sk_backlog_rcv = sk->sk_prot->backlog_rcv;
>  	sock_reset_flag(newsk, SOCK_ZAPPED);
> +
> +	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
>  
>  	newinet = inet_sk(newsk);
>  
> 

Can you explain why the sctp_v4 changes are need for the this case?
I don't see how the code in sctp/protocol.c comes into play for this
particular bug.

Thanks
-vlad

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

Vlad Yasevich writes:
> Can you explain why the sctp_v4 changes are need for the this case?
> I don't see how the code in sctp/protocol.c comes into play for this
> particular bug.

connect() on v6 socket to v4 mapped address will trigger
sctp_v4_to_sk_daddr:

strace:

socket(PF_INET6, SOCK_STREAM, 0x84 /* IPPROTO_??? */) = 3
bind(3, {sa_family=AF_INET6, sin6_port=htons(0), inet_pton(AF_INET6, "::", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
connect(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
getsockname(3, {sa_family=AF_INET6, sin6_port=htons(32771), inet_pton(AF_INET6, "::ffff:192.168.207.234", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0
getpeername(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0

stack dump:

 [<f8c539de>] sctp_v4_to_sk_daddr+0x3e/0x80 [sctp]
 [<f8c5f6c4>] __sctp_connect+0x2a4/0x520 [sctp]
 [<f8c61810>] sctp_connect+0x60/0x70 [sctp]
 [<c02c4b2c>] inet_dgram_connect+0x4c/0x80
 [<c026dbab>] sys_connect+0x8b/0xd0
 [<c026e711>] sys_socketcall+0xb1/0x260
 [<c0103013>] syscall_call+0x7/0xb

I'm unsure if there is a path to call sctp_v4_to_sk_saddr() but it was
added just to be complete.

v4mapped in sctp_v4_create_accept_sk() probably isn't needed, but
since v4mapped is in sctp_sock not sctp6_sock copying it seems like a
good idea.

-- 
Dave Johnson
Starent Networks

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

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

Dave Johnson wrote:
> Vlad Yasevich writes:
>> Can you explain why the sctp_v4 changes are need for the this case?
>> I don't see how the code in sctp/protocol.c comes into play for this
>> particular bug.
> 
> connect() on v6 socket to v4 mapped address will trigger
> sctp_v4_to_sk_daddr:
> 
> strace:
> 
> socket(PF_INET6, SOCK_STREAM, 0x84 /* IPPROTO_??? */) = 3
> bind(3, {sa_family=AF_INET6, sin6_port=htons(0), inet_pton(AF_INET6, "::", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
> connect(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, 128) = 0
> getsockname(3, {sa_family=AF_INET6, sin6_port=htons(32771), inet_pton(AF_INET6, "::ffff:192.168.207.234", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0
> getpeername(3, {sa_family=AF_INET6, sin6_port=htons(33333), inet_pton(AF_INET6, "::ffff:192.168.207.231", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 0
> 
> stack dump:
> 
>  [<f8c539de>] sctp_v4_to_sk_daddr+0x3e/0x80 [sctp]
>  [<f8c5f6c4>] __sctp_connect+0x2a4/0x520 [sctp]
>  [<f8c61810>] sctp_connect+0x60/0x70 [sctp]
>  [<c02c4b2c>] inet_dgram_connect+0x4c/0x80
>  [<c026dbab>] sys_connect+0x8b/0xd0
>  [<c026e711>] sys_socketcall+0xb1/0x260
>  [<c0103013>] syscall_call+0x7/0xb
> 
> I'm unsure if there is a path to call sctp_v4_to_sk_saddr() but it was
> added just to be complete.
> 
> v4mapped in sctp_v4_create_accept_sk() probably isn't needed, but
> since v4mapped is in sctp_sock not sctp6_sock copying it seems like a
> good idea.

Ok.  First, this is a different bug, so I would prefer a separate patch.
Also, I see the problem and it's ugly, but this solution is not really correct,
both conceptually and code wise.

Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
muck with them.  They are IPv6 addresses and there should be a clean separation.

Code wise, the code in the __sctp_connect() is terrible.

Does the attached patch work for you in this case.

Thanks
-vlad

[-- Attachment #2: connect_bug.txt --]
[-- Type: text/plain, Size: 2544 bytes --]

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index b1917f6..1577814 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -977,7 +977,7 @@ static int __sctp_connect(struct sock* sk,
 	int err = 0;
 	int addrcnt = 0;
 	int walk_size = 0;
-	union sctp_addr *sa_addr;
+	union sctp_addr *sa_addr = NULL;
 	void *addr_buf;
 	unsigned short port;
 	unsigned int f_flags = 0;
@@ -1011,7 +1011,10 @@ static int __sctp_connect(struct sock* sk,
 			goto out_free;
 		}
 
-		err = sctp_verify_addr(sk, sa_addr, af->sockaddr_len);
+		/* Save current address so we can work with it */
+		memcpy(&to, sa_addr, af->sockaddr_len);
+
+		err = sctp_verify_addr(sk, &to, af->sockaddr_len);
 		if (err)
 			goto out_free;
 
@@ -1021,12 +1024,11 @@ static int __sctp_connect(struct sock* sk,
 		if (asoc && asoc->peer.port && asoc->peer.port != port)
 			goto out_free;
 
-		memcpy(&to, sa_addr, af->sockaddr_len);
 
 		/* Check if there already is a matching association on the
 		 * endpoint (other than the one created here).
 		 */
-		asoc2 = sctp_endpoint_lookup_assoc(ep, sa_addr, &transport);
+		asoc2 = sctp_endpoint_lookup_assoc(ep, &to, &transport);
 		if (asoc2 && asoc2 != asoc) {
 			if (asoc2->state >= SCTP_STATE_ESTABLISHED)
 				err = -EISCONN;
@@ -1039,7 +1041,7 @@ static int __sctp_connect(struct sock* sk,
 		 * make sure that there is no peeled-off association matching
 		 * the peer address even on another socket.
 		 */
-		if (sctp_endpoint_is_peeled_off(ep, sa_addr)) {
+		if (sctp_endpoint_is_peeled_off(ep, &to)) {
 			err = -EADDRNOTAVAIL;
 			goto out_free;
 		}
@@ -1070,7 +1072,7 @@ static int __sctp_connect(struct sock* sk,
 				}
 			}
 
-			scope = sctp_scope(sa_addr);
+			scope = sctp_scope(&to);
 			asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
 			if (!asoc) {
 				err = -ENOMEM;
@@ -1079,7 +1081,7 @@ static int __sctp_connect(struct sock* sk,
 		}
 
 		/* Prime the peer's transport structures.  */
-		transport = sctp_assoc_add_peer(asoc, sa_addr, GFP_KERNEL,
+		transport = sctp_assoc_add_peer(asoc, &to, GFP_KERNEL,
 						SCTP_UNKNOWN);
 		if (!transport) {
 			err = -ENOMEM;
@@ -1103,8 +1105,8 @@ static int __sctp_connect(struct sock* sk,
 
 	/* Initialize sk's dport and daddr for getpeername() */
 	inet_sk(sk)->dport = htons(asoc->peer.port);
-	af = sctp_get_af_specific(to.sa.sa_family);
-	af->to_sk_daddr(&to, sk);
+	af = sctp_get_af_specific(sa_addr->sa.sa_family);
+	af->to_sk_daddr(sa_addr, sk);
 	sk->sk_err = 0;
 
 	/* in-kernel sockets don't generally have a file allocated to them

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

Vlad Yasevich writes:
> Ok.  First, this is a different bug, so I would prefer a separate patch.
> Also, I see the problem and it's ugly, but this solution is not really correct,
> both conceptually and code wise.
> 
> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
> muck with them.  They are IPv6 addresses and there should be a clean separation.
> 
> Code wise, the code in the __sctp_connect() is terrible.
> 
> Does the attached patch work for you in this case.

yes, with the v4mapped in ipv6.c and your patch, connect and accept
both work with v4 mapped addresses.

Note instead of:

> +	af = sctp_get_af_specific(sa_addr->sa.sa_family);
> +	af->to_sk_daddr(sa_addr, sk);

you should have:

> +	af = sctp_get_af_specific(sa_addr->sa_family);
> +	af->to_sk_daddr((union sctp_addr *)sa_addr, sk);


-- 
Dave Johnson
Starent Networks

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

Dave Johnson wrote:
> Vlad Yasevich writes:
>> Ok.  First, this is a different bug, so I would prefer a separate patch.
>> Also, I see the problem and it's ugly, but this solution is not really correct,
>> both conceptually and code wise.
>>
>> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
>> muck with them.  They are IPv6 addresses and there should be a clean separation.
>>
>> Code wise, the code in the __sctp_connect() is terrible.
>>
>> Does the attached patch work for you in this case.
> 
> yes, with the v4mapped in ipv6.c and your patch, connect and accept
> both work with v4 mapped addresses.
> 
> Note instead of:
> 
>> +	af = sctp_get_af_specific(sa_addr->sa.sa_family);
>> +	af->to_sk_daddr(sa_addr, sk);
> 
> you should have:
> 
>> +	af = sctp_get_af_specific(sa_addr->sa_family);
>> +	af->to_sk_daddr((union sctp_addr *)sa_addr, sk);
> 
> 

Feel free to clean it up and submit both patches.

-vlad

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 26 Jul 2007 16:49:40 -0400

> Feel free to clean it up and submit both patches.

Ping?  Somebody?

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Thu, 26 Jul 2007 16:49:40 -0400
> 
>> Feel free to clean it up and submit both patches.
> 
> Ping?  Somebody?
> 

I am head deep in an ugly bug.  I might take a break
and do this mindless excercise later today if nobody beats
me to it.

-vlad

[PATCH] SCTP: fix IPv4 addr in SCTPv6 accept()/getpeername()

[Dave Johnson]

An accept() call on a SCTPv6 socket that returns due to connection of
a IPv4 mapped peer will fill out the 'struct sockaddr' with a zero
IPv6 address instead of the IPv4 mapped address of the peer.

This is due to the v4mapped flag not getting copied into the new
socket on accept().

Signed-off-by: Dave Johnson <djohnson@sw.starentnetworks.com>
Cc: Srinivas Akkipeddi <sakkiped@starentnetworks.com>

===== net/sctp/ipv6.c 1.108 vs edited =====
--- 1.108/net/sctp/ipv6.c	2007-07-05 20:40:15 -04:00
+++ edited/net/sctp/ipv6.c	2007-07-25 16:30:41 -04:00
@@ -641,6 +641,8 @@
 	newsctp6sk = (struct sctp6_sock *)newsk;
 	inet_sk(newsk)->pinet6 = &newsctp6sk->inet6;
 
+	sctp_sk(newsk)->v4mapped = sctp_sk(sk)->v4mapped;
+
 	newinet = inet_sk(newsk);
 	newnp = inet6_sk(newsk);
 

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Dave Johnson]

Vlad Yasevich writes:
> Ok.  First, this is a different bug, so I would prefer a separate patch.
> Also, I see the problem and it's ugly, but this solution is not really correct,
> both conceptually and code wise.
> 
> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
> muck with them.  They are IPv6 addresses and there should be a clean separation.
> 
> Code wise, the code in the __sctp_connect() is terrible.
> 
> Does the attached patch work for you in this case.

Sorry about the confusion before, your patch to __sctp_connect()
fixes calls to getpeername() after connect() just fine.

I'll post a patch for the accept()/getpeername() case in a bit.

Acked-by: Dave Johnson <djohnson@sw.starentnetworks.com>

-- 
Dave Johnson
Starent Networks

Re: [PATCH] SCTP: IPv4 mapped addr not returned in SCTPv6 accept()

[Vlad Yasevich]

Dave Johnson wrote:
> Vlad Yasevich writes:
>> Ok.  First, this is a different bug, so I would prefer a separate patch.
>> Also, I see the problem and it's ugly, but this solution is not really correct,
>> both conceptually and code wise.
>>
>> Conceptually, the v4 code should never worry about V4-mapped addresses and shouldn't
>> muck with them.  They are IPv6 addresses and there should be a clean separation.
>>
>> Code wise, the code in the __sctp_connect() is terrible.
>>
>> Does the attached patch work for you in this case.
> 
> Sorry about the confusion before, your patch to __sctp_connect()
> fixes calls to getpeername() after connect() just fine.
> 
> I'll post a patch for the accept()/getpeername() case in a bit.
> 
> Acked-by: Dave Johnson <djohnson@sw.starentnetworks.com>
> 

I had time to tinker and pushed both patches already.  They were in
the last pull that Dave Miller did.  I also tested both patches locally
to make sure that they worked as supposed to.

-vlad

[PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Wei Yongjun]

If SCTP data sender received a SACK which contains Cumulative TSN Ack is 
not less than the Cumulative TSN Ack Point, and if this Cumulative TSN 
Ack is not used by the data sender, SCTP data sender still accept this 
SACK , and next SACK which send correctly to DATA sender be dropped, 
because it is less than the new Cumulative TSN Ack Point.
After received this SACK, data will be retrans again and again even if 
correct SACK is received.
So I think this SACK must be dropped to let data transmit  correctly.

Following is the tcpdump of my test. And patch in this mail can avoid 
this problem.

02:19:38.233278 sctp (1) [INIT] [init tag: 1250461886] [rwnd: 54784] [OS: 10] [MIS: 65535] [init TSN: 217114040] 
02:19:39.782160 sctp (1) [INIT ACK] [init tag: 1] [rwnd: 54784] [OS: 100] [MIS: 65535] [init TSN: 100] 
02:19:39.798583 sctp (1) [COOKIE ECHO] 
02:19:40.082125 sctp (1) [COOKIE ACK] 
02:19:40.097859 sctp (1) [DATA] (B)(E) [TSN: 217114040] [SID: 0] [SSEQ 0] [PPID 0xf192090b] 
02:19:40.100162 sctp (1) [DATA] (B)(E) [TSN: 217114041] [SID: 0] [SSEQ 1] [PPID 0x3e467007] 
02:19:40.100779 sctp (1) [DATA] (B)(E) [TSN: 217114042] [SID: 0] [SSEQ 2] [PPID 0x11b12a0a] 
02:19:40.101200 sctp (1) [DATA] (B)(E) [TSN: 217114043] [SID: 0] [SSEQ 3] [PPID 0x30e7d979] 
02:19:40.561147 sctp (1) [SACK] [cum ack 217114040] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:40.568498 sctp (1) [DATA] (B)(E) [TSN: 217114044] [SID: 0] [SSEQ 4] [PPID 0x251ff86f] 
02:19:40.569308 sctp (1) [DATA] (B)(E) [TSN: 217114045] [SID: 0] [SSEQ 5] [PPID 0xe5d5da5d] 
02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:40.701562 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
02:19:40.701567 sctp (1) [DATA] (B)(E) [TSN: 217114047] [SID: 0] [SSEQ 7] [PPID 0xca47e645] 
02:19:40.701569 sctp (1) [DATA] (B)(E) [TSN: 217114048] [SID: 0] [SSEQ 8] [PPID 0x6c0ea150] 
02:19:40.701576 sctp (1) [DATA] (B)(E) [TSN: 217114049] [SID: 0] [SSEQ 9] [PPID 0x9cc1994f] 
02:19:40.701585 sctp (1) [DATA] (B)(E) [TSN: 217114050] [SID: 0] [SSEQ 10] [PPID 0xb1df4129] 
02:19:41.098201 sctp (1) [SACK] [cum ack 217114041] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:41.283257 sctp (1) [SACK] [cum ack 217114042] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:41.457217 sctp (1) [SACK] [cum ack 217114043] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:41.691528 sctp (1) [SACK] [cum ack 217114044] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:41.849636 sctp (1) [SACK] [cum ack 217114045] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:41.975473 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
02:19:42.021229 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:42.196495 sctp (1) [SACK] [cum ack 217114047] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:42.424319 sctp (1) [SACK] [cum ack 217114048] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:42.586924 sctp (1) [SACK] [cum ack 217114049] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:42.744810 sctp (1) [SACK] [cum ack 217114050] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:42.965536 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:43.106385 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
02:19:43.218969 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:45.374101 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
02:19:45.489258 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:49.830116 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
02:19:49.984577 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
02:19:58.760300 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
02:19:58.931690 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 


Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
+++ net/sctp/sm_statefuns.c	2007-07-29 18:14:49.000000000 -0400
@@ -2880,6 +2880,15 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
 		return SCTP_DISPOSITION_DISCARD;
 	}
 
+	/* If Cumulative TSN Ack is not less than the Cumulative TSN
+	 * Ack which will be send in the next data, drop the SACK.
+	 */
+	if (!TSN_lt(ctsn, asoc->next_tsn)) {
+		SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
+		SCTP_DEBUG_PRINTK("next_tsn %x\n", asoc->next_tsn);
+		return SCTP_DISPOSITION_DISCARD;
+	}
+
 	/* Return this SACK for further processing.  */
 	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
 

Re: [Lksctp-developers] [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Neil Horman]

On Tue, Jul 31, 2007 at 12:44:27PM +0800, Wei Yongjun wrote:
> If SCTP data sender received a SACK which contains Cumulative TSN Ack is 
> not less than the Cumulative TSN Ack Point, and if this Cumulative TSN 
> Ack is not used by the data sender, SCTP data sender still accept this 
> SACK , and next SACK which send correctly to DATA sender be dropped, 
> because it is less than the new Cumulative TSN Ack Point.
> After received this SACK, data will be retrans again and again even if 
> correct SACK is received.
> So I think this SACK must be dropped to let data transmit  correctly.
> 
> Following is the tcpdump of my test. And patch in this mail can avoid 
> this problem.
> 
> 02:19:38.233278 sctp (1) [INIT] [init tag: 1250461886] [rwnd: 54784] [OS: 10] [MIS: 65535] [init TSN: 217114040] 
> 02:19:39.782160 sctp (1) [INIT ACK] [init tag: 1] [rwnd: 54784] [OS: 100] [MIS: 65535] [init TSN: 100] 
> 02:19:39.798583 sctp (1) [COOKIE ECHO] 
> 02:19:40.082125 sctp (1) [COOKIE ACK] 
> 02:19:40.097859 sctp (1) [DATA] (B)(E) [TSN: 217114040] [SID: 0] [SSEQ 0] [PPID 0xf192090b] 
> 02:19:40.100162 sctp (1) [DATA] (B)(E) [TSN: 217114041] [SID: 0] [SSEQ 1] [PPID 0x3e467007] 
> 02:19:40.100779 sctp (1) [DATA] (B)(E) [TSN: 217114042] [SID: 0] [SSEQ 2] [PPID 0x11b12a0a] 
> 02:19:40.101200 sctp (1) [DATA] (B)(E) [TSN: 217114043] [SID: 0] [SSEQ 3] [PPID 0x30e7d979] 
> 02:19:40.561147 sctp (1) [SACK] [cum ack 217114040] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:40.568498 sctp (1) [DATA] (B)(E) [TSN: 217114044] [SID: 0] [SSEQ 4] [PPID 0x251ff86f] 
> 02:19:40.569308 sctp (1) [DATA] (B)(E) [TSN: 217114045] [SID: 0] [SSEQ 5] [PPID 0xe5d5da5d] 
> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:40.701562 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> 02:19:40.701567 sctp (1) [DATA] (B)(E) [TSN: 217114047] [SID: 0] [SSEQ 7] [PPID 0xca47e645] 
> 02:19:40.701569 sctp (1) [DATA] (B)(E) [TSN: 217114048] [SID: 0] [SSEQ 8] [PPID 0x6c0ea150] 
> 02:19:40.701576 sctp (1) [DATA] (B)(E) [TSN: 217114049] [SID: 0] [SSEQ 9] [PPID 0x9cc1994f] 
> 02:19:40.701585 sctp (1) [DATA] (B)(E) [TSN: 217114050] [SID: 0] [SSEQ 10] [PPID 0xb1df4129] 
> 02:19:41.098201 sctp (1) [SACK] [cum ack 217114041] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:41.283257 sctp (1) [SACK] [cum ack 217114042] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:41.457217 sctp (1) [SACK] [cum ack 217114043] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:41.691528 sctp (1) [SACK] [cum ack 217114044] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:41.849636 sctp (1) [SACK] [cum ack 217114045] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:41.975473 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> 02:19:42.021229 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:42.196495 sctp (1) [SACK] [cum ack 217114047] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:42.424319 sctp (1) [SACK] [cum ack 217114048] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:42.586924 sctp (1) [SACK] [cum ack 217114049] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:42.744810 sctp (1) [SACK] [cum ack 217114050] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:42.965536 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:43.106385 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> 02:19:43.218969 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:45.374101 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> 02:19:45.489258 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:49.830116 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> 02:19:49.984577 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 02:19:58.760300 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> 02:19:58.931690 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> 
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> --- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
> +++ net/sctp/sm_statefuns.c	2007-07-29 18:14:49.000000000 -0400
> @@ -2880,6 +2880,15 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
>  		return SCTP_DISPOSITION_DISCARD;
>  	}
>  
> +	/* If Cumulative TSN Ack is not less than the Cumulative TSN
> +	 * Ack which will be send in the next data, drop the SACK.
> +	 */
> +	if (!TSN_lt(ctsn, asoc->next_tsn)) {
> +		SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
> +		SCTP_DEBUG_PRINTK("next_tsn %x\n", asoc->next_tsn);
> +		return SCTP_DISPOSITION_DISCARD;
> +	}
> +
>  	/* Return this SACK for further processing.  */
>  	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
>  
> 
> 
Whats the behavior on this in the event that a sack is received in which the
ctsn falls within a a missing space in a stream of gap acks?  I.e. what if the
sack being sent falls into a hole between the ack point and the first gap ack
range?  Does this patch impact that at all?

Also, what is this:
02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] ....

That ack value seems rather out of range for the rest of the trace. Was that
part of your test?  If so, what caused it?

Thanks & Regards
Neil

> 
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> Lksctp-developers mailing list
> Lksctp-developers@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lksctp-developers

-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/

Re: [Lksctp-developers] [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Sridhar Samudrala]

On Tue, 2007-07-31 at 07:37 -0400, Neil Horman wrote:
> On Tue, Jul 31, 2007 at 12:44:27PM +0800, Wei Yongjun wrote:
> > If SCTP data sender received a SACK which contains Cumulative TSN Ack is 
> > not less than the Cumulative TSN Ack Point, and if this Cumulative TSN 
> > Ack is not used by the data sender, SCTP data sender still accept this 
> > SACK , and next SACK which send correctly to DATA sender be dropped, 
> > because it is less than the new Cumulative TSN Ack Point.
> > After received this SACK, data will be retrans again and again even if 
> > correct SACK is received.
> > So I think this SACK must be dropped to let data transmit  correctly.
> > 
> > Following is the tcpdump of my test. And patch in this mail can avoid 
> > this problem.
> > 
> > 02:19:38.233278 sctp (1) [INIT] [init tag: 1250461886] [rwnd: 54784] [OS: 10] [MIS: 65535] [init TSN: 217114040] 
> > 02:19:39.782160 sctp (1) [INIT ACK] [init tag: 1] [rwnd: 54784] [OS: 100] [MIS: 65535] [init TSN: 100] 
> > 02:19:39.798583 sctp (1) [COOKIE ECHO] 
> > 02:19:40.082125 sctp (1) [COOKIE ACK] 
> > 02:19:40.097859 sctp (1) [DATA] (B)(E) [TSN: 217114040] [SID: 0] [SSEQ 0] [PPID 0xf192090b] 
> > 02:19:40.100162 sctp (1) [DATA] (B)(E) [TSN: 217114041] [SID: 0] [SSEQ 1] [PPID 0x3e467007] 
> > 02:19:40.100779 sctp (1) [DATA] (B)(E) [TSN: 217114042] [SID: 0] [SSEQ 2] [PPID 0x11b12a0a] 
> > 02:19:40.101200 sctp (1) [DATA] (B)(E) [TSN: 217114043] [SID: 0] [SSEQ 3] [PPID 0x30e7d979] 
> > 02:19:40.561147 sctp (1) [SACK] [cum ack 217114040] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:40.568498 sctp (1) [DATA] (B)(E) [TSN: 217114044] [SID: 0] [SSEQ 4] [PPID 0x251ff86f] 
> > 02:19:40.569308 sctp (1) [DATA] (B)(E) [TSN: 217114045] [SID: 0] [SSEQ 5] [PPID 0xe5d5da5d] 
> > 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:40.701562 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> > 02:19:40.701567 sctp (1) [DATA] (B)(E) [TSN: 217114047] [SID: 0] [SSEQ 7] [PPID 0xca47e645] 
> > 02:19:40.701569 sctp (1) [DATA] (B)(E) [TSN: 217114048] [SID: 0] [SSEQ 8] [PPID 0x6c0ea150] 
> > 02:19:40.701576 sctp (1) [DATA] (B)(E) [TSN: 217114049] [SID: 0] [SSEQ 9] [PPID 0x9cc1994f] 
> > 02:19:40.701585 sctp (1) [DATA] (B)(E) [TSN: 217114050] [SID: 0] [SSEQ 10] [PPID 0xb1df4129] 
> > 02:19:41.098201 sctp (1) [SACK] [cum ack 217114041] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:41.283257 sctp (1) [SACK] [cum ack 217114042] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:41.457217 sctp (1) [SACK] [cum ack 217114043] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:41.691528 sctp (1) [SACK] [cum ack 217114044] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:41.849636 sctp (1) [SACK] [cum ack 217114045] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:41.975473 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> > 02:19:42.021229 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:42.196495 sctp (1) [SACK] [cum ack 217114047] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:42.424319 sctp (1) [SACK] [cum ack 217114048] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:42.586924 sctp (1) [SACK] [cum ack 217114049] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:42.744810 sctp (1) [SACK] [cum ack 217114050] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:42.965536 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:43.106385 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> > 02:19:43.218969 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:45.374101 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> > 02:19:45.489258 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:49.830116 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> > 02:19:49.984577 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 02:19:58.760300 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
> > 02:19:58.931690 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
> > 
> > 
> > Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> > 
> > --- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
> > +++ net/sctp/sm_statefuns.c	2007-07-29 18:14:49.000000000 -0400
> > @@ -2880,6 +2880,15 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
> >  		return SCTP_DISPOSITION_DISCARD;
> >  	}
> >  
> > +	/* If Cumulative TSN Ack is not less than the Cumulative TSN
> > +	 * Ack which will be send in the next data, drop the SACK.
> > +	 */
> > +	if (!TSN_lt(ctsn, asoc->next_tsn)) {
> > +		SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
> > +		SCTP_DEBUG_PRINTK("next_tsn %x\n", asoc->next_tsn);
> > +		return SCTP_DISPOSITION_DISCARD;
> > +	}
> > +
> >  	/* Return this SACK for further processing.  */
> >  	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
> >  
> > 
> > 
> Whats the behavior on this in the event that a sack is received in which the
> ctsn falls within a a missing space in a stream of gap acks?  I.e. what if the
> sack being sent falls into a hole between the ack point and the first gap ack
> range?  Does this patch impact that at all?
> 
> Also, what is this:
> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] ....
> 
> That ack value seems rather out of range for the rest of the trace. Was that
> part of your test?  If so, what caused it?

Yes. This SACK seems to be totally out of range and may be causing the problem.

I would expect the following check in sctp_sf_eat_sack_6_2() to drop any SACKs
with CTSN value lower than the earlier SACKs.

        /* i) If Cumulative TSN Ack is less than the Cumulative TSN
         *     Ack Point, then drop the SACK.  Since Cumulative TSN
         *     Ack is monotonically increasing, a SACK whose
         *     Cumulative TSN Ack is less than the Cumulative TSN Ack
         *     Point indicates an out-of-order SACK.
         */
        if (TSN_lt(ctsn, asoc->ctsn_ack_point)) {
                SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
                SCTP_DEBUG_PRINTK("ctsn_ack_point %x\n", asoc->ctsn_ack_point);
                return SCTP_DISPOSITION_DISCARD;
        }

Thanks
Sridhar

Re: [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Wei Yongjun]

> On Tue, 2007-07-31 at 07:37 -0400, Neil Horman wrote:
>   
>> On Tue, Jul 31, 2007 at 12:44:27PM +0800, Wei Yongjun wrote:
>>     
>>> If SCTP data sender received a SACK which contains Cumulative TSN Ack is 
>>> not less than the Cumulative TSN Ack Point, and if this Cumulative TSN 
>>> Ack is not used by the data sender, SCTP data sender still accept this 
>>> SACK , and next SACK which send correctly to DATA sender be dropped, 
>>> because it is less than the new Cumulative TSN Ack Point.
>>> After received this SACK, data will be retrans again and again even if 
>>> correct SACK is received.
>>> So I think this SACK must be dropped to let data transmit  correctly.
>>>
>>> Following is the tcpdump of my test. And patch in this mail can avoid 
>>> this problem.
>>>
>>> 02:19:38.233278 sctp (1) [INIT] [init tag: 1250461886] [rwnd: 54784] [OS: 10] [MIS: 65535] [init TSN: 217114040] 
>>> 02:19:39.782160 sctp (1) [INIT ACK] [init tag: 1] [rwnd: 54784] [OS: 100] [MIS: 65535] [init TSN: 100] 
>>> 02:19:39.798583 sctp (1) [COOKIE ECHO] 
>>> 02:19:40.082125 sctp (1) [COOKIE ACK] 
>>> 02:19:40.097859 sctp (1) [DATA] (B)(E) [TSN: 217114040] [SID: 0] [SSEQ 0] [PPID 0xf192090b] 
>>> 02:19:40.100162 sctp (1) [DATA] (B)(E) [TSN: 217114041] [SID: 0] [SSEQ 1] [PPID 0x3e467007] 
>>> 02:19:40.100779 sctp (1) [DATA] (B)(E) [TSN: 217114042] [SID: 0] [SSEQ 2] [PPID 0x11b12a0a] 
>>> 02:19:40.101200 sctp (1) [DATA] (B)(E) [TSN: 217114043] [SID: 0] [SSEQ 3] [PPID 0x30e7d979] 
>>> 02:19:40.561147 sctp (1) [SACK] [cum ack 217114040] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:40.568498 sctp (1) [DATA] (B)(E) [TSN: 217114044] [SID: 0] [SSEQ 4] [PPID 0x251ff86f] 
>>> 02:19:40.569308 sctp (1) [DATA] (B)(E) [TSN: 217114045] [SID: 0] [SSEQ 5] [PPID 0xe5d5da5d] 
>>> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:40.701562 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>> 02:19:40.701567 sctp (1) [DATA] (B)(E) [TSN: 217114047] [SID: 0] [SSEQ 7] [PPID 0xca47e645] 
>>> 02:19:40.701569 sctp (1) [DATA] (B)(E) [TSN: 217114048] [SID: 0] [SSEQ 8] [PPID 0x6c0ea150] 
>>> 02:19:40.701576 sctp (1) [DATA] (B)(E) [TSN: 217114049] [SID: 0] [SSEQ 9] [PPID 0x9cc1994f] 
>>> 02:19:40.701585 sctp (1) [DATA] (B)(E) [TSN: 217114050] [SID: 0] [SSEQ 10] [PPID 0xb1df4129] 
>>> 02:19:41.098201 sctp (1) [SACK] [cum ack 217114041] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:41.283257 sctp (1) [SACK] [cum ack 217114042] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:41.457217 sctp (1) [SACK] [cum ack 217114043] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:41.691528 sctp (1) [SACK] [cum ack 217114044] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:41.849636 sctp (1) [SACK] [cum ack 217114045] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:41.975473 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>> 02:19:42.021229 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:42.196495 sctp (1) [SACK] [cum ack 217114047] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:42.424319 sctp (1) [SACK] [cum ack 217114048] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:42.586924 sctp (1) [SACK] [cum ack 217114049] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:42.744810 sctp (1) [SACK] [cum ack 217114050] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:42.965536 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:43.106385 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>> 02:19:43.218969 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:45.374101 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>> 02:19:45.489258 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:49.830116 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>> 02:19:49.984577 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>> 02:19:58.760300 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>> 02:19:58.931690 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>
>>>
>>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>>>
>>> --- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
>>> +++ net/sctp/sm_statefuns.c	2007-07-29 18:14:49.000000000 -0400
>>> @@ -2880,6 +2880,15 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
>>>  		return SCTP_DISPOSITION_DISCARD;
>>>  	}
>>>  
>>> +	/* If Cumulative TSN Ack is not less than the Cumulative TSN
>>> +	 * Ack which will be send in the next data, drop the SACK.
>>> +	 */
>>> +	if (!TSN_lt(ctsn, asoc->next_tsn)) {
>>> +		SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
>>> +		SCTP_DEBUG_PRINTK("next_tsn %x\n", asoc->next_tsn);
>>> +		return SCTP_DISPOSITION_DISCARD;
>>> +	}
>>> +
>>>  	/* Return this SACK for further processing.  */
>>>  	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
>>>  
>>>
>>>
>>>       
>> Whats the behavior on this in the event that a sack is received in which the
>> ctsn falls within a a missing space in a stream of gap acks?  I.e. what if the
>> sack being sent falls into a hole between the ack point and the first gap ack
>> range?  Does this patch impact that at all?
>>
>> Also, what is this:
>> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] ....
>>
>> That ack value seems rather out of range for the rest of the trace. Was that
>> part of your test?  If so, what caused it?
>>     
>
> Yes. This SACK seems to be totally out of range and may be causing the problem.
>
> I would expect the following check in sctp_sf_eat_sack_6_2() to drop any SACKs
> with CTSN value lower than the earlier SACKs.
>
>         /* i) If Cumulative TSN Ack is less than the Cumulative TSN
>          *     Ack Point, then drop the SACK.  Since Cumulative TSN
>          *     Ack is monotonically increasing, a SACK whose
>          *     Cumulative TSN Ack is less than the Cumulative TSN Ack
>          *     Point indicates an out-of-order SACK.
>          */
>         if (TSN_lt(ctsn, asoc->ctsn_ack_point)) {
>                 SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
>                 SCTP_DEBUG_PRINTK("ctsn_ack_point %x\n", asoc->ctsn_ack_point);
>                 return SCTP_DISPOSITION_DISCARD;
>         }
>   
This place SACK with CTSN value *higher than* the earlier SACKs, So it
can not be dropped.
In my test I send a dup SACK with future CTSN to attack a SCTP assoc,
and it cause data transmit incorrectly. My test procedure is like this:

Endpoint A                                                Endpoint B
                            <---------------   DATA (TSN=1)
SACK(TSN=1) --------------->   (*1)
                             <---------------   DATA (TSN=2)
                             <---------------   DATA (TSN=3)
                             <---------------   DATA (TSN=4)
                             <---------------   DATA (TSN=5)
SACK(TSN=5)  --------------->(*2)
SACK(TSN=1000) --------------->(*3)
                             <---------------   DATA (TSN=6)
                             <---------------   DATA (TSN=7)
                             <---------------   DATA (TSN=8)
                             <---------------   DATA (TSN=9)
SACK(TSN=6)  --------------->(*4)
                            <---------------   DATA (TSN=6)(retrans)


(*1) At this point ctsn_ack_point=0,next_tsn=2, ctsn=1, SACK is accept.
After accept SACK, ctsn_ack_point=1.
(*2) At this point ctsn_ack_point=1,next_tsn=6, ctsn=5,TSN_lt(ctsn,
ctsn_ack_point) is ture, so accept SACK, and then ctsn_ack_point=5
(*3) At this point SACK is a dup SACK, ctsn_ack_point=5,next_tsn=6,
ctsn=1000,TSN_lt(ctsn, ctsn_ack_point) is ture, so accept SACK, and then
ctsn_ack_point=1000
(*4) At this point ctsn_ack_point=1000, next_tsn=10,ctsn=6, TSN_lt(ctsn,
ctsn_ack_point) is false, so SACK is dropped.

Re: [Lksctp-developers] [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Vlad Yasevich]

Sorry, coming in late due to list issues...

Wei Yongjun wrote:
>> On Tue, 2007-07-31 at 07:37 -0400, Neil Horman wrote:
>>   
>>> On Tue, Jul 31, 2007 at 12:44:27PM +0800, Wei Yongjun wrote:
>>>     
>>>> If SCTP data sender received a SACK which contains Cumulative TSN Ack is 
>>>> not less than the Cumulative TSN Ack Point, and if this Cumulative TSN 
>>>> Ack is not used by the data sender, SCTP data sender still accept this 
>>>> SACK , and next SACK which send correctly to DATA sender be dropped, 
>>>> because it is less than the new Cumulative TSN Ack Point.
>>>> After received this SACK, data will be retrans again and again even if 
>>>> correct SACK is received.
>>>> So I think this SACK must be dropped to let data transmit  correctly.
>>>>
>>>> Following is the tcpdump of my test. And patch in this mail can avoid 
>>>> this problem.
>>>>
>>>> 02:19:38.233278 sctp (1) [INIT] [init tag: 1250461886] [rwnd: 54784] [OS: 10] [MIS: 65535] [init TSN: 217114040] 
>>>> 02:19:39.782160 sctp (1) [INIT ACK] [init tag: 1] [rwnd: 54784] [OS: 100] [MIS: 65535] [init TSN: 100] 
>>>> 02:19:39.798583 sctp (1) [COOKIE ECHO] 
>>>> 02:19:40.082125 sctp (1) [COOKIE ACK] 
>>>> 02:19:40.097859 sctp (1) [DATA] (B)(E) [TSN: 217114040] [SID: 0] [SSEQ 0] [PPID 0xf192090b] 
>>>> 02:19:40.100162 sctp (1) [DATA] (B)(E) [TSN: 217114041] [SID: 0] [SSEQ 1] [PPID 0x3e467007] 
>>>> 02:19:40.100779 sctp (1) [DATA] (B)(E) [TSN: 217114042] [SID: 0] [SSEQ 2] [PPID 0x11b12a0a] 
>>>> 02:19:40.101200 sctp (1) [DATA] (B)(E) [TSN: 217114043] [SID: 0] [SSEQ 3] [PPID 0x30e7d979] 
>>>> 02:19:40.561147 sctp (1) [SACK] [cum ack 217114040] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:40.568498 sctp (1) [DATA] (B)(E) [TSN: 217114044] [SID: 0] [SSEQ 4] [PPID 0x251ff86f] 
>>>> 02:19:40.569308 sctp (1) [DATA] (B)(E) [TSN: 217114045] [SID: 0] [SSEQ 5] [PPID 0xe5d5da5d] 
>>>> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:40.701562 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>> 02:19:40.701567 sctp (1) [DATA] (B)(E) [TSN: 217114047] [SID: 0] [SSEQ 7] [PPID 0xca47e645] 
>>>> 02:19:40.701569 sctp (1) [DATA] (B)(E) [TSN: 217114048] [SID: 0] [SSEQ 8] [PPID 0x6c0ea150] 
>>>> 02:19:40.701576 sctp (1) [DATA] (B)(E) [TSN: 217114049] [SID: 0] [SSEQ 9] [PPID 0x9cc1994f] 
>>>> 02:19:40.701585 sctp (1) [DATA] (B)(E) [TSN: 217114050] [SID: 0] [SSEQ 10] [PPID 0xb1df4129] 
>>>> 02:19:41.098201 sctp (1) [SACK] [cum ack 217114041] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:41.283257 sctp (1) [SACK] [cum ack 217114042] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:41.457217 sctp (1) [SACK] [cum ack 217114043] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:41.691528 sctp (1) [SACK] [cum ack 217114044] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:41.849636 sctp (1) [SACK] [cum ack 217114045] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:41.975473 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>> 02:19:42.021229 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:42.196495 sctp (1) [SACK] [cum ack 217114047] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:42.424319 sctp (1) [SACK] [cum ack 217114048] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:42.586924 sctp (1) [SACK] [cum ack 217114049] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:42.744810 sctp (1) [SACK] [cum ack 217114050] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:42.965536 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:43.106385 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>> 02:19:43.218969 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:45.374101 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>> 02:19:45.489258 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:49.830116 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>> 02:19:49.984577 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>> 02:19:58.760300 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>> 02:19:58.931690 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>
>>>>
>>>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>>>>
>>>> --- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
>>>> +++ net/sctp/sm_statefuns.c	2007-07-29 18:14:49.000000000 -0400
>>>> @@ -2880,6 +2880,15 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
>>>>  		return SCTP_DISPOSITION_DISCARD;
>>>>  	}
>>>>  
>>>> +	/* If Cumulative TSN Ack is not less than the Cumulative TSN
>>>> +	 * Ack which will be send in the next data, drop the SACK.
>>>> +	 */
>>>> +	if (!TSN_lt(ctsn, asoc->next_tsn)) {
>>>> +		SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
>>>> +		SCTP_DEBUG_PRINTK("next_tsn %x\n", asoc->next_tsn);
>>>> +		return SCTP_DISPOSITION_DISCARD;
>>>> +	}
>>>> +
>>>>  	/* Return this SACK for further processing.  */
>>>>  	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
>>>>  

This is an interesting case, but I am not sure that simply discarding
the SACK is the right thing.

The peer in this case is violating the protocol whereby he is trying to
advance the cumulative tsn ack to a point beyond the max tsn currently
sent.  I would vote for terminating the association in this case since
either the peer is a mis-behaved implementation, or the association is
under attack.

-vlad

Re: [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Wei Yongjun]

> Sorry, coming in late due to list issues...
>
> Wei Yongjun wrote:
>   
>>> On Tue, 2007-07-31 at 07:37 -0400, Neil Horman wrote:
>>>   
>>>       
>>>> On Tue, Jul 31, 2007 at 12:44:27PM +0800, Wei Yongjun wrote:
>>>>     
>>>>         
>>>>> If SCTP data sender received a SACK which contains Cumulative TSN Ack is 
>>>>> not less than the Cumulative TSN Ack Point, and if this Cumulative TSN 
>>>>> Ack is not used by the data sender, SCTP data sender still accept this 
>>>>> SACK , and next SACK which send correctly to DATA sender be dropped, 
>>>>> because it is less than the new Cumulative TSN Ack Point.
>>>>> After received this SACK, data will be retrans again and again even if 
>>>>> correct SACK is received.
>>>>> So I think this SACK must be dropped to let data transmit  correctly.
>>>>>
>>>>> Following is the tcpdump of my test. And patch in this mail can avoid 
>>>>> this problem.
>>>>>
>>>>> 02:19:38.233278 sctp (1) [INIT] [init tag: 1250461886] [rwnd: 54784] [OS: 10] [MIS: 65535] [init TSN: 217114040] 
>>>>> 02:19:39.782160 sctp (1) [INIT ACK] [init tag: 1] [rwnd: 54784] [OS: 100] [MIS: 65535] [init TSN: 100] 
>>>>> 02:19:39.798583 sctp (1) [COOKIE ECHO] 
>>>>> 02:19:40.082125 sctp (1) [COOKIE ACK] 
>>>>> 02:19:40.097859 sctp (1) [DATA] (B)(E) [TSN: 217114040] [SID: 0] [SSEQ 0] [PPID 0xf192090b] 
>>>>> 02:19:40.100162 sctp (1) [DATA] (B)(E) [TSN: 217114041] [SID: 0] [SSEQ 1] [PPID 0x3e467007] 
>>>>> 02:19:40.100779 sctp (1) [DATA] (B)(E) [TSN: 217114042] [SID: 0] [SSEQ 2] [PPID 0x11b12a0a] 
>>>>> 02:19:40.101200 sctp (1) [DATA] (B)(E) [TSN: 217114043] [SID: 0] [SSEQ 3] [PPID 0x30e7d979] 
>>>>> 02:19:40.561147 sctp (1) [SACK] [cum ack 217114040] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:40.568498 sctp (1) [DATA] (B)(E) [TSN: 217114044] [SID: 0] [SSEQ 4] [PPID 0x251ff86f] 
>>>>> 02:19:40.569308 sctp (1) [DATA] (B)(E) [TSN: 217114045] [SID: 0] [SSEQ 5] [PPID 0xe5d5da5d] 
>>>>> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:40.701562 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>>> 02:19:40.701567 sctp (1) [DATA] (B)(E) [TSN: 217114047] [SID: 0] [SSEQ 7] [PPID 0xca47e645] 
>>>>> 02:19:40.701569 sctp (1) [DATA] (B)(E) [TSN: 217114048] [SID: 0] [SSEQ 8] [PPID 0x6c0ea150] 
>>>>> 02:19:40.701576 sctp (1) [DATA] (B)(E) [TSN: 217114049] [SID: 0] [SSEQ 9] [PPID 0x9cc1994f] 
>>>>> 02:19:40.701585 sctp (1) [DATA] (B)(E) [TSN: 217114050] [SID: 0] [SSEQ 10] [PPID 0xb1df4129] 
>>>>> 02:19:41.098201 sctp (1) [SACK] [cum ack 217114041] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:41.283257 sctp (1) [SACK] [cum ack 217114042] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:41.457217 sctp (1) [SACK] [cum ack 217114043] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:41.691528 sctp (1) [SACK] [cum ack 217114044] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:41.849636 sctp (1) [SACK] [cum ack 217114045] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:41.975473 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>>> 02:19:42.021229 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:42.196495 sctp (1) [SACK] [cum ack 217114047] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:42.424319 sctp (1) [SACK] [cum ack 217114048] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:42.586924 sctp (1) [SACK] [cum ack 217114049] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:42.744810 sctp (1) [SACK] [cum ack 217114050] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:42.965536 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:43.106385 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>>> 02:19:43.218969 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:45.374101 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>>> 02:19:45.489258 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:49.830116 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>>> 02:19:49.984577 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>> 02:19:58.760300 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0] [SSEQ 6] [PPID 0x87d8b423] 
>>>>> 02:19:58.931690 sctp (1) [SACK] [cum ack 217114046] [a_rwnd 54784] [#gap acks 0] [#dup tsns 0] 
>>>>>           
>
>   
> This is an interesting case, but I am not sure that simply discarding
> the SACK is the right thing.
>
> The peer in this case is violating the protocol whereby he is trying to
> advance the cumulative tsn ack to a point beyond the max tsn currently
> sent. I would vote for terminating the association in this case since
> either the peer is a mis-behaved implementation, or the association is
> under attack.
I have modify the patch to abort the association with protocol violation 
error cause, and new patch is come on. May be this patch is correct.^_^

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
+++ net/sctp/sm_statefuns.c	2007-07-31 00:29:16.000000000 -0400
@@ -104,6 +104,13 @@ static sctp_disposition_t sctp_sf_violat
 				     void *arg,
 				     sctp_cmd_seq_t *commands);
 
+static sctp_disposition_t sctp_sf_violation_ctsn(
+				     const struct sctp_endpoint *ep,
+				     const struct sctp_association *asoc,
+				     const sctp_subtype_t type,
+				     void *arg,
+				     sctp_cmd_seq_t *commands);
+
 /* Small helper function that checks if the chunk length
  * is of the appropriate length.  The 'required_length' argument
  * is set to be the size of a specific chunk we are testing.
@@ -2880,6 +2887,13 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
 		return SCTP_DISPOSITION_DISCARD;
 	}
 
+	/* If Cumulative TSN Ack beyond the max tsn currently
+	 * send, terminating the association and respond to the 
+	 * sender with an ABORT.
+	 */
+	if (!TSN_lt(ctsn, asoc->next_tsn))
+		return sctp_sf_violation_ctsn(ep, asoc, type, arg, commands);
+
 	/* Return this SACK for further processing.  */
 	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
 
@@ -3756,6 +3770,68 @@ nomem:
 	return SCTP_DISPOSITION_NOMEM;
 }
 
+/*
+ * Handle a protocol violation when the peer trying to advance the 
+ * cumulative tsn ack to a point beyond the max tsn currently sent.
+ *
+ * We inform the other end by sending an ABORT with a Protocol Violation
+ * error code.
+ *
+ * Section: Not specified
+ * Verification Tag:  Nothing to do
+ * Inputs
+ * (endpoint, asoc, chunk)
+ *
+ * Outputs
+ * (reply_msg, msg_up, counters)
+ *
+ * Generate an  ABORT chunk and terminate the association.
+ */
+static sctp_disposition_t sctp_sf_violation_ctsn(
+				     const struct sctp_endpoint *ep,
+				     const struct sctp_association *asoc,
+				     const sctp_subtype_t type,
+				     void *arg,
+				     sctp_cmd_seq_t *commands)
+{
+	struct sctp_chunk *chunk =  arg;
+	struct sctp_chunk *abort = NULL;
+	char err_str[] = "The cumulative tsn ack beyond the max tsn currently sent:";
+
+	/* Make the abort chunk. */
+	abort = sctp_make_abort_violation(asoc, chunk, err_str,
+					  sizeof(err_str));
+	if (!abort)
+		goto nomem;
+
+	sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
+	SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
+
+	if (asoc->state <= SCTP_STATE_COOKIE_ECHOED) {
+		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
+				SCTP_TO(SCTP_EVENT_TIMEOUT_T1_INIT));
+		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+				SCTP_ERROR(ECONNREFUSED));
+		sctp_add_cmd_sf(commands, SCTP_CMD_INIT_FAILED,
+				SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
+	} else {
+		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
+				SCTP_ERROR(ECONNABORTED));
+		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+				SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
+		SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
+	}
+
+	sctp_add_cmd_sf(commands, SCTP_CMD_DISCARD_PACKET, SCTP_NULL());
+
+	SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
+
+	return SCTP_DISPOSITION_ABORT;
+
+nomem:
+	return SCTP_DISPOSITION_NOMEM;
+}
+
 /***************************************************************************
  * These are the state functions for handling primitive (Section 10) events.
  ***************************************************************************/



-- 

A new email address of FJWAN is launched from Apr.1 2007.
The updated address is: yjwei@cn.fujitsu.com 
--------------------------------------------------
Wei Yongjun
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
8/F., Civil Defense Building, No.189 Guangzhou Road,
Nanjing, 210029, China
TEL: +86+25-86630523-858
COINS: 79955-858
FAX: +86+25-83317685
MAIL: yjwei@cn.fujitsu.com
--------------------------------------------------

Re: [Lksctp-developers] [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Vlad Yasevich]

This is a little better.

One suggestion.  The new function you create is almost exactly like
sctp_sf_violation_chunklen() with the exception of the error string.
Can you extract the common parts into a single function so that
we don't have duplication of code.

Thanks
-vlad

Wei Yongjun wrote:
>>   
>> This is an interesting case, but I am not sure that simply discarding
>> the SACK is the right thing.
>>
>> The peer in this case is violating the protocol whereby he is trying to
>> advance the cumulative tsn ack to a point beyond the max tsn currently
>> sent. I would vote for terminating the association in this case since
>> either the peer is a mis-behaved implementation, or the association is
>> under attack.
> I have modify the patch to abort the association with protocol violation 
> error cause, and new patch is come on. May be this patch is correct.^_^
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> --- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
> +++ net/sctp/sm_statefuns.c	2007-07-31 00:29:16.000000000 -0400
> @@ -104,6 +104,13 @@ static sctp_disposition_t sctp_sf_violat
>  				     void *arg,
>  				     sctp_cmd_seq_t *commands);
>  
> +static sctp_disposition_t sctp_sf_violation_ctsn(
> +				     const struct sctp_endpoint *ep,
> +				     const struct sctp_association *asoc,
> +				     const sctp_subtype_t type,
> +				     void *arg,
> +				     sctp_cmd_seq_t *commands);
> +
>  /* Small helper function that checks if the chunk length
>   * is of the appropriate length.  The 'required_length' argument
>   * is set to be the size of a specific chunk we are testing.
> @@ -2880,6 +2887,13 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
>  		return SCTP_DISPOSITION_DISCARD;
>  	}
>  
> +	/* If Cumulative TSN Ack beyond the max tsn currently
> +	 * send, terminating the association and respond to the 
> +	 * sender with an ABORT.
> +	 */
> +	if (!TSN_lt(ctsn, asoc->next_tsn))
> +		return sctp_sf_violation_ctsn(ep, asoc, type, arg, commands);
> +
>  	/* Return this SACK for further processing.  */
>  	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
>  
> @@ -3756,6 +3770,68 @@ nomem:
>  	return SCTP_DISPOSITION_NOMEM;
>  }
>  
> +/*
> + * Handle a protocol violation when the peer trying to advance the 
> + * cumulative tsn ack to a point beyond the max tsn currently sent.
> + *
> + * We inform the other end by sending an ABORT with a Protocol Violation
> + * error code.
> + *
> + * Section: Not specified
> + * Verification Tag:  Nothing to do
> + * Inputs
> + * (endpoint, asoc, chunk)
> + *
> + * Outputs
> + * (reply_msg, msg_up, counters)
> + *
> + * Generate an  ABORT chunk and terminate the association.
> + */
> +static sctp_disposition_t sctp_sf_violation_ctsn(
> +				     const struct sctp_endpoint *ep,
> +				     const struct sctp_association *asoc,
> +				     const sctp_subtype_t type,
> +				     void *arg,
> +				     sctp_cmd_seq_t *commands)
> +{
> +	struct sctp_chunk *chunk =  arg;
> +	struct sctp_chunk *abort = NULL;
> +	char err_str[] = "The cumulative tsn ack beyond the max tsn currently sent:";
> +
> +	/* Make the abort chunk. */
> +	abort = sctp_make_abort_violation(asoc, chunk, err_str,
> +					  sizeof(err_str));
> +	if (!abort)
> +		goto nomem;
> +
> +	sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> +	SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);
> +
> +	if (asoc->state <= SCTP_STATE_COOKIE_ECHOED) {
> +		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
> +				SCTP_TO(SCTP_EVENT_TIMEOUT_T1_INIT));
> +		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
> +				SCTP_ERROR(ECONNREFUSED));
> +		sctp_add_cmd_sf(commands, SCTP_CMD_INIT_FAILED,
> +				SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
> +	} else {
> +		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
> +				SCTP_ERROR(ECONNABORTED));
> +		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> +				SCTP_PERR(SCTP_ERROR_PROTO_VIOLATION));
> +		SCTP_DEC_STATS(SCTP_MIB_CURRESTAB);
> +	}
> +
> +	sctp_add_cmd_sf(commands, SCTP_CMD_DISCARD_PACKET, SCTP_NULL());
> +
> +	SCTP_INC_STATS(SCTP_MIB_ABORTEDS);
> +
> +	return SCTP_DISPOSITION_ABORT;
> +
> +nomem:
> +	return SCTP_DISPOSITION_NOMEM;
> +}
> +
>  /***************************************************************************
>   * These are the state functions for handling primitive (Section 10) events.
>   ***************************************************************************/
> 
> 
> 

Re: [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Wei Yongjun]

Vlad Yasevich wrote:
> This is a little better.
>
> One suggestion.  The new function you create is almost exactly like
> sctp_sf_violation_chunklen() with the exception of the error string.
> Can you extract the common parts into a single function so that
> we don't have duplication of code.
>
> Thanks
> -vlad
>
>
>   
>>>   
>>> This is an interesting case, but I am not sure that simply discarding
>>> the SACK is the right thing.
>>>
>>> The peer in this case is violating the protocol whereby he is trying to
>>> advance the cumulative tsn ack to a point beyond the max tsn currently
>>> sent. I would vote for terminating the association in this case since
>>> either the peer is a mis-behaved implementation, or the association is
>>> under attack.
>>>       
Patch has been modified base on comment.
Thanks.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000 -0400
+++ net/sctp/sm_statefuns.c	2007-07-31 17:49:22.000000000 -0400
@@ -97,6 +97,13 @@
 					   const struct sctp_association *asoc,
 					   struct sctp_transport *transport);
 
+static sctp_disposition_t sctp_sf_abort_violation(
+				     const struct sctp_association *asoc,
+				     void *arg,
+				     sctp_cmd_seq_t *commands,
+				     const __u8 *payload,
+				     const size_t paylen);
+
 static sctp_disposition_t sctp_sf_violation_chunklen(
 				     const struct sctp_endpoint *ep,
 				     const struct sctp_association *asoc,
@@ -104,6 +111,13 @@
 				     void *arg,
 				     sctp_cmd_seq_t *commands);
 
+static sctp_disposition_t sctp_sf_violation_ctsn(
+				     const struct sctp_endpoint *ep,
+				     const struct sctp_association *asoc,
+				     const sctp_subtype_t type,
+				     void *arg,
+				     sctp_cmd_seq_t *commands);
+
 /* Small helper function that checks if the chunk length
  * is of the appropriate length.  The 'required_length' argument
  * is set to be the size of a specific chunk we are testing.
@@ -2880,6 +2894,13 @@
 		return SCTP_DISPOSITION_DISCARD;
 	}
 
+	/* If Cumulative TSN Ack beyond the max tsn currently
+	 * send, terminating the association and respond to the
+	 * sender with an ABORT.
+	 */
+	if (!TSN_lt(ctsn, asoc->next_tsn))
+		return sctp_sf_violation_ctsn(ep, asoc, type, arg, commands);
+
 	/* Return this SACK for further processing.  */
 	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH(sackh));
 
@@ -3691,40 +3712,21 @@
 	return SCTP_DISPOSITION_VIOLATION;
 }
 
-
 /*
- * Handle a protocol violation when the chunk length is invalid.
- * "Invalid" length is identified as smaller then the minimal length a
- * given chunk can be.  For example, a SACK chunk has invalid length
- * if it's length is set to be smaller then the size of sctp_sack_chunk_t.
- *
- * We inform the other end by sending an ABORT with a Protocol Violation
- * error code.
- *
- * Section: Not specified
- * Verification Tag:  Nothing to do
- * Inputs
- * (endpoint, asoc, chunk)
- *
- * Outputs
- * (reply_msg, msg_up, counters)
- *
- * Generate an  ABORT chunk and terminate the association.
+ * Common function to handle a protocol violation.
  */
-static sctp_disposition_t sctp_sf_violation_chunklen(
-				     const struct sctp_endpoint *ep,
+static sctp_disposition_t sctp_sf_abort_violation(
 				     const struct sctp_association *asoc,
-				     const sctp_subtype_t type,
 				     void *arg,
-				     sctp_cmd_seq_t *commands)
+				     sctp_cmd_seq_t *commands,
+				     const __u8 *payload,
+				     const size_t paylen)
 {
 	struct sctp_chunk *chunk =  arg;
 	struct sctp_chunk *abort = NULL;
-	char 		   err_str[]="The following chunk had invalid length:";
 
 	/* Make the abort chunk. */
-	abort = sctp_make_abort_violation(asoc, chunk, err_str,
-					  sizeof(err_str));
+	abort = sctp_make_abort_violation(asoc, chunk, payload, paylen);
 	if (!abort)
 		goto nomem;
 
@@ -3756,6 +3758,57 @@
 	return SCTP_DISPOSITION_NOMEM;
 }
 
+/*
+ * Handle a protocol violation when the chunk length is invalid.
+ * "Invalid" length is identified as smaller then the minimal length a
+ * given chunk can be.  For example, a SACK chunk has invalid length
+ * if it's length is set to be smaller then the size of sctp_sack_chunk_t.
+ *
+ * We inform the other end by sending an ABORT with a Protocol Violation
+ * error code.
+ *
+ * Section: Not specified
+ * Verification Tag:  Nothing to do
+ * Inputs
+ * (endpoint, asoc, chunk)
+ *
+ * Outputs
+ * (reply_msg, msg_up, counters)
+ *
+ * Generate an  ABORT chunk and terminate the association.
+ */
+static sctp_disposition_t sctp_sf_violation_chunklen(
+				     const struct sctp_endpoint *ep,
+				     const struct sctp_association *asoc,
+				     const sctp_subtype_t type,
+				     void *arg,
+				     sctp_cmd_seq_t *commands)
+{
+	char err_str[]="The following chunk had invalid length:";
+
+	return sctp_sf_abort_violation(asoc, arg, commands, err_str,
+					sizeof(err_str));
+}
+
+/* Handle a protocol violation when the peer trying to advance the
+ * cumulative tsn ack to a point beyond the max tsn currently sent.
+ *
+ * We inform the other end by sending an ABORT with a Protocol Violation
+ * error code.
+ */
+static sctp_disposition_t sctp_sf_violation_ctsn(
+				     const struct sctp_endpoint *ep,
+				     const struct sctp_association *asoc,
+				     const sctp_subtype_t type,
+				     void *arg,
+				     sctp_cmd_seq_t *commands)
+{
+	char err_str[]="The cumulative tsn ack beyond the max tsn currently sent:";
+
+	return sctp_sf_abort_violation(asoc, arg, commands, err_str,
+					sizeof(err_str));
+}
+
 /***************************************************************************
  * These are the state functions for handling primitive (Section 10) events.
  ***************************************************************************/

Re: [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Vlad Yasevich]

Wei Yongjun wrote:
> Patch has been modified base on comment.
> Thanks.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 

Ok, I've applied this patch, but in the future, please
generate patches so that they can be applied
with a -p1 flag.

Please see Documentation/SubmittingPatches for proper
format.

Thanks
-vlad

Re: [Lksctp-developers] [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Michael Tuexen]

Hi Wei,

see my comments in-line.

Best regards
Michael

On Aug 1, 2007, at 3:06 AM, Wei Yongjun wrote:

>
>> On Tue, 2007-07-31 at 07:37 -0400, Neil Horman wrote:
>>
>>> On Tue, Jul 31, 2007 at 12:44:27PM +0800, Wei Yongjun wrote:
>>>
>>>> If SCTP data sender received a SACK which contains Cumulative  
>>>> TSN Ack is
>>>> not less than the Cumulative TSN Ack Point, and if this  
>>>> Cumulative TSN
>>>> Ack is not used by the data sender, SCTP data sender still  
>>>> accept this
>>>> SACK , and next SACK which send correctly to DATA sender be  
>>>> dropped,
>>>> because it is less than the new Cumulative TSN Ack Point.
>>>> After received this SACK, data will be retrans again and again  
>>>> even if
>>>> correct SACK is received.
>>>> So I think this SACK must be dropped to let data transmit   
>>>> correctly.
>>>>
>>>> Following is the tcpdump of my test. And patch in this mail can  
>>>> avoid
>>>> this problem.
>>>>
>>>> 02:19:38.233278 sctp (1) [INIT] [init tag: 1250461886] [rwnd:  
>>>> 54784] [OS: 10] [MIS: 65535] [init TSN: 217114040]
>>>> 02:19:39.782160 sctp (1) [INIT ACK] [init tag: 1] [rwnd: 54784]  
>>>> [OS: 100] [MIS: 65535] [init TSN: 100]
>>>> 02:19:39.798583 sctp (1) [COOKIE ECHO]
>>>> 02:19:40.082125 sctp (1) [COOKIE ACK]
>>>> 02:19:40.097859 sctp (1) [DATA] (B)(E) [TSN: 217114040] [SID: 0]  
>>>> [SSEQ 0] [PPID 0xf192090b]
>>>> 02:19:40.100162 sctp (1) [DATA] (B)(E) [TSN: 217114041] [SID: 0]  
>>>> [SSEQ 1] [PPID 0x3e467007]
>>>> 02:19:40.100779 sctp (1) [DATA] (B)(E) [TSN: 217114042] [SID: 0]  
>>>> [SSEQ 2] [PPID 0x11b12a0a]
>>>> 02:19:40.101200 sctp (1) [DATA] (B)(E) [TSN: 217114043] [SID: 0]  
>>>> [SSEQ 3] [PPID 0x30e7d979]
>>>> 02:19:40.561147 sctp (1) [SACK] [cum ack 217114040] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:40.568498 sctp (1) [DATA] (B)(E) [TSN: 217114044] [SID: 0]  
>>>> [SSEQ 4] [PPID 0x251ff86f]
>>>> 02:19:40.569308 sctp (1) [DATA] (B)(E) [TSN: 217114045] [SID: 0]  
>>>> [SSEQ 5] [PPID 0xe5d5da5d]
>>>> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:40.701562 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0]  
>>>> [SSEQ 6] [PPID 0x87d8b423]
>>>> 02:19:40.701567 sctp (1) [DATA] (B)(E) [TSN: 217114047] [SID: 0]  
>>>> [SSEQ 7] [PPID 0xca47e645]
>>>> 02:19:40.701569 sctp (1) [DATA] (B)(E) [TSN: 217114048] [SID: 0]  
>>>> [SSEQ 8] [PPID 0x6c0ea150]
>>>> 02:19:40.701576 sctp (1) [DATA] (B)(E) [TSN: 217114049] [SID: 0]  
>>>> [SSEQ 9] [PPID 0x9cc1994f]
>>>> 02:19:40.701585 sctp (1) [DATA] (B)(E) [TSN: 217114050] [SID: 0]  
>>>> [SSEQ 10] [PPID 0xb1df4129]
>>>> 02:19:41.098201 sctp (1) [SACK] [cum ack 217114041] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:41.283257 sctp (1) [SACK] [cum ack 217114042] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:41.457217 sctp (1) [SACK] [cum ack 217114043] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:41.691528 sctp (1) [SACK] [cum ack 217114044] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:41.849636 sctp (1) [SACK] [cum ack 217114045] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:41.975473 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0]  
>>>> [SSEQ 6] [PPID 0x87d8b423]
>>>> 02:19:42.021229 sctp (1) [SACK] [cum ack 217114046] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:42.196495 sctp (1) [SACK] [cum ack 217114047] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:42.424319 sctp (1) [SACK] [cum ack 217114048] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:42.586924 sctp (1) [SACK] [cum ack 217114049] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:42.744810 sctp (1) [SACK] [cum ack 217114050] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:42.965536 sctp (1) [SACK] [cum ack 217114046] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:43.106385 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0]  
>>>> [SSEQ 6] [PPID 0x87d8b423]
>>>> 02:19:43.218969 sctp (1) [SACK] [cum ack 217114046] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:45.374101 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0]  
>>>> [SSEQ 6] [PPID 0x87d8b423]
>>>> 02:19:45.489258 sctp (1) [SACK] [cum ack 217114046] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:49.830116 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0]  
>>>> [SSEQ 6] [PPID 0x87d8b423]
>>>> 02:19:49.984577 sctp (1) [SACK] [cum ack 217114046] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>> 02:19:58.760300 sctp (1) [DATA] (B)(E) [TSN: 217114046] [SID: 0]  
>>>> [SSEQ 6] [PPID 0x87d8b423]
>>>> 02:19:58.931690 sctp (1) [SACK] [cum ack 217114046] [a_rwnd  
>>>> 54784] [#gap acks 0] [#dup tsns 0]
>>>>
>>>>
>>>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>>>>
>>>> --- net/sctp/sm_statefuns.c.orig	2007-07-29 18:11:01.000000000  
>>>> -0400
>>>> +++ net/sctp/sm_statefuns.c	2007-07-29 18:14:49.000000000 -0400
>>>> @@ -2880,6 +2880,15 @@ sctp_disposition_t sctp_sf_eat_sack_6_2(
>>>>  		return SCTP_DISPOSITION_DISCARD;
>>>>  	}
>>>>
>>>> +	/* If Cumulative TSN Ack is not less than the Cumulative TSN
>>>> +	 * Ack which will be send in the next data, drop the SACK.
>>>> +	 */
>>>> +	if (!TSN_lt(ctsn, asoc->next_tsn)) {
>>>> +		SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
>>>> +		SCTP_DEBUG_PRINTK("next_tsn %x\n", asoc->next_tsn);
>>>> +		return SCTP_DISPOSITION_DISCARD;
>>>> +	}
>>>> +
>>>>  	/* Return this SACK for further processing.  */
>>>>  	sctp_add_cmd_sf(commands, SCTP_CMD_PROCESS_SACK, SCTP_SACKH 
>>>> (sackh));
>>>>
>>>>
>>>>
>>>>
>>> Whats the behavior on this in the event that a sack is received  
>>> in which the
>>> ctsn falls within a a missing space in a stream of gap acks?   
>>> I.e. what if the
>>> sack being sent falls into a hole between the ack point and the  
>>> first gap ack
>>> range?  Does this patch impact that at all?
>>>
>>> Also, what is this:
>>> 02:19:40.700584 sctp (1) [SACK] [cum ack 290855864] ....
>>>
>>> That ack value seems rather out of range for the rest of the  
>>> trace. Was that
>>> part of your test?  If so, what caused it?
>>>
>>
>> Yes. This SACK seems to be totally out of range and may be causing  
>> the problem.
>>
>> I would expect the following check in sctp_sf_eat_sack_6_2() to  
>> drop any SACKs
>> with CTSN value lower than the earlier SACKs.
>>
>>         /* i) If Cumulative TSN Ack is less than the Cumulative TSN
>>          *     Ack Point, then drop the SACK.  Since Cumulative TSN
>>          *     Ack is monotonically increasing, a SACK whose
>>          *     Cumulative TSN Ack is less than the Cumulative TSN Ack
>>          *     Point indicates an out-of-order SACK.
>>          */
>>         if (TSN_lt(ctsn, asoc->ctsn_ack_point)) {
>>                 SCTP_DEBUG_PRINTK("ctsn %x\n", ctsn);
>>                 SCTP_DEBUG_PRINTK("ctsn_ack_point %x\n", asoc- 
>> >ctsn_ack_point);
>>                 return SCTP_DISPOSITION_DISCARD;
>>         }
>>
> This place SACK with CTSN value *higher than* the earlier SACKs, So it
> can not be dropped.
> In my test I send a dup SACK with future CTSN to attack a SCTP assoc,
> and it cause data transmit incorrectly. My test procedure is like  
> this:
>
> Endpoint A                                                Endpoint B
>                             <---------------   DATA (TSN=1)
> SACK(TSN=1) --------------->   (*1)
>                              <---------------   DATA (TSN=2)
>                              <---------------   DATA (TSN=3)
>                              <---------------   DATA (TSN=4)
>                              <---------------   DATA (TSN=5)
> SACK(TSN=5)  --------------->(*2)
> SACK(TSN=1000) --------------->(*3)
>                              <---------------   DATA (TSN=6)
>                              <---------------   DATA (TSN=7)
>                              <---------------   DATA (TSN=8)
>                              <---------------   DATA (TSN=9)
> SACK(TSN=6)  --------------->(*4)
>                             <---------------   DATA (TSN=6)(retrans)
>
>
> (*1) At this point ctsn_ack_point=0,next_tsn=2, ctsn=1, SACK is  
> accept.
> After accept SACK, ctsn_ack_point=1.
> (*2) At this point ctsn_ack_point=1,next_tsn=6, ctsn=5,TSN_lt(ctsn,
> ctsn_ack_point) is ture, so accept SACK, and then ctsn_ack_point=5
> (*3) At this point SACK is a dup SACK, ctsn_ack_point=5,next_tsn=6,
> ctsn=1000,TSN_lt(ctsn, ctsn_ack_point) is ture, so accept SACK, and  
> then
> ctsn_ack_point=1000
I would not consider it a duplicate SACK. RFC 4460, section 2.37.2 says
that an implementation SHOULD abort the association when receiving a
SACK acknowledging unsent data. So I would suggest to send an ABORT  
chunk.
> (*4) At this point ctsn_ack_point=1000, next_tsn=10,ctsn=6, TSN_lt 
> (ctsn,
> ctsn_ack_point) is false, so SACK is dropped.
>
>
>
> ---------------------------------------------------------------------- 
> ---
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a  
> browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/ 
> _______________________________________________
> Lksctp-developers mailing list
> Lksctp-developers@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lksctp-developers

Re: [Lksctp-developers] [PATCH] SCTP: drop SACK if ctsn is not less than the next tsn of assoc

[Neil Horman]

On Wed, Aug 01, 2007 at 11:01:21AM +0200, Michael Tuexen wrote:
> Hi Wei,
> 
> see my comments in-line.
> 
> Best regards
> Michael
> 
><snip >
> >(*1) At this point ctsn_ack_point=0,next_tsn=2, ctsn=1, SACK is  
> >accept.
> >After accept SACK, ctsn_ack_point=1.
> >(*2) At this point ctsn_ack_point=1,next_tsn=6, ctsn=5,TSN_lt(ctsn,
> >ctsn_ack_point) is ture, so accept SACK, and then ctsn_ack_point=5
> >(*3) At this point SACK is a dup SACK, ctsn_ack_point=5,next_tsn=6,
> >ctsn=1000,TSN_lt(ctsn, ctsn_ack_point) is ture, so accept SACK, and  
> >then
> >ctsn_ack_point=1000
> I would not consider it a duplicate SACK. RFC 4460, section 2.37.2 says
> that an implementation SHOULD abort the association when receiving a
> SACK acknowledging unsent data. So I would suggest to send an ABORT  
> chunk.

+1.  I didn't notice the ctsn value before.  We can't safely accept that a peer
pre-acks data we haven't sent.  Too many security holes.  

Neil



-- 
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *nhorman@tuxdriver.com
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/