netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] sctp: send abort chunk when max_retrans exceeded
@ 2012-11-20 17:59 Neil Horman
  2012-11-20 18:44 ` Vlad Yasevich
  2012-11-20 20:14 ` [PATCH v2] " Neil Horman
  0 siblings, 2 replies; 9+ messages in thread
From: Neil Horman @ 2012-11-20 17:59 UTC (permalink / raw)
  To: netdev; +Cc: Neil Horman, Vlad Yasevich, David S. Miller, linux-sctp

In the event that an association exceeds its max_retrans attempts, we should
send an ABORT chunk indicating that we are closing the assocation as a result.
Because of the nature of the error, its unlikely to be received, but its a nice
clean way to close the association if it does make it through, and it will give
anyone watching via tcpdump a clue as to what happened.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Vlad Yasevich <vyasevich@gmail.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
 include/net/sctp/sm.h    |  2 ++
 net/sctp/sm_make_chunk.c | 26 +++++++++++++++++++++-----
 net/sctp/sm_sideeffect.c |  9 ++++++++-
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index b5887e1..2a82d13 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -234,6 +234,8 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
 struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
 				   const struct sctp_chunk *,
 				   struct sctp_paramhdr *);
+struct sctp_chunk *sctp_make_violation_max_retrans(const struct sctp_association *,
+						   const struct sctp_chunk *);
 struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
 				  const struct sctp_transport *);
 struct sctp_chunk *sctp_make_heartbeat_ack(const struct sctp_association *,
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fbe1636..d6a8c80 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1074,17 +1074,33 @@ struct sctp_chunk *sctp_make_violation_paramlen(
 {
 	struct sctp_chunk *retval;
 	static const char error[] = "The following parameter had invalid length:";
-	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t) +
-				sizeof(sctp_paramhdr_t);
+	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
 
 	retval = sctp_make_abort(asoc, chunk, payload_len);
 	if (!retval)
 		goto nodata;
 
-	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
-			sizeof(error) + sizeof(sctp_paramhdr_t));
+	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
+	sctp_addto_chunk(retval, sizeof(error), error);
+
+nodata:
+	return retval;
+}
+
+struct sctp_chunk *sctp_make_violation_max_retrans(
+	const struct sctp_association *asoc,
+	const struct sctp_chunk *chunk)
+{
+	struct sctp_chunk *retval;
+	static const char error[] = "Association exceeded its max_retans count";
+	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
+
+	retval = sctp_make_abort(asoc, chunk, payload_len);
+	if (!retval)
+		goto nodata;
+
+	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
 	sctp_addto_chunk(retval, sizeof(error), error);
-	sctp_addto_param(retval, sizeof(sctp_paramhdr_t), param);
 
 nodata:
 	return retval;
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 6eecf7e..c076956 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -577,7 +577,7 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
 				  unsigned int error)
 {
 	struct sctp_ulpevent *event;
-
+	struct sctp_chunk *abort;
 	/* Cancel any partial delivery in progress. */
 	sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
 
@@ -593,6 +593,13 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
 		sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP,
 				SCTP_ULPEVENT(event));
 
