[PATCH 1/2] sctp: fix heartbeat count of association failure

RFC4960 Section 8.1 defined that the association should enter CLOSE
state when the value of association error counter exceeds the limit 
indicated in the protocol parameter 'Association.Max.Retrans'. This
means that the association should enter CLOSE state after max_retrans+1 
heartbeats are not acknowledged.

Signed-off-by: Chunbo Luo <redacted>
---
 net/sctp/sm_statefuns.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 7288192..90e4f06 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c

@@ -5190,7 +5190,7 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(const struct sctp_endpoint *ep,
 
 	SCTP_INC_STATS(SCTP_MIB_T3_RTX_EXPIREDS);
 
-	if (asoc->overall_error_count >= asoc->max_retrans) {
+	if (asoc->overall_error_count > asoc->max_retrans) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
 				SCTP_ERROR(ETIMEDOUT));
 		/* CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */

@@ -5404,7 +5404,7 @@ sctp_disposition_t sctp_sf_t2_timer_expire(const struct sctp_endpoint *ep,
 
 	((struct sctp_association *)asoc)->shutdown_retries++;
 
-	if (asoc->overall_error_count >= asoc->max_retrans) {
+	if (asoc->overall_error_count > asoc->max_retrans) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,
 				SCTP_ERROR(ETIMEDOUT));
 		/* Note:  CMD_ASSOC_FAILED calls CMD_DELETE_TCB. */

@@ -5487,7 +5487,7 @@ sctp_disposition_t sctp_sf_t4_timer_expire(
 	 * RFC2960 [5] section 8.1 and 8.2.
 	 * association error counter is incremented in SCTP_CMD_STRIKE.
 	 */
-	if (asoc->overall_error_count >= asoc->max_retrans) {
+	if (asoc->overall_error_count > asoc->max_retrans) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
 				SCTP_TO(SCTP_EVENT_TIMEOUT_T4_RTO));
 		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR,

-- 
1.6.4

RFC4960 Section 8.2 defined that the transport should enter INACTIVE
state only when the value in the error counter exceeds the protocol 
parameter 'Path.Max.Retrans' of that destination address. This means 
that the transport should enter INACTIVE state after pathmaxrxt+1
heartbeats are not acknowledged.


Signed-off-by: Chunbo Luo <redacted>
---
 net/sctp/sm_sideeffect.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 86426aa..0e2e269 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c

@@ -447,7 +447,7 @@ static void sctp_do_8_2_transport_strike(struct sctp_association *asoc,
 		asoc->overall_error_count++;
 
 	if (transport->state != SCTP_INACTIVE &&
-	    (transport->error_count++ >= transport->pathmaxrxt)) {
+	    (transport->error_count++ > transport->pathmaxrxt)) {
 		SCTP_DEBUG_PRINTK_IPADDR("transport_strike:association %p",
 					 " transport IP: port:%d failed.\n",
 					 asoc,

-- 
1.6.4

Chunbo Luo wrote:

RFC4960 Section 8.2 defined that the transport should enter INACTIVE
state only when the value in the error counter exceeds the protocol 
parameter 'Path.Max.Retrans' of that destination address. This means 
that the transport should enter INACTIVE state after pathmaxrxt+1
heartbeats are not acknowledged.


Signed-off-by: Chunbo Luo <redacted>

NAK.  This patch seems to resurface periodically and I have to keep
explaining that it's wrong.

Every time we send a HB, we tick up the error count and clear it when
the HB-ACK is received.  Each HB is separate and not a retransmission,
so we once we reach the pathmaxrxt, we've already sent max+1 HB, so we
have time out.  Walk through the code with some values and you'll see
what I mean.

-vlad

On Wed, 2009-08-19 at 10:48 -0400, Vlad Yasevich wrote:

Chunbo Luo wrote:

quoted

RFC4960 Section 8.2 defined that the transport should enter INACTIVE
state only when the value in the error counter exceeds the protocol 
parameter 'Path.Max.Retrans' of that destination address. This means 
that the transport should enter INACTIVE state after pathmaxrxt+1
heartbeats are not acknowledged.


Signed-off-by: Chunbo Luo <redacted>

NAK.  This patch seems to resurface periodically and I have to keep
explaining that it's wrong.

Every time we send a HB, we tick up the error count and clear it when
the HB-ACK is received.  Each HB is separate and not a retransmission,
so we once we reach the pathmaxrxt, we've already sent max+1 HB, so we
have time out.  Walk through the code with some values and you'll see
what I mean.

Although we've already sent max+1 HB, but the code set the transport to
INACTIVE state immediately, which is equal to not sending the max+1 HB
at all.  We should wait for a next period and make sure the max+1 HB was
not acknowledged.

Chunbo 

-vlad

quoted

---
 net/sctp/sm_sideeffect.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 86426aa..0e2e269 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c

@@ -447,7 +447,7 @@ static void sctp_do_8_2_transport_strike(struct sctp_association *asoc,
 		asoc->overall_error_count++;
 
 	if (transport->state != SCTP_INACTIVE &&
-	    (transport->error_count++ >= transport->pathmaxrxt)) {
+	    (transport->error_count++ > transport->pathmaxrxt)) {
 		SCTP_DEBUG_PRINTK_IPADDR("transport_strike:association %p",
 					 " transport IP: port:%d failed.\n",
 					 asoc,

Luo Chunbo wrote:

On Wed, 2009-08-19 at 10:48 -0400, Vlad Yasevich wrote:

quoted

Chunbo Luo wrote:

quoted

RFC4960 Section 8.2 defined that the transport should enter INACTIVE
state only when the value in the error counter exceeds the protocol 
parameter 'Path.Max.Retrans' of that destination address. This means 
that the transport should enter INACTIVE state after pathmaxrxt+1
heartbeats are not acknowledged.


Signed-off-by: Chunbo Luo <redacted>

NAK.  This patch seems to resurface periodically and I have to keep
explaining that it's wrong.

Every time we send a HB, we tick up the error count and clear it when
the HB-ACK is received.  Each HB is separate and not a retransmission,
so we once we reach the pathmaxrxt, we've already sent max+1 HB, so we
have time out.  Walk through the code with some values and you'll see
what I mean.

Although we've already sent max+1 HB, but the code set the transport to
INACTIVE state immediately, which is equal to not sending the max+1 HB
at all.  We should wait for a next period and make sure the max+1 HB was
not acknowledged.

Ok, I just re-read the section, and although we don't quite wait long
enough to mark the transport DOWN, this patch will cause us to send an
extra HB chunk, thus exceeding pathmaxrxt by 2.

What the spec really says is that an error is incremented when an
outstanding HB is not acknowledged.

So this code needs a bit of a rework.  This patch is still NAKed.

-vlad

Chunbo 

quoted

-vlad

quoted

---
 net/sctp/sm_sideeffect.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 86426aa..0e2e269 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c

@@ -447,7 +447,7 @@ static void sctp_do_8_2_transport_strike(struct sctp_association *asoc,
 		asoc->overall_error_count++;
 
 	if (transport->state != SCTP_INACTIVE &&
-	    (transport->error_count++ >= transport->pathmaxrxt)) {
+	    (transport->error_count++ > transport->pathmaxrxt)) {
 		SCTP_DEBUG_PRINTK_IPADDR("transport_strike:association %p",
 					 " transport IP: port:%d failed.\n",
 					 asoc,