Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACKwhen bundling

[Xufeng Zhang]

On 7/30/12, Xufeng Zhang <xufengzhang.main@xxxxxxxxx> wrote:
> On 7/28/12, Vlad Yasevich <vyasevich@xxxxxxxxx> wrote:
>> here is an untested prototype of what I was talking about.  This should
>> handle multiple data chunks.
>
> Yes, it works if only the end of the DATA chunk in a packet has
> invalid stream identifier
> and I have verified this patch by my test case, but what happens if
> there are multiple
> DATA chunks which have invalid stream identifier in a packet?
>
> Consider the below example:
> A packet has several chunks bundling together: "COOKIE_ECHO DATA DATA",
> both
> of the two DATA chunks have invalid stream identifier, then the
> response will be
> "COOKIE_ACK ERROR SACK ERROR", right?

I just wrote a test case for my above assumption and have verified
that SACK always
bundled before the end of the ERROR chunk if multiple error DATA
chunks happened.

So this patch didn't handle all the situations and this is really what
I suspected before.


Thanks,
Xufeng Zhang
>
>
>
> Thanks,
> Xufeng Zhang
>
>>
>> -vlad
>>
>> ---
>>   include/net/sctp/command.h |    1 +
>>   net/sctp/sm_sideeffect.c   |   22 ++++++++++++++++++++++
>>   net/sctp/sm_statefuns.c    |   18 ++++++++++--------
>>   3 files changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
>> index 712b3be..4043445 100644
>> --- a/include/net/sctp/command.h
>> +++ b/include/net/sctp/command.h
>> @@ -110,6 +110,7 @@ typedef enum {
>>   	SCTP_CMD_SEND_NEXT_ASCONF, /* Send the next ASCONF after ACK */
>>   	SCTP_CMD_PURGE_ASCONF_QUEUE, /* Purge all asconf queues.*/
>>   	SCTP_CMD_SET_ASOC,	 /* Restore association context */
>> +	SCTP_CMD_GEN_BAD_STREAM, /* Issue an Invalid Stream error */
>>   	SCTP_CMD_LAST
>>   } sctp_verb_t;
>>
>> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
>> index 1ff51c9..c5a1322 100644
>> --- a/net/sctp/sm_sideeffect.c
>> +++ b/net/sctp/sm_sideeffect.c
>> @@ -1036,6 +1036,22 @@ static void sctp_cmd_send_asconf(struct
>> sctp_association *asoc)
>>   	}
>>   }
>>
>> +static void sctp_cmd_make_inv_stream_err(sctp_cmd_seq_t *commands,
>> +					 struct sctp_association *asoc,
>> +					 struct sctp_chunk *chunk,
>> +					 struct sctp_datahdr *data_hdr)
>> +{
>> +	struct sctp_chunk *err;
>> +
>> +	err = sctp_make_op_error(asoc, chunk, SCTP_ERROR_INV_STRM,
>> +				 &data_hdr->stream,
>> +				 sizeof(data_hdr->stream),
>> +				 sizeof(u16));
>> +	if (err)
>> +		sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
>> +					SCTP_CHUNK(err));
>> +}
>> +
>>
>>   /* These three macros allow us to pull the debugging code out of the
>>    * main flow of sctp_do_sm() to keep attention focused on the real
>> @@ -1700,6 +1716,12 @@ static int sctp_cmd_interpreter(sctp_event_t
>> event_type,
>>   			asoc = cmd->obj.asoc;
>>   			break;
>>
>> +		case SCTP_CMD_GEN_BAD_STREAM:
>> +			sctp_cmd_make_inv_stream_err(commands,
>> +					 asoc, chunk,
>> +					 (struct sctp_datahdr *)cmd->obj.ptr);
>> +			break;
>> +
>>   		default:
>>   			pr_warn("Impossible command: %u, %p\n",
>>   				cmd->verb, cmd->obj.ptr);
>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
>> index 891f5db..57532e3 100644
>> --- a/net/sctp/sm_statefuns.c
>> +++ b/net/sctp/sm_statefuns.c
>> @@ -2972,6 +2972,12 @@ discard_noforce:
>>   	if (chunk->end_of_packet)
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force);
>>
>> +	/* Queue the INVALID STREAM error after the SACK if one is needed. */
>> +	if (SCTP_IERROR_BAD_STREAM == error) {
>> +		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
>> +				SCTP_PTR(chunk->subh.data_hdr));
>> +	}
>> +
>>   	return SCTP_DISPOSITION_DISCARD;
>>   consume:
>>   	return SCTP_DISPOSITION_CONSUME;
>> @@ -3044,6 +3050,10 @@ sctp_disposition_t
>> sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep,
>>   		 */
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SHUTDOWN, SCTP_NULL());
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE());
>> +		if (SCTP_IERROR_BAD_STREAM == error) {
>> +			sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM,
>> +					SCTP_PTR(chunk->subh.data_hdr));
>> +		}
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
>>   				SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
>>   	}
>> @@ -6140,14 +6150,6 @@ static int sctp_eat_data(const struct
>> sctp_association *asoc,
>>   	if (sid >= asoc->c.sinit_max_instreams) {
>>   		/* Mark tsn as received even though we drop it */
>>   		sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_TSN, SCTP_U32(tsn));
>> -
>> -		err = sctp_make_op_error(asoc, chunk, SCTP_ERROR_INV_STRM,
>> -					 &data_hdr->stream,
>> -					 sizeof(data_hdr->stream),
>> -					 sizeof(u16));
>> -		if (err)
>> -			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
>> -					SCTP_CHUNK(err));
>>   		return SCTP_IERROR_BAD_STREAM;
>>   	}
>>
>> -- 1.7.7.6
>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/