+	if (asoc->overall_error_count >= asoc->max_retrans) {
+		abort = sctp_make_violation_max_retrans(asoc, chunk);
+		if (abort)
+			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
+					SCTP_CHUNK(abort));
+	}
+
 	sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
 			SCTP_STATE(SCTP_STATE_CLOSED));
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 17:59 [PATCH] sctp: send abort chunk when max_retrans exceeded Neil Horman
@ 2012-11-20 18:44 ` Vlad Yasevich
  2012-11-20 19:15   ` Neil Horman
  2012-11-20 20:14 ` [PATCH v2] " Neil Horman
  1 sibling, 1 reply; 9+ messages in thread
From: Vlad Yasevich @ 2012-11-20 18:44 UTC (permalink / raw)
  To: Neil Horman; +Cc: netdev, David S. Miller, linux-sctp

On 11/20/2012 12:59 PM, Neil Horman wrote:
> In the event that an association exceeds its max_retrans attempts, we should
> send an ABORT chunk indicating that we are closing the assocation as a result.
> Because of the nature of the error, its unlikely to be received, but its a nice
> clean way to close the association if it does make it through, and it will give
> anyone watching via tcpdump a clue as to what happened.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Vlad Yasevich <vyasevich@gmail.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: linux-sctp@vger.kernel.org
> ---
>   include/net/sctp/sm.h    |  2 ++
>   net/sctp/sm_make_chunk.c | 26 +++++++++++++++++++++-----
>   net/sctp/sm_sideeffect.c |  9 ++++++++-
>   3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index b5887e1..2a82d13 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -234,6 +234,8 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
>   struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
>   				   const struct sctp_chunk *,
>   				   struct sctp_paramhdr *);
> +struct sctp_chunk *sctp_make_violation_max_retrans(const struct sctp_association *,
> +						   const struct sctp_chunk *);
>   struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
>   				  const struct sctp_transport *);
>   struct sctp_chunk *sctp_make_heartbeat_ack(const struct sctp_association *,
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index fbe1636..d6a8c80 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1074,17 +1074,33 @@ struct sctp_chunk *sctp_make_violation_paramlen(
>   {
>   	struct sctp_chunk *retval;
>   	static const char error[] = "The following parameter had invalid length:";
> -	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t) +
> -				sizeof(sctp_paramhdr_t);
> +	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
>
>   	retval = sctp_make_abort(asoc, chunk, payload_len);
>   	if (!retval)
>   		goto nodata;
>
> -	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
> -			sizeof(error) + sizeof(sctp_paramhdr_t));
> +	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
> +	sctp_addto_chunk(retval, sizeof(error), error);
> +
> +nodata:
> +	return retval;
> +}

Neil

You ended dropping the parameter information of the parameter that 
caused the violation.  Was that intentional?

-vlad

> +
> +struct sctp_chunk *sctp_make_violation_max_retrans(
> +	const struct sctp_association *asoc,
> +	const struct sctp_chunk *chunk)
> +{
> +	struct sctp_chunk *retval;
> +	static const char error[] = "Association exceeded its max_retans count";
> +	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
> +
> +	retval = sctp_make_abort(asoc, chunk, payload_len);
> +	if (!retval)
> +		goto nodata;
> +
> +	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
>   	sctp_addto_chunk(retval, sizeof(error), error);
> -	sctp_addto_param(retval, sizeof(sctp_paramhdr_t), param);
>
>   nodata:
>   	return retval;
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 6eecf7e..c076956 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -577,7 +577,7 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
>   				  unsigned int error)
>   {
>   	struct sctp_ulpevent *event;
> -
> +	struct sctp_chunk *abort;
>   	/* Cancel any partial delivery in progress. */
>   	sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
>
> @@ -593,6 +593,13 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
>   		sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP,
>   				SCTP_ULPEVENT(event));
>
> +	if (asoc->overall_error_count >= asoc->max_retrans) {
> +		abort = sctp_make_violation_max_retrans(asoc, chunk);
> +		if (abort)
> +			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
> +					SCTP_CHUNK(abort));
> +	}
> +
>   	sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
>   			SCTP_STATE(SCTP_STATE_CLOSED));
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 18:44 ` Vlad Yasevich
@ 2012-11-20 19:15   ` Neil Horman
  2012-11-20 19:28     ` Vlad Yasevich
  0 siblings, 1 reply; 9+ messages in thread
From: Neil Horman @ 2012-11-20 19:15 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: netdev, David S. Miller, linux-sctp

On Tue, Nov 20, 2012 at 01:44:23PM -0500, Vlad Yasevich wrote:
> On 11/20/2012 12:59 PM, Neil Horman wrote:
> >In the event that an association exceeds its max_retrans attempts, we should
> >send an ABORT chunk indicating that we are closing the assocation as a result.
> >Because of the nature of the error, its unlikely to be received, but its a nice
> >clean way to close the association if it does make it through, and it will give
> >anyone watching via tcpdump a clue as to what happened.
> >
> >Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> >CC: Vlad Yasevich <vyasevich@gmail.com>
> >CC: "David S. Miller" <davem@davemloft.net>
> >CC: linux-sctp@vger.kernel.org
> >---
> >  include/net/sctp/sm.h    |  2 ++
> >  net/sctp/sm_make_chunk.c | 26 +++++++++++++++++++++-----
> >  net/sctp/sm_sideeffect.c |  9 ++++++++-
> >  3 files changed, 31 insertions(+), 6 deletions(-)
> >
> >diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> >index b5887e1..2a82d13 100644
> >--- a/include/net/sctp/sm.h
> >+++ b/include/net/sctp/sm.h
> >@@ -234,6 +234,8 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
> >  struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
> >  				   const struct sctp_chunk *,
> >  				   struct sctp_paramhdr *);
> >+struct sctp_chunk *sctp_make_violation_max_retrans(const struct sctp_association *,
> >+						   const struct sctp_chunk *);
> >  struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
> >  				  const struct sctp_transport *);
> >  struct sctp_chunk *sctp_make_heartbeat_ack(const struct sctp_association *,
> >diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> >index fbe1636..d6a8c80 100644
> >--- a/net/sctp/sm_make_chunk.c
> >+++ b/net/sctp/sm_make_chunk.c
> >@@ -1074,17 +1074,33 @@ struct sctp_chunk *sctp_make_violation_paramlen(
> >  {
> >  	struct sctp_chunk *retval;
> >  	static const char error[] = "The following parameter had invalid length:";
> >-	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t) +
> >-				sizeof(sctp_paramhdr_t);
> >+	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
> >
> >  	retval = sctp_make_abort(asoc, chunk, payload_len);
> >  	if (!retval)
> >  		goto nodata;
> >
> >-	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
> >-			sizeof(error) + sizeof(sctp_paramhdr_t));
> >+	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
> >+	sctp_addto_chunk(retval, sizeof(error), error);
> >+
> >+nodata:
> >+	return retval;
> >+}
> 
> Neil
> 
> You ended dropping the parameter information of the parameter that
> caused the violation.  Was that intentional?
> 
Yes, it was, because theres not really IMO a specific parameter that causes this
abort condition.  If a chunk needs to be resent more than max_retrans times, we
abort the connection, theres no specific parameter that we can point to that
says "this caused the problem", we're just aborting because we can't get a SACK 
from the peer.  Likewise, I can't think of any information that we can include
that would give the peer, or the anyone tcpdumping the connection an improved
view as to why the abort happened, beyond the string this patch currently
includes.

I know I had privately sent you an early version of the patch as a rough draft
which did include space for a param header, but that patch never filled that
space out, since we don't have any valid information to fill it out with.

Thanks & Regards
Neil

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 19:15   ` Neil Horman
@ 2012-11-20 19:28     ` Vlad Yasevich
  2012-11-20 19:52       ` Vlad Yasevich
  0 siblings, 1 reply; 9+ messages in thread
From: Vlad Yasevich @ 2012-11-20 19:28 UTC (permalink / raw)
  To: Neil Horman; +Cc: netdev, David S. Miller, linux-sctp

On 11/20/2012 02:15 PM, Neil Horman wrote:
> On Tue, Nov 20, 2012 at 01:44:23PM -0500, Vlad Yasevich wrote:
>> On 11/20/2012 12:59 PM, Neil Horman wrote:
>>> In the event that an association exceeds its max_retrans attempts, we should
>>> send an ABORT chunk indicating that we are closing the assocation as a result.
>>> Because of the nature of the error, its unlikely to be received, but its a nice
>>> clean way to close the association if it does make it through, and it will give
>>> anyone watching via tcpdump a clue as to what happened.
>>>
>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>> CC: Vlad Yasevich <vyasevich@gmail.com>
>>> CC: "David S. Miller" <davem@davemloft.net>
>>> CC: linux-sctp@vger.kernel.org
>>> ---
>>>   include/net/sctp/sm.h    |  2 ++
>>>   net/sctp/sm_make_chunk.c | 26 +++++++++++++++++++++-----
>>>   net/sctp/sm_sideeffect.c |  9 ++++++++-
>>>   3 files changed, 31 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
>>> index b5887e1..2a82d13 100644
>>> --- a/include/net/sctp/sm.h
>>> +++ b/include/net/sctp/sm.h
>>> @@ -234,6 +234,8 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
>>>   struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
>>>   				   const struct sctp_chunk *,
>>>   				   struct sctp_paramhdr *);
>>> +struct sctp_chunk *sctp_make_violation_max_retrans(const struct sctp_association *,
>>> +						   const struct sctp_chunk *);
>>>   struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
>>>   				  const struct sctp_transport *);
>>>   struct sctp_chunk *sctp_make_heartbeat_ack(const struct sctp_association *,
>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>> index fbe1636..d6a8c80 100644
>>> --- a/net/sctp/sm_make_chunk.c
>>> +++ b/net/sctp/sm_make_chunk.c
>>> @@ -1074,17 +1074,33 @@ struct sctp_chunk *sctp_make_violation_paramlen(
>>>   {
>>>   	struct sctp_chunk *retval;
>>>   	static const char error[] = "The following parameter had invalid length:";
>>> -	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t) +
>>> -				sizeof(sctp_paramhdr_t);
>>> +	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
>>>
>>>   	retval = sctp_make_abort(asoc, chunk, payload_len);
>>>   	if (!retval)
>>>   		goto nodata;
>>>
>>> -	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
>>> -			sizeof(error) + sizeof(sctp_paramhdr_t));
>>> +	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
>>> +	sctp_addto_chunk(retval, sizeof(error), error);
>>> +
>>> +nodata:
>>> +	return retval;
>>> +}
>>
>> Neil
>>
>> You ended dropping the parameter information of the parameter that
>> caused the violation.  Was that intentional?
>>
> Yes, it was, because theres not really IMO a specific parameter that causes this
> abort condition.

Sure there is.  You changed sctp_make_violation_paramlen() which is 
called when we receive a protocol parameter which has an invalid length.
This triggers a violation and the parameter is report back.  This has 
nothing to do with max_rtx overflow.

The new code doesn't have to include parameter information and I am fine 
with that.

-vlad


>  If a chunk needs to be resent more than max_retrans times, we
> abort the connection, theres no specific parameter that we can point to that
> says "this caused the problem", we're just aborting because we can't get a SACK
> from the peer.  Likewise, I can't think of any information that we can include
> that would give the peer, or the anyone tcpdumping the connection an improved
> view as to why the abort happened, beyond the string this patch currently
> includes.
>
> I know I had privately sent you an early version of the patch as a rough draft
> which did include space for a param header, but that patch never filled that
> space out, since we don't have any valid information to fill it out with.
>
> Thanks & Regards
> Neil
>

Neil

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 19:28     ` Vlad Yasevich
@ 2012-11-20 19:52       ` Vlad Yasevich
  2012-11-20 20:07         ` Neil Horman
  0 siblings, 1 reply; 9+ messages in thread
From: Vlad Yasevich @ 2012-11-20 19:52 UTC (permalink / raw)
  To: Neil Horman; +Cc: netdev, David S. Miller, linux-sctp

On 11/20/2012 02:28 PM, Vlad Yasevich wrote:
> On 11/20/2012 02:15 PM, Neil Horman wrote:
>> On Tue, Nov 20, 2012 at 01:44:23PM -0500, Vlad Yasevich wrote:
>>> On 11/20/2012 12:59 PM, Neil Horman wrote:
>>>> In the event that an association exceeds its max_retrans attempts,
>>>> we should
>>>> send an ABORT chunk indicating that we are closing the assocation as
>>>> a result.
>>>> Because of the nature of the error, its unlikely to be received, but
>>>> its a nice
>>>> clean way to close the association if it does make it through, and
>>>> it will give
>>>> anyone watching via tcpdump a clue as to what happened.
>>>>
>>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>>> CC: Vlad Yasevich <vyasevich@gmail.com>
>>>> CC: "David S. Miller" <davem@davemloft.net>
>>>> CC: linux-sctp@vger.kernel.org
>>>> ---
>>>>   include/net/sctp/sm.h    |  2 ++
>>>>   net/sctp/sm_make_chunk.c | 26 +++++++++++++++++++++-----
>>>>   net/sctp/sm_sideeffect.c |  9 ++++++++-
>>>>   3 files changed, 31 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
>>>> index b5887e1..2a82d13 100644
>>>> --- a/include/net/sctp/sm.h
>>>> +++ b/include/net/sctp/sm.h
>>>> @@ -234,6 +234,8 @@ struct sctp_chunk
>>>> *sctp_make_abort_violation(const struct sctp_association *,
>>>>   struct sctp_chunk *sctp_make_violation_paramlen(const struct
>>>> sctp_association *,
>>>>                      const struct sctp_chunk *,
>>>>                      struct sctp_paramhdr *);
>>>> +struct sctp_chunk *sctp_make_violation_max_retrans(const struct
>>>> sctp_association *,
>>>> +                           const struct sctp_chunk *);
>>>>   struct sctp_chunk *sctp_make_heartbeat(const struct
>>>> sctp_association *,
>>>>                     const struct sctp_transport *);
>>>>   struct sctp_chunk *sctp_make_heartbeat_ack(const struct
>>>> sctp_association *,
>>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>>> index fbe1636..d6a8c80 100644
>>>> --- a/net/sctp/sm_make_chunk.c
>>>> +++ b/net/sctp/sm_make_chunk.c
>>>> @@ -1074,17 +1074,33 @@ struct sctp_chunk
>>>> *sctp_make_violation_paramlen(
>>>>   {
>>>>       struct sctp_chunk *retval;
>>>>       static const char error[] = "The following parameter had
>>>> invalid length:";
>>>> -    size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t) +
>>>> -                sizeof(sctp_paramhdr_t);
>>>> +    size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
>>>>
>>>>       retval = sctp_make_abort(asoc, chunk, payload_len);
>>>>       if (!retval)
>>>>           goto nodata;
>>>>
>>>> -    sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
>>>> -            sizeof(error) + sizeof(sctp_paramhdr_t));
>>>> +    sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
>>>> sizeof(error));
>>>> +    sctp_addto_chunk(retval, sizeof(error), error);
>>>> +
>>>> +nodata:
>>>> +    return retval;
>>>> +}
>>>
>>> Neil
>>>
>>> You ended dropping the parameter information of the parameter that
>>> caused the violation.  Was that intentional?
>>>
>> Yes, it was, because theres not really IMO a specific parameter that
>> causes this
>> abort condition.
>
> Sure there is.  You changed sctp_make_violation_paramlen() which is
> called when we receive a protocol parameter which has an invalid length.
> This triggers a violation and the parameter is report back.  This has
> nothing to do with max_rtx overflow.

It looks like you tried to re-use sctp_make_violation_paramlen(), 
abandoned that approach, but forgot to fully restore the old function...

-vlad

>
> The new code doesn't have to include parameter information and I am fine
> with that.
>
> -vlad
>
>
>>  If a chunk needs to be resent more than max_retrans times, we
>> abort the connection, theres no specific parameter that we can point
>> to that
>> says "this caused the problem", we're just aborting because we can't
>> get a SACK
>> from the peer.  Likewise, I can't think of any information that we can
>> include
>> that would give the peer, or the anyone tcpdumping the connection an
>> improved
>> view as to why the abort happened, beyond the string this patch currently
>> includes.
>>
>> I know I had privately sent you an early version of the patch as a
>> rough draft
>> which did include space for a param header, but that patch never
>> filled that
>> space out, since we don't have any valid information to fill it out with.
>>
>> Thanks & Regards
>> Neil
>>
>
> Neil
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 19:52       ` Vlad Yasevich
@ 2012-11-20 20:07         ` Neil Horman
  0 siblings, 0 replies; 9+ messages in thread
From: Neil Horman @ 2012-11-20 20:07 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: netdev, David S. Miller, linux-sctp

On Tue, Nov 20, 2012 at 02:52:06PM -0500, Vlad Yasevich wrote:
> On 11/20/2012 02:28 PM, Vlad Yasevich wrote:
> >On 11/20/2012 02:15 PM, Neil Horman wrote:
> >>On Tue, Nov 20, 2012 at 01:44:23PM -0500, Vlad Yasevich wrote:
> >>>On 11/20/2012 12:59 PM, Neil Horman wrote:
> >>>>In the event that an association exceeds its max_retrans attempts,
> >>>>we should
> >>>>send an ABORT chunk indicating that we are closing the assocation as
> >>>>a result.
> >>>>Because of the nature of the error, its unlikely to be received, but
> >>>>its a nice
> >>>>clean way to close the association if it does make it through, and
> >>>>it will give
> >>>>anyone watching via tcpdump a clue as to what happened.
> >>>>
> >>>>Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> >>>>CC: Vlad Yasevich <vyasevich@gmail.com>
> >>>>CC: "David S. Miller" <davem@davemloft.net>
> >>>>CC: linux-sctp@vger.kernel.org
> >>>>---
> >>>>  include/net/sctp/sm.h    |  2 ++
> >>>>  net/sctp/sm_make_chunk.c | 26 +++++++++++++++++++++-----
> >>>>  net/sctp/sm_sideeffect.c |  9 ++++++++-
> >>>>  3 files changed, 31 insertions(+), 6 deletions(-)
> >>>>
> >>>>diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> >>>>index b5887e1..2a82d13 100644
> >>>>--- a/include/net/sctp/sm.h
> >>>>+++ b/include/net/sctp/sm.h
> >>>>@@ -234,6 +234,8 @@ struct sctp_chunk
> >>>>*sctp_make_abort_violation(const struct sctp_association *,
> >>>>  struct sctp_chunk *sctp_make_violation_paramlen(const struct
> >>>>sctp_association *,
> >>>>                     const struct sctp_chunk *,
> >>>>                     struct sctp_paramhdr *);
> >>>>+struct sctp_chunk *sctp_make_violation_max_retrans(const struct
> >>>>sctp_association *,
> >>>>+                           const struct sctp_chunk *);
> >>>>  struct sctp_chunk *sctp_make_heartbeat(const struct
> >>>>sctp_association *,
> >>>>                    const struct sctp_transport *);
> >>>>  struct sctp_chunk *sctp_make_heartbeat_ack(const struct
> >>>>sctp_association *,
> >>>>diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> >>>>index fbe1636..d6a8c80 100644
> >>>>--- a/net/sctp/sm_make_chunk.c
> >>>>+++ b/net/sctp/sm_make_chunk.c
> >>>>@@ -1074,17 +1074,33 @@ struct sctp_chunk
> >>>>*sctp_make_violation_paramlen(
> >>>>  {
> >>>>      struct sctp_chunk *retval;
> >>>>      static const char error[] = "The following parameter had
> >>>>invalid length:";
> >>>>-    size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t) +
> >>>>-                sizeof(sctp_paramhdr_t);
> >>>>+    size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
> >>>>
> >>>>      retval = sctp_make_abort(asoc, chunk, payload_len);
> >>>>      if (!retval)
> >>>>          goto nodata;
> >>>>
> >>>>-    sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
> >>>>-            sizeof(error) + sizeof(sctp_paramhdr_t));
> >>>>+    sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION,
> >>>>sizeof(error));
> >>>>+    sctp_addto_chunk(retval, sizeof(error), error);
> >>>>+
> >>>>+nodata:
> >>>>+    return retval;
> >>>>+}
> >>>
> >>>Neil
> >>>
> >>>You ended dropping the parameter information of the parameter that
> >>>caused the violation.  Was that intentional?
> >>>
> >>Yes, it was, because theres not really IMO a specific parameter that
> >>causes this
> >>abort condition.
> >
> >Sure there is.  You changed sctp_make_violation_paramlen() which is
> >called when we receive a protocol parameter which has an invalid length.
> >This triggers a violation and the parameter is report back.  This has
> >nothing to do with max_rtx overflow.
> 
> It looks like you tried to re-use sctp_make_violation_paramlen(),
> abandoned that approach, but forgot to fully restore the old
> function...
> 
> -vlad
> 
Oh hell, you're right, my apologies, thats exactly what happened, and I
misunderstood what you were saying.  I thought you were asking why I wasn't
including a parameter segment in the max_retrans abort (because there isn't
any).  I completly missed the fact that I inadvertently mangled the
sctp_violation_paramlen function. 

I'll fix that up and repost.

Thanks
Neil

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH v2] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 17:59 [PATCH] sctp: send abort chunk when max_retrans exceeded Neil Horman
  2012-11-20 18:44 ` Vlad Yasevich
@ 2012-11-20 20:14 ` Neil Horman
  2012-11-20 20:25   ` Vlad Yasevich
  1 sibling, 1 reply; 9+ messages in thread
From: Neil Horman @ 2012-11-20 20:14 UTC (permalink / raw)
  To: netdev; +Cc: Neil Horman, Vlad Yasevich, David S. Miller, linux-sctp

In the event that an association exceeds its max_retrans attempts, we should
send an ABORT chunk indicating that we are closing the assocation as a result.
Because of the nature of the error, its unlikely to be received, but its a nice
clean way to close the association if it does make it through, and it will give
anyone watching via tcpdump a clue as to what happened.

Change notes:
v2)
	* Removed erroneous changes from sctp_make_violation_parmlen

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Vlad Yasevich <vyasevich@gmail.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
 include/net/sctp/sm.h    |  2 ++
 net/sctp/sm_make_chunk.c | 19 +++++++++++++++++++
 net/sctp/sm_sideeffect.c |  9 ++++++++-
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index b5887e1..2a82d13 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -234,6 +234,8 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
 struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
 				   const struct sctp_chunk *,
 				   struct sctp_paramhdr *);
+struct sctp_chunk *sctp_make_violation_max_retrans(const struct sctp_association *,
+						   const struct sctp_chunk *);
 struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
 				  const struct sctp_transport *);
 struct sctp_chunk *sctp_make_heartbeat_ack(const struct sctp_association *,
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fbe1636..e0f01a4 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1090,6 +1090,25 @@ nodata:
 	return retval;
 }
 
+struct sctp_chunk *sctp_make_violation_max_retrans(
+	const struct sctp_association *asoc,
+	const struct sctp_chunk *chunk)
+{
+	struct sctp_chunk *retval;
+	static const char error[] = "Association exceeded its max_retans count";
+	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
+
+	retval = sctp_make_abort(asoc, chunk, payload_len);
+	if (!retval)
+		goto nodata;
+
+	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
+	sctp_addto_chunk(retval, sizeof(error), error);
+
+nodata:
+	return retval;
+}
+
 /* Make a HEARTBEAT chunk.  */
 struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
 				  const struct sctp_transport *transport)
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 6eecf7e..c076956 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -577,7 +577,7 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
 				  unsigned int error)
 {
 	struct sctp_ulpevent *event;
-
+	struct sctp_chunk *abort;
 	/* Cancel any partial delivery in progress. */
 	sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
 
@@ -593,6 +593,13 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
 		sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP,
 				SCTP_ULPEVENT(event));
 
+	if (asoc->overall_error_count >= asoc->max_retrans) {
+		abort = sctp_make_violation_max_retrans(asoc, chunk);
+		if (abort)
+			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
+					SCTP_CHUNK(abort));
+	}
+
 	sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
 			SCTP_STATE(SCTP_STATE_CLOSED));
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH v2] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 20:14 ` [PATCH v2] " Neil Horman
@ 2012-11-20 20:25   ` Vlad Yasevich
  2012-11-20 20:51     ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Vlad Yasevich @ 2012-11-20 20:25 UTC (permalink / raw)
  To: Neil Horman; +Cc: netdev, David S. Miller, linux-sctp

On 11/20/2012 03:14 PM, Neil Horman wrote:
> In the event that an association exceeds its max_retrans attempts, we should
> send an ABORT chunk indicating that we are closing the assocation as a result.
> Because of the nature of the error, its unlikely to be received, but its a nice
> clean way to close the association if it does make it through, and it will give
> anyone watching via tcpdump a clue as to what happened.
>
> Change notes:
> v2)
> 	* Removed erroneous changes from sctp_make_violation_parmlen
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Vlad Yasevich <vyasevich@gmail.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> CC: "David S. Miller" <davem@davemloft.net>
> CC: linux-sctp@vger.kernel.org
> ---
>   include/net/sctp/sm.h    |  2 ++
>   net/sctp/sm_make_chunk.c | 19 +++++++++++++++++++
>   net/sctp/sm_sideeffect.c |  9 ++++++++-
>   3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index b5887e1..2a82d13 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -234,6 +234,8 @@ struct sctp_chunk *sctp_make_abort_violation(const struct sctp_association *,
>   struct sctp_chunk *sctp_make_violation_paramlen(const struct sctp_association *,
>   				   const struct sctp_chunk *,
>   				   struct sctp_paramhdr *);
> +struct sctp_chunk *sctp_make_violation_max_retrans(const struct sctp_association *,
> +						   const struct sctp_chunk *);
>   struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *,
>   				  const struct sctp_transport *);
>   struct sctp_chunk *sctp_make_heartbeat_ack(const struct sctp_association *,
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index fbe1636..e0f01a4 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1090,6 +1090,25 @@ nodata:
>   	return retval;
>   }
>
> +struct sctp_chunk *sctp_make_violation_max_retrans(
> +	const struct sctp_association *asoc,
> +	const struct sctp_chunk *chunk)
> +{
> +	struct sctp_chunk *retval;
> +	static const char error[] = "Association exceeded its max_retans count";
> +	size_t payload_len = sizeof(error) + sizeof(sctp_errhdr_t);
> +
> +	retval = sctp_make_abort(asoc, chunk, payload_len);
> +	if (!retval)
> +		goto nodata;
> +
> +	sctp_init_cause(retval, SCTP_ERROR_PROTO_VIOLATION, sizeof(error));
> +	sctp_addto_chunk(retval, sizeof(error), error);
> +
> +nodata:
> +	return retval;
> +}
> +
>   /* Make a HEARTBEAT chunk.  */
>   struct sctp_chunk *sctp_make_heartbeat(const struct sctp_association *asoc,
>   				  const struct sctp_transport *transport)
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 6eecf7e..c076956 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -577,7 +577,7 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
>   				  unsigned int error)
>   {
>   	struct sctp_ulpevent *event;
> -
> +	struct sctp_chunk *abort;
>   	/* Cancel any partial delivery in progress. */
>   	sctp_ulpq_abort_pd(&asoc->ulpq, GFP_ATOMIC);
>
> @@ -593,6 +593,13 @@ static void sctp_cmd_assoc_failed(sctp_cmd_seq_t *commands,
>   		sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP,
>   				SCTP_ULPEVENT(event));
>
> +	if (asoc->overall_error_count >= asoc->max_retrans) {
> +		abort = sctp_make_violation_max_retrans(asoc, chunk);
> +		if (abort)
> +			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
> +					SCTP_CHUNK(abort));
> +	}
> +
>   	sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
>   			SCTP_STATE(SCTP_STATE_CLOSED));
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v2] sctp: send abort chunk when max_retrans exceeded
  2012-11-20 20:25   ` Vlad Yasevich
@ 2012-11-20 20:51     ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2012-11-20 20:51 UTC (permalink / raw)
  To: vyasevich; +Cc: nhorman, netdev, linux-sctp

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Tue, 20 Nov 2012 15:25:56 -0500

> On 11/20/2012 03:14 PM, Neil Horman wrote:
>> In the event that an association exceeds its max_retrans attempts, we
>> should
>> send an ABORT chunk indicating that we are closing the assocation as a
>> result.
>> Because of the nature of the error, its unlikely to be received, but
>> its a nice
>> clean way to close the association if it does make it through, and it
>> will give
>> anyone watching via tcpdump a clue as to what happened.
>>
>> Change notes:
>> v2)
>> 	* Removed erroneous changes from sctp_make_violation_parmlen
>>
>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>> CC: Vlad Yasevich <vyasevich@gmail.com>
> 
> Acked-by: Vlad Yasevich <vyasevich@gmail.com>

Applied, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-11-20 20:51 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-20 17:59 [PATCH] sctp: send abort chunk when max_retrans exceeded Neil Horman
2012-11-20 18:44 ` Vlad Yasevich
2012-11-20 19:15   ` Neil Horman
2012-11-20 19:28     ` Vlad Yasevich
2012-11-20 19:52       ` Vlad Yasevich
2012-11-20 20:07         ` Neil Horman
2012-11-20 20:14 ` [PATCH v2] " Neil Horman
2012-11-20 20:25   ` Vlad Yasevich
2012-11-20 20:51     ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).