Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 08/03/2012 05:17 AM, Vlad Yasevich wrote: On 07/31/2012 02:51 AM, xufeng zhang wrote: Sorry, please ignore the above patch, there was an paste error. Please check the following patch. I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. I started thinking along the same vein, but was thinking that maybe it makes sense to make error list more generic. I need to check the spec on the ordering of ERROR chunks. If they are always after other control chunks, then maybe make an error list and queue all errors there. Then when sending control chunks, drain the control queue first, then the error queue, and finally the data queue. I didn't find the explicitly bundling order description in spec for other ERROR chunks, but it's reasonable to do this. Anyway, I'll wait for your final solution. Thanks, Xufeng Zhang BTW, the patch below doesn't include the code to queue the error chunk onto the new error queue. -vlad Thanks, Xufeng Zhang --- include/net/sctp/command.h |1 + include/net/sctp/structs.h |3 +++ net/sctp/outqueue.c|7 +++ net/sctp/sm_sideeffect.c | 16 net/sctp/sm_statefuns.c| 17 ++--- 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(>retransmit); INIT_LIST_HEAD(>sacked); INIT_LIST_HEAD(>abandoned); + INIT_LIST_HEAD(>bad_stream_err); q->fast_rtx = 0; q->outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(>list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp,>bad_stream_err, list) { + list_del_init(>list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..4698593 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,18 @@ 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 *err, *tmp; + struct sctp_outq *q =>outqueue; + + list_for_each_entry_safe(err, tmp,>bad_stream_err, list) { + list_del_init(>list); + 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 @@ -1724,6 +1736,10 @@ 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); + 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 9fca103..1c1bcd9 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: - if
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 07/31/2012 02:51 AM, xufeng zhang wrote: Sorry, please ignore the above patch, there was an paste error. Please check the following patch. I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. I started thinking along the same vein, but was thinking that maybe it makes sense to make error list more generic. I need to check the spec on the ordering of ERROR chunks. If they are always after other control chunks, then maybe make an error list and queue all errors there. Then when sending control chunks, drain the control queue first, then the error queue, and finally the data queue. BTW, the patch below doesn't include the code to queue the error chunk onto the new error queue. -vlad Thanks, Xufeng Zhang --- include/net/sctp/command.h |1 + include/net/sctp/structs.h |3 +++ net/sctp/outqueue.c|7 +++ net/sctp/sm_sideeffect.c | 16 net/sctp/sm_statefuns.c| 17 ++--- 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(>retransmit); INIT_LIST_HEAD(>sacked); INIT_LIST_HEAD(>abandoned); + INIT_LIST_HEAD(>bad_stream_err); q->fast_rtx = 0; q->outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(>list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp,>bad_stream_err, list) { + list_del_init(>list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..4698593 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,18 @@ 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 *err, *tmp; + struct sctp_outq *q =>outqueue; + + list_for_each_entry_safe(err, tmp,>bad_stream_err, list) { + list_del_init(>list); + 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 @@ -1724,6 +1736,10 @@ 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); + 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 9fca103..1c1bcd9 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: - if (chunk->end_of_packet) + if (chunk->end_of_packet) { + struct sctp_outq *q =>outqueue; sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force); + /* Queue the INVALID STREAM error after the SACK if
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 07/31/2012 02:51 AM, xufeng zhang wrote: Sorry, please ignore the above patch, there was an paste error. Please check the following patch. I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. I started thinking along the same vein, but was thinking that maybe it makes sense to make error list more generic. I need to check the spec on the ordering of ERROR chunks. If they are always after other control chunks, then maybe make an error list and queue all errors there. Then when sending control chunks, drain the control queue first, then the error queue, and finally the data queue. BTW, the patch below doesn't include the code to queue the error chunk onto the new error queue. -vlad Thanks, Xufeng Zhang --- include/net/sctp/command.h |1 + include/net/sctp/structs.h |3 +++ net/sctp/outqueue.c|7 +++ net/sctp/sm_sideeffect.c | 16 net/sctp/sm_statefuns.c| 17 ++--- 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(q-retransmit); INIT_LIST_HEAD(q-sacked); INIT_LIST_HEAD(q-abandoned); + INIT_LIST_HEAD(q-bad_stream_err); q-fast_rtx = 0; q-outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(chunk-list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp,q-bad_stream_err, list) { + list_del_init(chunk-list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..4698593 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,18 @@ 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 *err, *tmp; + struct sctp_outq *q =asoc-outqueue; + + list_for_each_entry_safe(err, tmp,q-bad_stream_err, list) { + list_del_init(err-list); + 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 @@ -1724,6 +1736,10 @@ 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); + 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 9fca103..1c1bcd9 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: - if (chunk-end_of_packet) + if (chunk-end_of_packet) { + struct sctp_outq *q =asoc-outqueue; sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force); + /* Queue the INVALID STREAM error
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 08/03/2012 05:17 AM, Vlad Yasevich wrote: On 07/31/2012 02:51 AM, xufeng zhang wrote: Sorry, please ignore the above patch, there was an paste error. Please check the following patch. I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. I started thinking along the same vein, but was thinking that maybe it makes sense to make error list more generic. I need to check the spec on the ordering of ERROR chunks. If they are always after other control chunks, then maybe make an error list and queue all errors there. Then when sending control chunks, drain the control queue first, then the error queue, and finally the data queue. I didn't find the explicitly bundling order description in spec for other ERROR chunks, but it's reasonable to do this. Anyway, I'll wait for your final solution. Thanks, Xufeng Zhang BTW, the patch below doesn't include the code to queue the error chunk onto the new error queue. -vlad Thanks, Xufeng Zhang --- include/net/sctp/command.h |1 + include/net/sctp/structs.h |3 +++ net/sctp/outqueue.c|7 +++ net/sctp/sm_sideeffect.c | 16 net/sctp/sm_statefuns.c| 17 ++--- 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(q-retransmit); INIT_LIST_HEAD(q-sacked); INIT_LIST_HEAD(q-abandoned); + INIT_LIST_HEAD(q-bad_stream_err); q-fast_rtx = 0; q-outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(chunk-list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp,q-bad_stream_err, list) { + list_del_init(chunk-list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..4698593 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,18 @@ 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 *err, *tmp; + struct sctp_outq *q =asoc-outqueue; + + list_for_each_entry_safe(err, tmp,q-bad_stream_err, list) { + list_del_init(err-list); + 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 @@ -1724,6 +1736,10 @@ 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); + 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 9fca103..1c1bcd9 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: -
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
Sorry, please ignore the above patch, there was an paste error. Please check the following patch. I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. Thanks, Xufeng Zhang --- include/net/sctp/command.h |1 + include/net/sctp/structs.h |3 +++ net/sctp/outqueue.c|7 +++ net/sctp/sm_sideeffect.c | 16 net/sctp/sm_statefuns.c| 17 ++--- 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(>retransmit); INIT_LIST_HEAD(>sacked); INIT_LIST_HEAD(>abandoned); + INIT_LIST_HEAD(>bad_stream_err); q->fast_rtx = 0; q->outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(>list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp,>bad_stream_err, list) { + list_del_init(>list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..4698593 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,18 @@ 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 *err, *tmp; + struct sctp_outq *q =>outqueue; + + list_for_each_entry_safe(err, tmp,>bad_stream_err, list) { + list_del_init(>list); + 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 @@ -1724,6 +1736,10 @@ 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); + 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 9fca103..1c1bcd9 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: - if (chunk->end_of_packet) + if (chunk->end_of_packet) { + struct sctp_outq *q =>outqueue; sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force); + /* Queue the INVALID STREAM error after the SACK if one is needed. */ + if (!list_empty(>bad_stream_err)) + sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM, + SCTP_NULL()); + } return SCTP_DISPOSITION_DISCARD; consume: @@ -3037,11 +3043,16 @@ sctp_disposition_t sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep, * with a SACK, a SHUTDOWN chunk, and restart the T2-shutdown timer */ if (chunk->end_of_packet) { + struct sctp_outq *q =>outqueue; /* We must delay the chunk creation since
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. Thanks, Xufeng Zhang --- diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(>retransmit); INIT_LIST_HEAD(>sacked); INIT_LIST_HEAD(>abandoned); + INIT_LIST_HEAD(>bad_stream_err); q->fast_rtx = 0; q->outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(>list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp, >bad_stream_err, list) { + list_del_init(>list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..ab63fa1 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,17 @@ 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 *err, *tmp; + struct sctp_outq *q = >outqueue; + + list_for_each_entry_safe(err, tmp, >bad_stream_err, list) { + list_del_init(>list); + sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)); + struct sctp_association *asoc) +{ + struct sctp_chunk *err, *tmp; + struct sctp_outq *q = >outqueue; + + list_for_each_entry_safe(err, tmp, >bad_stream_err, list) { + list_del_init(>list); + 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 @@ -1724,6 +1735,10 @@ 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); + 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 9fca103..cab539f 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: - if (chunk->end_of_packet) + if (chunk->end_of_packet) { + struct sctp_outq *q = >outqueue; + sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force); + /* Queue the INVALID STREAM error after the SACK if one is needed. */ + if (!list_empty(>bad_stream_err)) + sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM, SCTP_NULL()); + } return SCTP_DISPOSITION_DISCARD; consume: @@ -3037,11 +3043,15 @@ sctp_disposition_t sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep, * with a SACK, a SHUTDOWN chunk, and restart the T2-shutdown timer */ if (chunk->end_of_packet) { + struct sctp_outq *q = >outqueue; /* We must delay the chunk creation since the cumulative * TSN has not been updated yet. */ sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SHUTDOWN, SCTP_NULL());
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. Thanks, Xufeng Zhang --- diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(q-retransmit); INIT_LIST_HEAD(q-sacked); INIT_LIST_HEAD(q-abandoned); + INIT_LIST_HEAD(q-bad_stream_err); q-fast_rtx = 0; q-outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(chunk-list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp, q-bad_stream_err, list) { + list_del_init(chunk-list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..ab63fa1 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,17 @@ 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 *err, *tmp; + struct sctp_outq *q = asoc-outqueue; + + list_for_each_entry_safe(err, tmp, q-bad_stream_err, list) { + list_del_init(err-list); + sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)); + struct sctp_association *asoc) +{ + struct sctp_chunk *err, *tmp; + struct sctp_outq *q = asoc-outqueue; + + list_for_each_entry_safe(err, tmp, q-bad_stream_err, list) { + list_del_init(err-list); + 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 @@ -1724,6 +1735,10 @@ 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); + 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 9fca103..cab539f 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: - if (chunk-end_of_packet) + if (chunk-end_of_packet) { + struct sctp_outq *q = asoc-outqueue; + sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force); + /* Queue the INVALID STREAM error after the SACK if one is needed. */ + if (!list_empty(q-bad_stream_err)) + sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM, SCTP_NULL()); + } return SCTP_DISPOSITION_DISCARD; consume: @@ -3037,11 +3043,15 @@ sctp_disposition_t sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep, * with a SACK, a SHUTDOWN chunk, and restart the T2-shutdown timer */ if (chunk-end_of_packet) { + struct sctp_outq *q = asoc-outqueue; /* We must delay the chunk creation since the cumulative * TSN has not been updated yet. */ sctp_add_cmd_sf(commands,
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
Sorry, please ignore the above patch, there was an paste error. Please check the following patch. I'm wondering if the below solution is fine to you which is based on your changes. BTW, I have verified this patch and it works ok for all the situation, but only one problem persists: there is a potential that commands will exceeds SCTP_MAX_NUM_COMMANDS which happens during sending lots of small error DATA chunks. Thanks, Xufeng Zhang --- include/net/sctp/command.h |1 + include/net/sctp/structs.h |3 +++ net/sctp/outqueue.c|7 +++ net/sctp/sm_sideeffect.c | 16 net/sctp/sm_statefuns.c| 17 ++--- 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h index 712b3be..62c34f5 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, /* Invalid Stream errors happened command */ SCTP_CMD_LAST } sctp_verb_t; diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index fc5e600..3d218e0 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1183,6 +1183,9 @@ struct sctp_outq { */ struct list_head abandoned; + /* Put Invalid Stream error chunks on this list */ + struct list_head bad_stream_err; + /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177..1e87b0b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -211,6 +211,7 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q) INIT_LIST_HEAD(q-retransmit); INIT_LIST_HEAD(q-sacked); INIT_LIST_HEAD(q-abandoned); + INIT_LIST_HEAD(q-bad_stream_err); q-fast_rtx = 0; q-outstanding_bytes = 0; @@ -283,6 +284,12 @@ void sctp_outq_teardown(struct sctp_outq *q) list_del_init(chunk-list); sctp_chunk_free(chunk); } + + /* Throw away any pending Invalid Stream error chunks */ + list_for_each_entry_safe(chunk, tmp,q-bad_stream_err, list) { + list_del_init(chunk-list); + sctp_chunk_free(chunk); + } } /* Free the outqueue structure and any related pending chunks. */ diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c index fe99628..4698593 100644 --- a/net/sctp/sm_sideeffect.c +++ b/net/sctp/sm_sideeffect.c @@ -1060,6 +1060,18 @@ 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 *err, *tmp; + struct sctp_outq *q =asoc-outqueue; + + list_for_each_entry_safe(err, tmp,q-bad_stream_err, list) { + list_del_init(err-list); + 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 @@ -1724,6 +1736,10 @@ 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); + 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 9fca103..1c1bcd9 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -2967,8 +2967,14 @@ discard_force: return SCTP_DISPOSITION_DISCARD; discard_noforce: - if (chunk-end_of_packet) + if (chunk-end_of_packet) { + struct sctp_outq *q =asoc-outqueue; sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, force); + /* Queue the INVALID STREAM error after the SACK if one is needed. */ + if (!list_empty(q-bad_stream_err)) + sctp_add_cmd_sf(commands, SCTP_CMD_GEN_BAD_STREAM, + SCTP_NULL()); + } return SCTP_DISPOSITION_DISCARD; consume: @@ -3037,11 +3043,16 @@ sctp_disposition_t sctp_sf_eat_data_fast_4_4(const struct sctp_endpoint *ep, * with a SACK, a SHUTDOWN chunk, and restart the T2-shutdown timer */ if (chunk-end_of_packet) { + struct sctp_outq *q =asoc-outqueue; /* We must delay the
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/30/12, Xufeng Zhang wrote: > On 7/28/12, Vlad Yasevich 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, >> + _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
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/28/12, Vlad Yasevich 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? 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, > + _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, > - _hdr->stream, > - sizeof(data_hdr->stream), > - sizeof(u16)); > - if (err) > -
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/28/12, Vlad Yasevich vyasev...@gmail.com 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? 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));
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/30/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/28/12, Vlad Yasevich vyasev...@gmail.com 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, -
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
here is an untested prototype of what I was talking about. This should handle multiple data chunks. -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, +_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, -_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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
here is an untested prototype of what I was talking about. This should handle multiple data chunks. -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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/26/12, Vlad Yasevich wrote: > Xufeng Zhang wrote: > >>On 7/26/12, Xufeng Zhang wrote: >>> On 7/25/12, Vlad Yasevich wrote: > And after take a moment to look into the relative codes, I think we > can implement it > by below way: > 1). Add a flag(isi_err_needed) in the embedded struct peer of >>struct > struct sctp_association > just like sack_needed flag. > 2). When "invalid stream identifier" ERROR happens in >>sctp_eat_data() > function, we just > set isi_err_needed flag and don't create ERROR chunk and also don't > insert SCTP_CMD_REPLY command. > 3). In sctp_gen_sack() function, we create ERROR chunk and also >>insert > SCTP_CMD_REPLY command if isi_err_needed flag is set. > > Is this way proper? > So, I looked at the code, and it looks very simple to do. We >>already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the >>error and adds it to the command list and give it its own small function >>that you can then call if SCTP_IERROR_BAD_STREAM error was returned. >>> >>> No, it will still has the same problem by just doing this. >>> SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, >>> sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK >>command >>> in sctp_cmd_interpreter(). >>> So it's not enough if we just insert SCTP_ERROR_INV_STRM command >>after >>> sctp_eat_data() return SCTP_IERROR_BAD_STREAM in >>sctp_sf_eat_data_6_2(). >> >>Yes, I just tried this way, SACK is still bundled after ERROR chunk. >>But I think my above method is also not fine if there are multiple >>error DATA chunks >>bundled in a packet. >>Really awesome! > > 1. Catch the error return. > 2. Set flag indicating error is needed. > 3. Queue sack as needed. > 4. If error flag set call new function to queue error chunk. Both step 3 and 4 need lots of changes to make it working since SACK is ready to queue only at the end of the packet and we also need to deal with multiple "invalid stream identifier" ERROR chunks in a single packet. Thanks, Xufeng Zhang > > That should fix things. Do this in all callers of sctp_eat_data. > > -vlad > >> >> >>Thanks, >>Xufeng Zhang >> >>> >>> >>> >>> Thanks, >>> Xufeng Zhang >>> -vlad > > Thanks, > Xufeng Zhang >> >> >> >> Thanks, >> Xufeng Zhang >>> >>> -vlad Thanks, Xufeng Zhang > > -vlad >>> >>> >>> -- >>> Sent from my Android phone with SkitMail. Please excuse my >>brevity. >>> >> >>> > > > -- > Sent from my Android phone with SkitMail. Please excuse my brevity. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
Xufeng Zhang wrote: >On 7/26/12, Xufeng Zhang wrote: >> On 7/25/12, Vlad Yasevich wrote: And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of >struct struct sctp_association just like sack_needed flag. 2). When "invalid stream identifier" ERROR happens in >sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also >insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? >>> >>> So, I looked at the code, and it looks very simple to do. We >already >>> return a specific status from sctp_eat_data() when the error was >>> generated. All you have to do is take the code that generates the >error >>> and adds it to the command list and give it its own small function >that >>> you can then call if SCTP_IERROR_BAD_STREAM error was returned. >> >> No, it will still has the same problem by just doing this. >> SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, >> sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK >command >> in sctp_cmd_interpreter(). >> So it's not enough if we just insert SCTP_ERROR_INV_STRM command >after >> sctp_eat_data() return SCTP_IERROR_BAD_STREAM in >sctp_sf_eat_data_6_2(). > >Yes, I just tried this way, SACK is still bundled after ERROR chunk. >But I think my above method is also not fine if there are multiple >error DATA chunks >bundled in a packet. >Really awesome! 1. Catch the error return. 2. Set flag indicating error is needed. 3. Queue sack as needed. 4. If error flag set call new function to queue error chunk. That should fix things. Do this in all callers of sctp_eat_data. -vlad > > >Thanks, >Xufeng Zhang > >> >> >> >> Thanks, >> Xufeng Zhang >> >>> >>> -vlad >>> Thanks, Xufeng Zhang > > > > Thanks, > Xufeng Zhang >> >> -vlad >>> >>> Thanks, >>> Xufeng Zhang -vlad >> >> >> -- >> Sent from my Android phone with SkitMail. Please excuse my >brevity. >> > >>> >>> >>> >> -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/26/12, Xufeng Zhang wrote: > On 7/25/12, Vlad Yasevich wrote: >>> And after take a moment to look into the relative codes, I think we >>> can implement it >>> by below way: >>> 1). Add a flag(isi_err_needed) in the embedded struct peer of struct >>> struct sctp_association >>> just like sack_needed flag. >>> 2). When "invalid stream identifier" ERROR happens in sctp_eat_data() >>> function, we just >>> set isi_err_needed flag and don't create ERROR chunk and also don't >>> insert SCTP_CMD_REPLY command. >>> 3). In sctp_gen_sack() function, we create ERROR chunk and also insert >>> SCTP_CMD_REPLY command if isi_err_needed flag is set. >>> >>> Is this way proper? >>> >> >> So, I looked at the code, and it looks very simple to do. We already >> return a specific status from sctp_eat_data() when the error was >> generated. All you have to do is take the code that generates the error >> and adds it to the command list and give it its own small function that >> you can then call if SCTP_IERROR_BAD_STREAM error was returned. > > No, it will still has the same problem by just doing this. > SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, > sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK command > in sctp_cmd_interpreter(). > So it's not enough if we just insert SCTP_ERROR_INV_STRM command after > sctp_eat_data() return SCTP_IERROR_BAD_STREAM in sctp_sf_eat_data_6_2(). Yes, I just tried this way, SACK is still bundled after ERROR chunk. But I think my above method is also not fine if there are multiple error DATA chunks bundled in a packet. Really awesome! Thanks, Xufeng Zhang > > > > Thanks, > Xufeng Zhang > >> >> -vlad >> >>> >>> Thanks, >>> Xufeng Zhang Thanks, Xufeng Zhang > > -vlad >> >> Thanks, >> Xufeng Zhang >>> >>> -vlad > > > -- > Sent from my Android phone with SkitMail. Please excuse my brevity. > >> >> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
Xufeng Zhang wrote: >On 7/25/12, Vlad Yasevich wrote: >>> And after take a moment to look into the relative codes, I think we >>> can implement it >>> by below way: >>> 1). Add a flag(isi_err_needed) in the embedded struct peer of struct >>> struct sctp_association >>> just like sack_needed flag. >>> 2). When "invalid stream identifier" ERROR happens in >sctp_eat_data() >>> function, we just >>> set isi_err_needed flag and don't create ERROR chunk and also don't >>> insert SCTP_CMD_REPLY command. >>> 3). In sctp_gen_sack() function, we create ERROR chunk and also >insert >>> SCTP_CMD_REPLY command if isi_err_needed flag is set. >>> >>> Is this way proper? >>> >> >> So, I looked at the code, and it looks very simple to do. We already >> return a specific status from sctp_eat_data() when the error was >> generated. All you have to do is take the code that generates the >error >> and adds it to the command list and give it its own small function >that >> you can then call if SCTP_IERROR_BAD_STREAM error was returned. > >No, it will still has the same problem by just doing this. >SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, >sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK >command >in sctp_cmd_interpreter(). >So it's not enough if we just insert SCTP_ERROR_INV_STRM command after >sctp_eat_data() return SCTP_IERROR_BAD_STREAM in >sctp_sf_eat_data_6_2(). > > All you have to do is change the order of side effect commands and the above is a guide. Ill prototype it tomorrow when I have time. -vlad > >Thanks, >Xufeng Zhang > >> >> -vlad >> >>> >>> Thanks, >>> Xufeng Zhang Thanks, Xufeng Zhang > > -vlad >> >> Thanks, >> Xufeng Zhang >>> >>> -vlad > > > -- > Sent from my Android phone with SkitMail. Please excuse my >brevity. > >> >> >> -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/25/12, Neil Horman wrote: > On Wed, Jul 25, 2012 at 05:22:19PM +0800, Xufeng Zhang wrote: >> On 7/25/12, Xufeng Zhang wrote: >> > On 7/25/12, Vlad Yasevich wrote: >> >> >> >> Actually not true. AUTH can be before SACK. So can any addip chunks >> >> that >> >> aid in locating an association. >> >> >> >> Now AUTH isn't a big issue since its autogenerated to the packet but >> >> ADDIP >> >> is since it could be queued up for retransmission. >> >> >> >> There could be other extensions as well. It really needs to be done >> >> either >> >> through side effects or making error chunks go at the end of other >> >> control >> >> chunks. Need to audit the spec to see if that's ok. >> > You are right, I just found SHUTDOWN chunks are also before SACK based >> > on >> > your commit "[SCTP]: Fix SACK sequence during shutdown". >> > Maybe the only solution is to do some work on side effects just as you >> > said. >> > Thanks for your explanation! >> >> And after take a moment to look into the relative codes, I think we >> can implement it >> by below way: >> 1). Add a flag(isi_err_needed) in the embedded struct peer of struct >> struct sctp_association >> just like sack_needed flag. >> 2). When "invalid stream identifier" ERROR happens in sctp_eat_data() >> function, we just >> set isi_err_needed flag and don't create ERROR chunk and also don't >> insert SCTP_CMD_REPLY command. >> 3). In sctp_gen_sack() function, we create ERROR chunk and also insert >> SCTP_CMD_REPLY command if isi_err_needed flag is set. >> >> Is this way proper? >> > That would probably work yes. Another way might just be to do some > re-ordering > in sctp_outq_flush. Before processing the control chunk list, scan it, > and: > 1) move all error chunks to the head of the list > 2) move all sack chunks to the head of the list > 3) move all shutdown chunks to the head of the list > > You can do that in a single iteration of the list if you use a few on-stack > lists and list_splice Thank you very much for your suggestion! I'll compare it with side effects modification and make a decision. Thanks, Xufeng Zhang > > Neil > >> >> Thanks, >> Xufeng Zhang >> > >> > >> > >> > Thanks, >> > Xufeng Zhang >> >> >> >> -vlad >> >>> >> >>>Thanks, >> >>>Xufeng Zhang >> >> -vlad >> >> >> >> >> >> -- >> >> Sent from my Android phone with SkitMail. Please excuse my brevity. >> >> >> > >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/25/12, Vlad Yasevich wrote: >> And after take a moment to look into the relative codes, I think we >> can implement it >> by below way: >> 1). Add a flag(isi_err_needed) in the embedded struct peer of struct >> struct sctp_association >> just like sack_needed flag. >> 2). When "invalid stream identifier" ERROR happens in sctp_eat_data() >> function, we just >> set isi_err_needed flag and don't create ERROR chunk and also don't >> insert SCTP_CMD_REPLY command. >> 3). In sctp_gen_sack() function, we create ERROR chunk and also insert >> SCTP_CMD_REPLY command if isi_err_needed flag is set. >> >> Is this way proper? >> > > So, I looked at the code, and it looks very simple to do. We already > return a specific status from sctp_eat_data() when the error was > generated. All you have to do is take the code that generates the error > and adds it to the command list and give it its own small function that > you can then call if SCTP_IERROR_BAD_STREAM error was returned. No, it will still has the same problem by just doing this. SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK command in sctp_cmd_interpreter(). So it's not enough if we just insert SCTP_ERROR_INV_STRM command after sctp_eat_data() return SCTP_IERROR_BAD_STREAM in sctp_sf_eat_data_6_2(). Thanks, Xufeng Zhang > > -vlad > >> >> Thanks, >> Xufeng Zhang >>> >>> >>> >>> Thanks, >>> Xufeng Zhang -vlad > > Thanks, > Xufeng Zhang >> >> -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. >>> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 07/25/2012 05:22 AM, Xufeng Zhang wrote: On 7/25/12, Xufeng Zhang wrote: On 7/25/12, Vlad Yasevich wrote: Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. You are right, I just found SHUTDOWN chunks are also before SACK based on your commit "[SCTP]: Fix SACK sequence during shutdown". Maybe the only solution is to do some work on side effects just as you said. Thanks for your explanation! And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When "invalid stream identifier" ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? So, I looked at the code, and it looks very simple to do. We already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the error and adds it to the command list and give it its own small function that you can then call if SCTP_IERROR_BAD_STREAM error was returned. -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On Wed, Jul 25, 2012 at 05:22:19PM +0800, Xufeng Zhang wrote: > On 7/25/12, Xufeng Zhang wrote: > > On 7/25/12, Vlad Yasevich wrote: > >> > >> Actually not true. AUTH can be before SACK. So can any addip chunks > >> that > >> aid in locating an association. > >> > >> Now AUTH isn't a big issue since its autogenerated to the packet but > >> ADDIP > >> is since it could be queued up for retransmission. > >> > >> There could be other extensions as well. It really needs to be done > >> either > >> through side effects or making error chunks go at the end of other > >> control > >> chunks. Need to audit the spec to see if that's ok. > > You are right, I just found SHUTDOWN chunks are also before SACK based on > > your commit "[SCTP]: Fix SACK sequence during shutdown". > > Maybe the only solution is to do some work on side effects just as you > > said. > > Thanks for your explanation! > > And after take a moment to look into the relative codes, I think we > can implement it > by below way: > 1). Add a flag(isi_err_needed) in the embedded struct peer of struct > struct sctp_association > just like sack_needed flag. > 2). When "invalid stream identifier" ERROR happens in sctp_eat_data() > function, we just > set isi_err_needed flag and don't create ERROR chunk and also don't > insert SCTP_CMD_REPLY command. > 3). In sctp_gen_sack() function, we create ERROR chunk and also insert > SCTP_CMD_REPLY command if isi_err_needed flag is set. > > Is this way proper? > That would probably work yes. Another way might just be to do some re-ordering in sctp_outq_flush. Before processing the control chunk list, scan it, and: 1) move all error chunks to the head of the list 2) move all sack chunks to the head of the list 3) move all shutdown chunks to the head of the list You can do that in a single iteration of the list if you use a few on-stack lists and list_splice Neil > > Thanks, > Xufeng Zhang > > > > > > > > Thanks, > > Xufeng Zhang > >> > >> -vlad > >>> > >>>Thanks, > >>>Xufeng Zhang > > -vlad > >> > >> > >> -- > >> Sent from my Android phone with SkitMail. Please excuse my brevity. > >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On Wed, Jul 25, 2012 at 10:34:32AM +0800, Xufeng Zhang wrote: > On 7/24/12, Neil Horman wrote: > > On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: > >> On 07/23/2012 08:14 PM, Neil Horman wrote: > >> >On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: > >> >>On 07/23/2012 08:49 AM, Neil Horman wrote: > >> >>>Not sure I understand how you came into this error. If we get an > >> >>> invalid > >> >>>stream, we issue an SCTP_REPORT_TSN side effect, followed by an > >> >>> SCTP_CMD_REPLY > >> >>>which sends the error chunk. The reply goes through > >> >>>sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. > >> >>>That last function checks to see if a sack is already part of the > >> >>> packet, and if > >> >>>there isn't one, appends one, using the updated tsn map. > >> >>Yes, you are right, but consider the invalid stream identifier's > >> >>DATA chunk is the first > >> >>DATA chunk in the association which will need SACK immediately. > >> >>Here is what I thought of the scenario: > >> >> sctp_sf_eat_data_6_2() > >> >> -->sctp_eat_data() > >> >> -->sctp_make_op_error() > >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > >> >> SCTP_CHUNK(err)) > >> >> -->sctp_outq_tail() /* First enqueue ERROR chunk > >> >> */ > >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) > >> >> -->sctp_gen_sack() > >> >> -->sctp_make_sack() > >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > >> >>SCTP_CHUNK(sack)) > >> >> -->sctp_outq_tail() /* Then enqueue SACK chunk > >> >> */ > >> >> > >> >>So SACK chunk is enqueued after ERROR chunk. > >> >Ah, I see. Since the ERROR and SACK chunks are both control chunks, and > >> > since > >> >we explicitly add the SACK to the control queue instead of going through > >> > the > >> >bundle path in sctp_packet_append_chunk the ordering gets wrong. > >> > > >> >Ok, so the problem makes sense. I think the soultion could be alot > >> > easier > >> >though. IIRC SACK chunks always live at the head of a packet, so why not > >> > just > >> >special case it in sctp_outq_tail? I.e. instead of doing a > >> > list_add_tail, in > >> >the else clause of sctp_outq_tail check the chunk_hdr->type to see if > >> > its > >> >SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I > >> > think > >> >that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? > >> > And then > >> >you won't have keep track of extra state in the packet configuration. > >> Yes, it's a good idea, but I think the premise is not correct: > >> RFC 4960 page 57: > >> "D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply > >>with a COOKIE ACK chunk after building a TCB and moving to the > >>ESTABLISHED state. A COOKIE ACK chunk may be bundled with any > >>pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk > >>MUST be the first chunk in the packet*." > >> > >> So we can't put SACK chunk always at the head of the packet. > >> > > Ok, Fair point, but that just changes the ordering a bit to: > > COOKIE_ACK > > SACK > > OTHER CONTROL CHUNKS > > > > What about something like this? Its completely untested, and I'm sure it > > can be > > cleaned up a bunch, but this keeps us from having to add additional state to > > the > > packet structure. > Yeah! I like this modification, thank you very much for your work! > I'll try to send a V2 patch based on your changes and run some tests. > Awesome, thank you! Neil -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/25/12, Xufeng Zhang wrote: > On 7/25/12, Vlad Yasevich wrote: >> >> Actually not true. AUTH can be before SACK. So can any addip chunks >> that >> aid in locating an association. >> >> Now AUTH isn't a big issue since its autogenerated to the packet but >> ADDIP >> is since it could be queued up for retransmission. >> >> There could be other extensions as well. It really needs to be done >> either >> through side effects or making error chunks go at the end of other >> control >> chunks. Need to audit the spec to see if that's ok. > You are right, I just found SHUTDOWN chunks are also before SACK based on > your commit "[SCTP]: Fix SACK sequence during shutdown". > Maybe the only solution is to do some work on side effects just as you > said. > Thanks for your explanation! And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When "invalid stream identifier" ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? Thanks, Xufeng Zhang > > > > Thanks, > Xufeng Zhang >> >> -vlad >>> >>>Thanks, >>>Xufeng Zhang -vlad >> >> >> -- >> Sent from my Android phone with SkitMail. Please excuse my brevity. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/25/12, Vlad Yasevich wrote: > > Actually not true. AUTH can be before SACK. So can any addip chunks that > aid in locating an association. > > Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP > is since it could be queued up for retransmission. > > There could be other extensions as well. It really needs to be done either > through side effects or making error chunks go at the end of other control > chunks. Need to audit the spec to see if that's ok. You are right, I just found SHUTDOWN chunks are also before SACK based on your commit "[SCTP]: Fix SACK sequence during shutdown". Maybe the only solution is to do some work on side effects just as you said. Thanks for your explanation! Thanks, Xufeng Zhang > > -vlad >> >>Thanks, >>Xufeng Zhang >>> >>> -vlad > > > -- > Sent from my Android phone with SkitMail. Please excuse my brevity. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
Xufeng Zhang wrote: >On 7/24/12, Vlad Yasevich wrote: > And I should clarify the above judgment code. > AFAIK, there should be two cases for the bundling when invalid >>>stream > identifier error happens: > 1). COOKIE_ACK ERROR SACK > 2). ERROR SACK > So I need to deal with the two cases differently. > > Sorry but I just don't buy that the above are the only 2 cases. >What >>>if there are addip chunks as well? What if there are some other >>>extensions also. This code has to be generic enough to handle any >>>condition. >>>Aha, you are right, this may happens. >>>So I think the general solution is to fix this problem in the enqueue >>>side. >>>What do you think? any better suggestion! >>> >> >> Don't have code in front of me but what if we carry the error >condition to >> where we queue the Sack and add the error side effect then? >Yes, this is the most direct way to fix this problem. >But I don't think it's the best way since we will take care of a lot >of things and >it also involves in lots of changes to side effect processing. >I prefer to Neil Horman's way for the solution since only COOKIE_ACK >chunk is >allowed to place ahead of SACK chunk when bundling into one packet. >What do you think? > > Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. -vlad > >Thanks, >Xufeng Zhang >> >> -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? So, I looked at the code, and it looks very simple to do. We already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the error and adds it to the command list and give it its own small function that you can then call if SCTP_IERROR_BAD_STREAM error was returned. No, it will still has the same problem by just doing this. SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK command in sctp_cmd_interpreter(). So it's not enough if we just insert SCTP_ERROR_INV_STRM command after sctp_eat_data() return SCTP_IERROR_BAD_STREAM in sctp_sf_eat_data_6_2(). Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/25/12, Neil Horman nhor...@tuxdriver.com wrote: On Wed, Jul 25, 2012 at 05:22:19PM +0800, Xufeng Zhang wrote: On 7/25/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. You are right, I just found SHUTDOWN chunks are also before SACK based on your commit [SCTP]: Fix SACK sequence during shutdown. Maybe the only solution is to do some work on side effects just as you said. Thanks for your explanation! And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? That would probably work yes. Another way might just be to do some re-ordering in sctp_outq_flush. Before processing the control chunk list, scan it, and: 1) move all error chunks to the head of the list 2) move all sack chunks to the head of the list 3) move all shutdown chunks to the head of the list You can do that in a single iteration of the list if you use a few on-stack lists and list_splice Thank you very much for your suggestion! I'll compare it with side effects modification and make a decision. Thanks, Xufeng Zhang Neil Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? So, I looked at the code, and it looks very simple to do. We already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the error and adds it to the command list and give it its own small function that you can then call if SCTP_IERROR_BAD_STREAM error was returned. No, it will still has the same problem by just doing this. SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK command in sctp_cmd_interpreter(). So it's not enough if we just insert SCTP_ERROR_INV_STRM command after sctp_eat_data() return SCTP_IERROR_BAD_STREAM in sctp_sf_eat_data_6_2(). All you have to do is change the order of side effect commands and the above is a guide. Ill prototype it tomorrow when I have time. -vlad Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/26/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? So, I looked at the code, and it looks very simple to do. We already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the error and adds it to the command list and give it its own small function that you can then call if SCTP_IERROR_BAD_STREAM error was returned. No, it will still has the same problem by just doing this. SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK command in sctp_cmd_interpreter(). So it's not enough if we just insert SCTP_ERROR_INV_STRM command after sctp_eat_data() return SCTP_IERROR_BAD_STREAM in sctp_sf_eat_data_6_2(). Yes, I just tried this way, SACK is still bundled after ERROR chunk. But I think my above method is also not fine if there are multiple error DATA chunks bundled in a packet. Really awesome! Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/26/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? So, I looked at the code, and it looks very simple to do. We already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the error and adds it to the command list and give it its own small function that you can then call if SCTP_IERROR_BAD_STREAM error was returned. No, it will still has the same problem by just doing this. SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK command in sctp_cmd_interpreter(). So it's not enough if we just insert SCTP_ERROR_INV_STRM command after sctp_eat_data() return SCTP_IERROR_BAD_STREAM in sctp_sf_eat_data_6_2(). Yes, I just tried this way, SACK is still bundled after ERROR chunk. But I think my above method is also not fine if there are multiple error DATA chunks bundled in a packet. Really awesome! 1. Catch the error return. 2. Set flag indicating error is needed. 3. Queue sack as needed. 4. If error flag set call new function to queue error chunk. That should fix things. Do this in all callers of sctp_eat_data. -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/26/12, Vlad Yasevich vyasev...@gmail.com wrote: Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/26/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? So, I looked at the code, and it looks very simple to do. We already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the error and adds it to the command list and give it its own small function that you can then call if SCTP_IERROR_BAD_STREAM error was returned. No, it will still has the same problem by just doing this. SCTP_CMD_GEN_SACK command actually don't enqueue SACK to outqueue, sctp_gen_sack() do this things when processing SCTP_CMD_GEN_SACK command in sctp_cmd_interpreter(). So it's not enough if we just insert SCTP_ERROR_INV_STRM command after sctp_eat_data() return SCTP_IERROR_BAD_STREAM in sctp_sf_eat_data_6_2(). Yes, I just tried this way, SACK is still bundled after ERROR chunk. But I think my above method is also not fine if there are multiple error DATA chunks bundled in a packet. Really awesome! 1. Catch the error return. 2. Set flag indicating error is needed. 3. Queue sack as needed. 4. If error flag set call new function to queue error chunk. Both step 3 and 4 need lots of changes to make it working since SACK is ready to queue only at the end of the packet and we also need to deal with multiple invalid stream identifier ERROR chunks in a single packet. Thanks, Xufeng Zhang That should fix things. Do this in all callers of sctp_eat_data. -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/24/12, Vlad Yasevich vyasev...@gmail.com wrote: And I should clarify the above judgment code. AFAIK, there should be two cases for the bundling when invalid stream identifier error happens: 1). COOKIE_ACK ERROR SACK 2). ERROR SACK So I need to deal with the two cases differently. Sorry but I just don't buy that the above are the only 2 cases. What if there are addip chunks as well? What if there are some other extensions also. This code has to be generic enough to handle any condition. Aha, you are right, this may happens. So I think the general solution is to fix this problem in the enqueue side. What do you think? any better suggestion! Don't have code in front of me but what if we carry the error condition to where we queue the Sack and add the error side effect then? Yes, this is the most direct way to fix this problem. But I don't think it's the best way since we will take care of a lot of things and it also involves in lots of changes to side effect processing. I prefer to Neil Horman's way for the solution since only COOKIE_ACK chunk is allowed to place ahead of SACK chunk when bundling into one packet. What do you think? Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. You are right, I just found SHUTDOWN chunks are also before SACK based on your commit [SCTP]: Fix SACK sequence during shutdown. Maybe the only solution is to do some work on side effects just as you said. Thanks for your explanation! Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/25/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. You are right, I just found SHUTDOWN chunks are also before SACK based on your commit [SCTP]: Fix SACK sequence during shutdown. Maybe the only solution is to do some work on side effects just as you said. Thanks for your explanation! And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On Wed, Jul 25, 2012 at 10:34:32AM +0800, Xufeng Zhang wrote: On 7/24/12, Neil Horman nhor...@tuxdriver.com wrote: On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: On 07/23/2012 08:14 PM, Neil Horman wrote: On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: On 07/23/2012 08:49 AM, Neil Horman wrote: Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail-sctp_outq_chunk-sctp_outq_transmit_chunk-sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. Yes, you are right, but consider the invalid stream identifier's DATA chunk is the first DATA chunk in the association which will need SACK immediately. Here is what I thought of the scenario: sctp_sf_eat_data_6_2() --sctp_eat_data() --sctp_make_op_error() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) --sctp_outq_tail() /* First enqueue ERROR chunk */ --sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) --sctp_gen_sack() --sctp_make_sack() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(sack)) --sctp_outq_tail() /* Then enqueue SACK chunk */ So SACK chunk is enqueued after ERROR chunk. Ah, I see. Since the ERROR and SACK chunks are both control chunks, and since we explicitly add the SACK to the control queue instead of going through the bundle path in sctp_packet_append_chunk the ordering gets wrong. Ok, so the problem makes sense. I think the soultion could be alot easier though. IIRC SACK chunks always live at the head of a packet, so why not just special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, in the else clause of sctp_outq_tail check the chunk_hdr-type to see if its SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I think that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And then you won't have keep track of extra state in the packet configuration. Yes, it's a good idea, but I think the premise is not correct: RFC 4960 page 57: D) Upon reception of the COOKIE ECHO chunk, endpoint Z will reply with a COOKIE ACK chunk after building a TCB and moving to the ESTABLISHED state. A COOKIE ACK chunk may be bundled with any pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk MUST be the first chunk in the packet*. So we can't put SACK chunk always at the head of the packet. Ok, Fair point, but that just changes the ordering a bit to: COOKIE_ACK SACK OTHER CONTROL CHUNKS What about something like this? Its completely untested, and I'm sure it can be cleaned up a bunch, but this keeps us from having to add additional state to the packet structure. Yeah! I like this modification, thank you very much for your work! I'll try to send a V2 patch based on your changes and run some tests. Awesome, thank you! Neil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On Wed, Jul 25, 2012 at 05:22:19PM +0800, Xufeng Zhang wrote: On 7/25/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. You are right, I just found SHUTDOWN chunks are also before SACK based on your commit [SCTP]: Fix SACK sequence during shutdown. Maybe the only solution is to do some work on side effects just as you said. Thanks for your explanation! And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? That would probably work yes. Another way might just be to do some re-ordering in sctp_outq_flush. Before processing the control chunk list, scan it, and: 1) move all error chunks to the head of the list 2) move all sack chunks to the head of the list 3) move all shutdown chunks to the head of the list You can do that in a single iteration of the list if you use a few on-stack lists and list_splice Neil Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 07/25/2012 05:22 AM, Xufeng Zhang wrote: On 7/25/12, Xufeng Zhang xufengzhang.m...@gmail.com wrote: On 7/25/12, Vlad Yasevich vyasev...@gmail.com wrote: Actually not true. AUTH can be before SACK. So can any addip chunks that aid in locating an association. Now AUTH isn't a big issue since its autogenerated to the packet but ADDIP is since it could be queued up for retransmission. There could be other extensions as well. It really needs to be done either through side effects or making error chunks go at the end of other control chunks. Need to audit the spec to see if that's ok. You are right, I just found SHUTDOWN chunks are also before SACK based on your commit [SCTP]: Fix SACK sequence during shutdown. Maybe the only solution is to do some work on side effects just as you said. Thanks for your explanation! And after take a moment to look into the relative codes, I think we can implement it by below way: 1). Add a flag(isi_err_needed) in the embedded struct peer of struct struct sctp_association just like sack_needed flag. 2). When invalid stream identifier ERROR happens in sctp_eat_data() function, we just set isi_err_needed flag and don't create ERROR chunk and also don't insert SCTP_CMD_REPLY command. 3). In sctp_gen_sack() function, we create ERROR chunk and also insert SCTP_CMD_REPLY command if isi_err_needed flag is set. Is this way proper? So, I looked at the code, and it looks very simple to do. We already return a specific status from sctp_eat_data() when the error was generated. All you have to do is take the code that generates the error and adds it to the command list and give it its own small function that you can then call if SCTP_IERROR_BAD_STREAM error was returned. -vlad Thanks, Xufeng Zhang Thanks, Xufeng Zhang -vlad Thanks, Xufeng Zhang -vlad -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/24/12, Neil Horman wrote: > On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: >> On 07/23/2012 08:14 PM, Neil Horman wrote: >> >On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: >> >>On 07/23/2012 08:49 AM, Neil Horman wrote: >> >>>Not sure I understand how you came into this error. If we get an >> >>> invalid >> >>>stream, we issue an SCTP_REPORT_TSN side effect, followed by an >> >>> SCTP_CMD_REPLY >> >>>which sends the error chunk. The reply goes through >> >>>sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. >> >>>That last function checks to see if a sack is already part of the >> >>> packet, and if >> >>>there isn't one, appends one, using the updated tsn map. >> >>Yes, you are right, but consider the invalid stream identifier's >> >>DATA chunk is the first >> >>DATA chunk in the association which will need SACK immediately. >> >>Here is what I thought of the scenario: >> >> sctp_sf_eat_data_6_2() >> >> -->sctp_eat_data() >> >> -->sctp_make_op_error() >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, >> >> SCTP_CHUNK(err)) >> >> -->sctp_outq_tail() /* First enqueue ERROR chunk >> >> */ >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) >> >> -->sctp_gen_sack() >> >> -->sctp_make_sack() >> >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, >> >>SCTP_CHUNK(sack)) >> >> -->sctp_outq_tail() /* Then enqueue SACK chunk >> >> */ >> >> >> >>So SACK chunk is enqueued after ERROR chunk. >> >Ah, I see. Since the ERROR and SACK chunks are both control chunks, and >> > since >> >we explicitly add the SACK to the control queue instead of going through >> > the >> >bundle path in sctp_packet_append_chunk the ordering gets wrong. >> > >> >Ok, so the problem makes sense. I think the soultion could be alot >> > easier >> >though. IIRC SACK chunks always live at the head of a packet, so why not >> > just >> >special case it in sctp_outq_tail? I.e. instead of doing a >> > list_add_tail, in >> >the else clause of sctp_outq_tail check the chunk_hdr->type to see if >> > its >> >SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I >> > think >> >that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? >> > And then >> >you won't have keep track of extra state in the packet configuration. >> Yes, it's a good idea, but I think the premise is not correct: >> RFC 4960 page 57: >> "D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply >>with a COOKIE ACK chunk after building a TCB and moving to the >>ESTABLISHED state. A COOKIE ACK chunk may be bundled with any >>pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk >>MUST be the first chunk in the packet*." >> >> So we can't put SACK chunk always at the head of the packet. >> > Ok, Fair point, but that just changes the ordering a bit to: > COOKIE_ACK > SACK > OTHER CONTROL CHUNKS > > What about something like this? Its completely untested, and I'm sure it > can be > cleaned up a bunch, but this keeps us from having to add additional state to > the > packet structure. Yeah! I like this modification, thank you very much for your work! I'll try to send a V2 patch based on your changes and run some tests. Thanks, Xufeng Zhang > > > diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c > index e7aa177c..eeac32f 100644 > --- a/net/sctp/outqueue.c > +++ b/net/sctp/outqueue.c > @@ -300,7 +300,7 @@ void sctp_outq_free(struct sctp_outq *q) > int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) > { > int error = 0; > - > + struct sctp_chunk *cptr; > SCTP_DEBUG_PRINTK("sctp_outq_tail(%p, %p[%s])\n", > q, chunk, chunk && chunk->chunk_hdr ? > sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) > @@ -344,7 +344,21 @@ int sctp_outq_tail(struct sctp_outq *q, struct > sctp_chunk *chunk) > break; > } > } else { > - list_add_tail(>list, >control_chunk_list); > + list_del_init(>list); > + if (chunk->chunk_hdr->type == SCTP_CID_COOKIE_ACK) > + list_add_head(>list, >control_chunk_list); > + else if (!list_empty(>control_chunk_list) && > + chunk->chunk_hdr->type == SCTP_CID_SACK) { > + list_for_each_entry(cptr, >control_chunk_list, list) > { > + if (cptr->chunk_hdr->type == > SCTP_CID_COOKIE_ACK) > + continue; > + list_add(>list, >list); > + break; > + } > + } > + > + if (list_empty(>list)) > + list_add_tail(>list, >control_chunk_list); > SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); > } >
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/24/12, Vlad Yasevich wrote: And I should clarify the above judgment code. AFAIK, there should be two cases for the bundling when invalid >>stream identifier error happens: 1). COOKIE_ACK ERROR SACK 2). ERROR SACK So I need to deal with the two cases differently. >>> Sorry but I just don't buy that the above are the only 2 cases. What >>if there are addip chunks as well? What if there are some other >>extensions also. This code has to be generic enough to handle any >>condition. >>> >>Aha, you are right, this may happens. >>So I think the general solution is to fix this problem in the enqueue >>side. >>What do you think? any better suggestion! >> > > Don't have code in front of me but what if we carry the error condition to > where we queue the Sack and add the error side effect then? Yes, this is the most direct way to fix this problem. But I don't think it's the best way since we will take care of a lot of things and it also involves in lots of changes to side effect processing. I prefer to Neil Horman's way for the solution since only COOKIE_ACK chunk is allowed to place ahead of SACK chunk when bundling into one packet. What do you think? Thanks, Xufeng Zhang > > -vlad -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
xufeng zhang wrote: >On 07/24/2012 10:27 AM, Vlad Yasevich wrote: >> xufeng zhang wrote: >> >> >>> On 07/19/2012 01:57 PM, xufengzhang.m...@gmail.com wrote: >>> When "Invalid Stream Identifier" ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling "Invalid Stream >Identifier" ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter >processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the "Invalid Stream Identifier" ERROR happens. Signed-off-by: Xufeng Zhang --- include/net/sctp/structs.h |2 ++ net/sctp/output.c | 26 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/include/net/sctp/structs.h >b/include/net/sctp/structs.h index 88949a9..5adf4de 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -842,6 +842,8 @@ struct sctp_packet { has_sack:1, /* This packet contains a SACK chunk. */ has_auth:1, /* This packet contains an AUTH chunk */ has_data:1, /* This packet contains at least 1 DATA chunk >*/ + has_isi_err:1, /* This packet contains a "Invalid Stream + * Identifier" ERROR chunk */ ipfragok:1, /* So let ip fragment this packet */ malloced:1; /* Is it malloced? */ }; diff --git a/net/sctp/output.c b/net/sctp/output.c index 817174e..77fb1ae 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet >>> *packet) >>> packet->has_sack = 0; packet->has_data = 0; packet->has_auth = 0; + packet->has_isi_err = 0; packet->ipfragok = 0; packet->auth = NULL; } @@ -267,6 +268,7 @@ static sctp_xmit_t >sctp_packet_bundle_sack(struct >>> sctp_packet *pkt, >>> sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, struct sctp_chunk *chunk) { + struct sctp_chunk *lchunk; sctp_xmit_t retval = SCTP_XMIT_OK; __u16 chunk_len = WORD_ROUND(ntohs(chunk->chunk_hdr->length)); @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct >>> sctp_packet *packet, >>> packet->has_cookie_echo = 1; break; + case SCTP_CID_ERROR: + if (chunk->subh.err_hdr->cause& SCTP_ERROR_INV_STRM) + packet->has_isi_err = 1; + break; + case SCTP_CID_SACK: + /* RFC 4960 + * 6.5 Stream Identifier and Stream Sequence Number + * The endpoint may bundle the ERROR chunk in the same + * packet as the SACK as long as the ERROR follows the SACK. + */ + if (packet->has_isi_err) { + if (list_is_singular(>chunk_list)) + list_add(>list,>chunk_list); + else { + lchunk = list_first_entry(>chunk_list, + struct sctp_chunk, list); + list_add(>list,>list); + } >>> And I should clarify the above judgment code. >>> AFAIK, there should be two cases for the bundling when invalid >stream >>> identifier error happens: >>> 1). COOKIE_ACK ERROR SACK >>> 2). ERROR SACK >>> So I need to deal with the two cases differently. >>> >>> >> Sorry but I just don't buy that the above are the only 2 cases. What >if there are addip chunks as well? What if there are some other >extensions also. This code has to be generic enough to handle any >condition. >> >Aha, you are right, this may happens. >So I think the general solution is to fix this problem in the enqueue >side. >What do you think? any better suggestion! > Don't have code in front of me but what if we carry the error
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: > On 07/23/2012 08:14 PM, Neil Horman wrote: > >On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: > >>On 07/23/2012 08:49 AM, Neil Horman wrote: > >>>Not sure I understand how you came into this error. If we get an invalid > >>>stream, we issue an SCTP_REPORT_TSN side effect, followed by an > >>>SCTP_CMD_REPLY > >>>which sends the error chunk. The reply goes through > >>>sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. > >>>That last function checks to see if a sack is already part of the packet, > >>>and if > >>>there isn't one, appends one, using the updated tsn map. > >>Yes, you are right, but consider the invalid stream identifier's > >>DATA chunk is the first > >>DATA chunk in the association which will need SACK immediately. > >>Here is what I thought of the scenario: > >> sctp_sf_eat_data_6_2() > >> -->sctp_eat_data() > >> -->sctp_make_op_error() > >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) > >> -->sctp_outq_tail() /* First enqueue ERROR chunk */ > >> -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) > >> -->sctp_gen_sack() > >> -->sctp_make_sack() > >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > >>SCTP_CHUNK(sack)) > >> -->sctp_outq_tail() /* Then enqueue SACK chunk */ > >> > >>So SACK chunk is enqueued after ERROR chunk. > >Ah, I see. Since the ERROR and SACK chunks are both control chunks, and > >since > >we explicitly add the SACK to the control queue instead of going through the > >bundle path in sctp_packet_append_chunk the ordering gets wrong. > > > >Ok, so the problem makes sense. I think the soultion could be alot easier > >though. IIRC SACK chunks always live at the head of a packet, so why not > >just > >special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, in > >the else clause of sctp_outq_tail check the chunk_hdr->type to see if its > >SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I > >think > >that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And > >then > >you won't have keep track of extra state in the packet configuration. > Yes, it's a good idea, but I think the premise is not correct: > RFC 4960 page 57: > "D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply >with a COOKIE ACK chunk after building a TCB and moving to the >ESTABLISHED state. A COOKIE ACK chunk may be bundled with any >pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk >MUST be the first chunk in the packet*." > > So we can't put SACK chunk always at the head of the packet. > Ok, Fair point, but that just changes the ordering a bit to: COOKIE_ACK SACK OTHER CONTROL CHUNKS What about something like this? Its completely untested, and I'm sure it can be cleaned up a bunch, but this keeps us from having to add additional state to the packet structure. diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177c..eeac32f 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -300,7 +300,7 @@ void sctp_outq_free(struct sctp_outq *q) int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) { int error = 0; - + struct sctp_chunk *cptr; SCTP_DEBUG_PRINTK("sctp_outq_tail(%p, %p[%s])\n", q, chunk, chunk && chunk->chunk_hdr ? sctp_cname(SCTP_ST_CHUNK(chunk->chunk_hdr->type)) @@ -344,7 +344,21 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) break; } } else { - list_add_tail(>list, >control_chunk_list); + list_del_init(>list); + if (chunk->chunk_hdr->type == SCTP_CID_COOKIE_ACK) + list_add_head(>list, >control_chunk_list); + else if (!list_empty(>control_chunk_list) && + chunk->chunk_hdr->type == SCTP_CID_SACK) { + list_for_each_entry(cptr, >control_chunk_list, list) { + if (cptr->chunk_hdr->type == SCTP_CID_COOKIE_ACK) + continue; + list_add(>list, >list); + break; + } + } + + if (list_empty(>list)) + list_add_tail(>list, >control_chunk_list); SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); } > > Thanks, > Xufeng Zhang > >Regards > >Neil > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: On 07/23/2012 08:14 PM, Neil Horman wrote: On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: On 07/23/2012 08:49 AM, Neil Horman wrote: Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail-sctp_outq_chunk-sctp_outq_transmit_chunk-sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. Yes, you are right, but consider the invalid stream identifier's DATA chunk is the first DATA chunk in the association which will need SACK immediately. Here is what I thought of the scenario: sctp_sf_eat_data_6_2() --sctp_eat_data() --sctp_make_op_error() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) --sctp_outq_tail() /* First enqueue ERROR chunk */ --sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) --sctp_gen_sack() --sctp_make_sack() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(sack)) --sctp_outq_tail() /* Then enqueue SACK chunk */ So SACK chunk is enqueued after ERROR chunk. Ah, I see. Since the ERROR and SACK chunks are both control chunks, and since we explicitly add the SACK to the control queue instead of going through the bundle path in sctp_packet_append_chunk the ordering gets wrong. Ok, so the problem makes sense. I think the soultion could be alot easier though. IIRC SACK chunks always live at the head of a packet, so why not just special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, in the else clause of sctp_outq_tail check the chunk_hdr-type to see if its SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I think that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And then you won't have keep track of extra state in the packet configuration. Yes, it's a good idea, but I think the premise is not correct: RFC 4960 page 57: D) Upon reception of the COOKIE ECHO chunk, endpoint Z will reply with a COOKIE ACK chunk after building a TCB and moving to the ESTABLISHED state. A COOKIE ACK chunk may be bundled with any pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk MUST be the first chunk in the packet*. So we can't put SACK chunk always at the head of the packet. Ok, Fair point, but that just changes the ordering a bit to: COOKIE_ACK SACK OTHER CONTROL CHUNKS What about something like this? Its completely untested, and I'm sure it can be cleaned up a bunch, but this keeps us from having to add additional state to the packet structure. diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177c..eeac32f 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -300,7 +300,7 @@ void sctp_outq_free(struct sctp_outq *q) int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) { int error = 0; - + struct sctp_chunk *cptr; SCTP_DEBUG_PRINTK(sctp_outq_tail(%p, %p[%s])\n, q, chunk, chunk chunk-chunk_hdr ? sctp_cname(SCTP_ST_CHUNK(chunk-chunk_hdr-type)) @@ -344,7 +344,21 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) break; } } else { - list_add_tail(chunk-list, q-control_chunk_list); + list_del_init(chunk-list); + if (chunk-chunk_hdr-type == SCTP_CID_COOKIE_ACK) + list_add_head(chunk-list, q-control_chunk_list); + else if (!list_empty(q-control_chunk_list) + chunk-chunk_hdr-type == SCTP_CID_SACK) { + list_for_each_entry(cptr, q-control_chunk_list, list) { + if (cptr-chunk_hdr-type == SCTP_CID_COOKIE_ACK) + continue; + list_add(chunk-list, cptr-list); + break; + } + } + + if (list_empty(chunk-list)) + list_add_tail(chunk-list, q-control_chunk_list); SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); } Thanks, Xufeng Zhang Regards Neil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
xufeng zhang xufeng.zh...@windriver.com wrote: On 07/24/2012 10:27 AM, Vlad Yasevich wrote: xufeng zhangxufeng.zh...@windriver.com wrote: On 07/19/2012 01:57 PM, xufengzhang.m...@gmail.com wrote: When Invalid Stream Identifier ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling Invalid Stream Identifier ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the Invalid Stream Identifier ERROR happens. Signed-off-by: Xufeng Zhangxufeng.zh...@windriver.com --- include/net/sctp/structs.h |2 ++ net/sctp/output.c | 26 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 88949a9..5adf4de 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -842,6 +842,8 @@ struct sctp_packet { has_sack:1, /* This packet contains a SACK chunk. */ has_auth:1, /* This packet contains an AUTH chunk */ has_data:1, /* This packet contains at least 1 DATA chunk */ + has_isi_err:1, /* This packet contains a Invalid Stream + * Identifier ERROR chunk */ ipfragok:1, /* So let ip fragment this packet */ malloced:1; /* Is it malloced? */ }; diff --git a/net/sctp/output.c b/net/sctp/output.c index 817174e..77fb1ae 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet *packet) packet-has_sack = 0; packet-has_data = 0; packet-has_auth = 0; + packet-has_isi_err = 0; packet-ipfragok = 0; packet-auth = NULL; } @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt, sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, struct sctp_chunk *chunk) { + struct sctp_chunk *lchunk; sctp_xmit_t retval = SCTP_XMIT_OK; __u16 chunk_len = WORD_ROUND(ntohs(chunk-chunk_hdr-length)); @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, packet-has_cookie_echo = 1; break; + case SCTP_CID_ERROR: + if (chunk-subh.err_hdr-cause SCTP_ERROR_INV_STRM) + packet-has_isi_err = 1; + break; + case SCTP_CID_SACK: + /* RFC 4960 + * 6.5 Stream Identifier and Stream Sequence Number + * The endpoint may bundle the ERROR chunk in the same + * packet as the SACK as long as the ERROR follows the SACK. + */ + if (packet-has_isi_err) { + if (list_is_singular(packet-chunk_list)) + list_add(chunk-list,packet-chunk_list); + else { + lchunk = list_first_entry(packet-chunk_list, + struct sctp_chunk, list); + list_add(chunk-list,lchunk-list); + } And I should clarify the above judgment code. AFAIK, there should be two cases for the bundling when invalid stream identifier error happens: 1). COOKIE_ACK ERROR SACK 2). ERROR SACK So I need to deal with the two cases differently. Sorry but I just don't buy that the above are the only 2 cases. What if there are addip chunks as well? What if there are some other extensions also. This code has to be generic enough to handle any condition. Aha, you are right, this may happens. So I think the general solution is to fix this problem in the enqueue side. What do you think? any better suggestion! Don't have code in front of me but what if we carry the error condition to where we queue the Sack and add the error side effect then? -vlad Thanks, Xufeng Zhang - vlad Thanks, Xufeng Zhang + packet-size += chunk_len; + chunk-transport = packet-transport; + packet-has_sack = 1; + goto finish; +
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/24/12, Vlad Yasevich vyasev...@gmail.com wrote: And I should clarify the above judgment code. AFAIK, there should be two cases for the bundling when invalid stream identifier error happens: 1). COOKIE_ACK ERROR SACK 2). ERROR SACK So I need to deal with the two cases differently. Sorry but I just don't buy that the above are the only 2 cases. What if there are addip chunks as well? What if there are some other extensions also. This code has to be generic enough to handle any condition. Aha, you are right, this may happens. So I think the general solution is to fix this problem in the enqueue side. What do you think? any better suggestion! Don't have code in front of me but what if we carry the error condition to where we queue the Sack and add the error side effect then? Yes, this is the most direct way to fix this problem. But I don't think it's the best way since we will take care of a lot of things and it also involves in lots of changes to side effect processing. I prefer to Neil Horman's way for the solution since only COOKIE_ACK chunk is allowed to place ahead of SACK chunk when bundling into one packet. What do you think? Thanks, Xufeng Zhang -vlad -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/24/12, Neil Horman nhor...@tuxdriver.com wrote: On Tue, Jul 24, 2012 at 09:50:18AM +0800, xufeng zhang wrote: On 07/23/2012 08:14 PM, Neil Horman wrote: On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: On 07/23/2012 08:49 AM, Neil Horman wrote: Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail-sctp_outq_chunk-sctp_outq_transmit_chunk-sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. Yes, you are right, but consider the invalid stream identifier's DATA chunk is the first DATA chunk in the association which will need SACK immediately. Here is what I thought of the scenario: sctp_sf_eat_data_6_2() --sctp_eat_data() --sctp_make_op_error() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) --sctp_outq_tail() /* First enqueue ERROR chunk */ --sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) --sctp_gen_sack() --sctp_make_sack() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(sack)) --sctp_outq_tail() /* Then enqueue SACK chunk */ So SACK chunk is enqueued after ERROR chunk. Ah, I see. Since the ERROR and SACK chunks are both control chunks, and since we explicitly add the SACK to the control queue instead of going through the bundle path in sctp_packet_append_chunk the ordering gets wrong. Ok, so the problem makes sense. I think the soultion could be alot easier though. IIRC SACK chunks always live at the head of a packet, so why not just special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, in the else clause of sctp_outq_tail check the chunk_hdr-type to see if its SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I think that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And then you won't have keep track of extra state in the packet configuration. Yes, it's a good idea, but I think the premise is not correct: RFC 4960 page 57: D) Upon reception of the COOKIE ECHO chunk, endpoint Z will reply with a COOKIE ACK chunk after building a TCB and moving to the ESTABLISHED state. A COOKIE ACK chunk may be bundled with any pending DATA chunks (and/or SACK chunks), *but the COOKIE ACK chunk MUST be the first chunk in the packet*. So we can't put SACK chunk always at the head of the packet. Ok, Fair point, but that just changes the ordering a bit to: COOKIE_ACK SACK OTHER CONTROL CHUNKS What about something like this? Its completely untested, and I'm sure it can be cleaned up a bunch, but this keeps us from having to add additional state to the packet structure. Yeah! I like this modification, thank you very much for your work! I'll try to send a V2 patch based on your changes and run some tests. Thanks, Xufeng Zhang diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index e7aa177c..eeac32f 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -300,7 +300,7 @@ void sctp_outq_free(struct sctp_outq *q) int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) { int error = 0; - + struct sctp_chunk *cptr; SCTP_DEBUG_PRINTK(sctp_outq_tail(%p, %p[%s])\n, q, chunk, chunk chunk-chunk_hdr ? sctp_cname(SCTP_ST_CHUNK(chunk-chunk_hdr-type)) @@ -344,7 +344,21 @@ int sctp_outq_tail(struct sctp_outq *q, struct sctp_chunk *chunk) break; } } else { - list_add_tail(chunk-list, q-control_chunk_list); + list_del_init(chunk-list); + if (chunk-chunk_hdr-type == SCTP_CID_COOKIE_ACK) + list_add_head(chunk-list, q-control_chunk_list); + else if (!list_empty(q-control_chunk_list) + chunk-chunk_hdr-type == SCTP_CID_SACK) { + list_for_each_entry(cptr, q-control_chunk_list, list) { + if (cptr-chunk_hdr-type == SCTP_CID_COOKIE_ACK) + continue; + list_add(chunk-list, cptr-list); + break; + } + } + + if (list_empty(chunk-list)) + list_add_tail(chunk-list, q-control_chunk_list); SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS); } Thanks, Xufeng Zhang Regards Neil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 07/24/2012 10:27 AM, Vlad Yasevich wrote: xufeng zhang wrote: On 07/19/2012 01:57 PM, xufengzhang.m...@gmail.com wrote: When "Invalid Stream Identifier" ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling "Invalid Stream Identifier" ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the "Invalid Stream Identifier" ERROR happens. Signed-off-by: Xufeng Zhang --- include/net/sctp/structs.h |2 ++ net/sctp/output.c | 26 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 88949a9..5adf4de 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -842,6 +842,8 @@ struct sctp_packet { has_sack:1, /* This packet contains a SACK chunk. */ has_auth:1, /* This packet contains an AUTH chunk */ has_data:1, /* This packet contains at least 1 DATA chunk */ + has_isi_err:1, /* This packet contains a "Invalid Stream +* Identifier" ERROR chunk */ ipfragok:1, /* So let ip fragment this packet */ malloced:1; /* Is it malloced? */ }; diff --git a/net/sctp/output.c b/net/sctp/output.c index 817174e..77fb1ae 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet *packet) packet->has_sack = 0; packet->has_data = 0; packet->has_auth = 0; + packet->has_isi_err = 0; packet->ipfragok = 0; packet->auth = NULL; } @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt, sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, struct sctp_chunk *chunk) { + struct sctp_chunk *lchunk; sctp_xmit_t retval = SCTP_XMIT_OK; __u16 chunk_len = WORD_ROUND(ntohs(chunk->chunk_hdr->length)); @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, packet->has_cookie_echo = 1; break; + case SCTP_CID_ERROR: + if (chunk->subh.err_hdr->cause& SCTP_ERROR_INV_STRM) + packet->has_isi_err = 1; + break; + case SCTP_CID_SACK: + /* RFC 4960 +* 6.5 Stream Identifier and Stream Sequence Number +* The endpoint may bundle the ERROR chunk in the same +* packet as the SACK as long as the ERROR follows the SACK. +*/ + if (packet->has_isi_err) { + if (list_is_singular(>chunk_list)) + list_add(>list,>chunk_list); + else { + lchunk = list_first_entry(>chunk_list, + struct sctp_chunk, list); + list_add(>list,>list); + } And I should clarify the above judgment code. AFAIK, there should be two cases for the bundling when invalid stream identifier error happens: 1). COOKIE_ACK ERROR SACK 2). ERROR SACK So I need to deal with the two cases differently. Sorry but I just don't buy that the above are the only 2 cases. What if there are addip chunks as well? What if there are some other extensions also. This code has to be generic enough to handle any condition. Aha, you are right, this may happens. So I think the general solution is to fix this problem in the enqueue side. What do you think? any better suggestion! Thanks, Xufeng Zhang - vlad Thanks, Xufeng Zhang + packet->size += chunk_len; + chunk->transport = packet->transport; + packet->has_sack = 1; + goto finish; + } + packet->has_sack = 1; break; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
xufeng zhang wrote: >On 07/19/2012 01:57 PM, xufengzhang.m...@gmail.com wrote: >> When "Invalid Stream Identifier" ERROR happens after process the >> received DATA chunks, this ERROR chunk is enqueued into outqueue >> before SACK chunk, so when bundling ERROR chunk with SACK chunk, >> the ERROR chunk is always placed first in the packet because of >> the chunk's position in the outqueue. >> This violates sctp specification: >> RFC 4960 6.5. Stream Identifier and Stream Sequence Number >> ...The endpoint may bundle the ERROR chunk in the same >> packet as the SACK as long as the ERROR follows the SACK. >> So we must place SACK first when bundling "Invalid Stream Identifier" >> ERROR and SACK in one packet. >> Although we can do that by enqueue SACK chunk into outqueue before >> ERROR chunk, it will violate the side-effect interpreter processing. >> It's easy to do this job when dequeue chunks from the outqueue, >> by this way, we introduce a flag 'has_isi_err' which indicate >> whether or not the "Invalid Stream Identifier" ERROR happens. >> >> Signed-off-by: Xufeng Zhang >> --- >> include/net/sctp/structs.h |2 ++ >> net/sctp/output.c | 26 ++ >> 2 files changed, 28 insertions(+), 0 deletions(-) >> >> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h >> index 88949a9..5adf4de 100644 >> --- a/include/net/sctp/structs.h >> +++ b/include/net/sctp/structs.h >> @@ -842,6 +842,8 @@ struct sctp_packet { >> has_sack:1, /* This packet contains a SACK chunk. */ >> has_auth:1, /* This packet contains an AUTH chunk */ >> has_data:1, /* This packet contains at least 1 DATA chunk */ >> +has_isi_err:1, /* This packet contains a "Invalid Stream >> + * Identifier" ERROR chunk */ >> ipfragok:1, /* So let ip fragment this packet */ >> malloced:1; /* Is it malloced? */ >> }; >> diff --git a/net/sctp/output.c b/net/sctp/output.c >> index 817174e..77fb1ae 100644 >> --- a/net/sctp/output.c >> +++ b/net/sctp/output.c >> @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet >*packet) >> packet->has_sack = 0; >> packet->has_data = 0; >> packet->has_auth = 0; >> +packet->has_isi_err = 0; >> packet->ipfragok = 0; >> packet->auth = NULL; >> } >> @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct >sctp_packet *pkt, >> sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, >> struct sctp_chunk *chunk) >> { >> +struct sctp_chunk *lchunk; >> sctp_xmit_t retval = SCTP_XMIT_OK; >> __u16 chunk_len = WORD_ROUND(ntohs(chunk->chunk_hdr->length)); >> >> @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct >sctp_packet *packet, >> packet->has_cookie_echo = 1; >> break; >> >> +case SCTP_CID_ERROR: >> +if (chunk->subh.err_hdr->cause& SCTP_ERROR_INV_STRM) >> +packet->has_isi_err = 1; >> +break; >> + >> case SCTP_CID_SACK: >> +/* RFC 4960 >> + * 6.5 Stream Identifier and Stream Sequence Number >> + * The endpoint may bundle the ERROR chunk in the same >> + * packet as the SACK as long as the ERROR follows the SACK. >> + */ >> +if (packet->has_isi_err) { >> +if (list_is_singular(>chunk_list)) >> +list_add(>list,>chunk_list); >> +else { >> +lchunk = list_first_entry(>chunk_list, >> +struct sctp_chunk, list); >> +list_add(>list,>list); >> +} >> >And I should clarify the above judgment code. >AFAIK, there should be two cases for the bundling when invalid stream >identifier error happens: >1). COOKIE_ACK ERROR SACK >2). ERROR SACK >So I need to deal with the two cases differently. > Sorry but I just don't buy that the above are the only 2 cases. What if there are addip chunks as well? What if there are some other extensions also. This code has to be generic enough to handle any condition. - vlad > >Thanks, >Xufeng Zhang >> +packet->size += chunk_len; >> +chunk->transport = packet->transport; >> +packet->has_sack = 1; >> +goto finish; >> +} >> + >> packet->has_sack = 1; >> break; >> >> -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 7/23/12, Neil Horman wrote: > On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: >> On 07/23/2012 08:49 AM, Neil Horman wrote: >> > >> >Not sure I understand how you came into this error. If we get an >> > invalid >> >stream, we issue an SCTP_REPORT_TSN side effect, followed by an >> > SCTP_CMD_REPLY >> >which sends the error chunk. The reply goes through >> >sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. >> >That last function checks to see if a sack is already part of the packet, >> > and if >> >there isn't one, appends one, using the updated tsn map. >> Yes, you are right, but consider the invalid stream identifier's >> DATA chunk is the first >> DATA chunk in the association which will need SACK immediately. >> Here is what I thought of the scenario: >> sctp_sf_eat_data_6_2() >> -->sctp_eat_data() >> -->sctp_make_op_error() >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) >> -->sctp_outq_tail() /* First enqueue ERROR chunk */ >> -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) >> -->sctp_gen_sack() >> -->sctp_make_sack() >> -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, >> SCTP_CHUNK(sack)) >> -->sctp_outq_tail() /* Then enqueue SACK chunk >> */ >> >> So SACK chunk is enqueued after ERROR chunk. > Ah, I see. Since the ERROR and SACK chunks are both control chunks, and > since > we explicitly add the SACK to the control queue instead of going through > the > bundle path in sctp_packet_append_chunk the ordering gets wrong. > > Ok, so the problem makes sense. I think the soultion could be alot easier > though. IIRC SACK chunks always live at the head of a packet, so why not > just > special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, > in > the else clause of sctp_outq_tail check the chunk_hdr->type to see if its > SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I > think > that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And > then > you won't have keep track of extra state in the packet configuration. (Please ignore the duplicate messages if you received, sorry for this!) Yes, it's a good idea, but I think the premise is not correct: RFC 4960 page 57: "D) Upon reception of the COOKIE ECHO chunk, endpoint "Z" will reply with a COOKIE ACK chunk after building a TCB and moving to the ESTABLISHED state. A COOKIE ACK chunk may be bundled with any pending DATA chunks (and/or SACK chunks), but the COOKIE ACK chunk MUST be the first chunk in the packet." So we can't put SACK chunk always at the head of the packet. Thanks, Xufeng Zhang > > Regards > Neil > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: > On 07/23/2012 08:49 AM, Neil Horman wrote: > > > >Not sure I understand how you came into this error. If we get an invalid > >stream, we issue an SCTP_REPORT_TSN side effect, followed by an > >SCTP_CMD_REPLY > >which sends the error chunk. The reply goes through > >sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. > >That last function checks to see if a sack is already part of the packet, > >and if > >there isn't one, appends one, using the updated tsn map. > Yes, you are right, but consider the invalid stream identifier's > DATA chunk is the first > DATA chunk in the association which will need SACK immediately. > Here is what I thought of the scenario: > sctp_sf_eat_data_6_2() > -->sctp_eat_data() > -->sctp_make_op_error() > -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) > -->sctp_outq_tail() /* First enqueue ERROR chunk */ > -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) > -->sctp_gen_sack() > -->sctp_make_sack() > -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, > SCTP_CHUNK(sack)) > -->sctp_outq_tail() /* Then enqueue SACK chunk */ > > So SACK chunk is enqueued after ERROR chunk. Ah, I see. Since the ERROR and SACK chunks are both control chunks, and since we explicitly add the SACK to the control queue instead of going through the bundle path in sctp_packet_append_chunk the ordering gets wrong. Ok, so the problem makes sense. I think the soultion could be alot easier though. IIRC SACK chunks always live at the head of a packet, so why not just special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, in the else clause of sctp_outq_tail check the chunk_hdr->type to see if its SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I think that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And then you won't have keep track of extra state in the packet configuration. Regards Neil -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: On 07/23/2012 08:49 AM, Neil Horman wrote: Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail-sctp_outq_chunk-sctp_outq_transmit_chunk-sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. Yes, you are right, but consider the invalid stream identifier's DATA chunk is the first DATA chunk in the association which will need SACK immediately. Here is what I thought of the scenario: sctp_sf_eat_data_6_2() --sctp_eat_data() --sctp_make_op_error() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) --sctp_outq_tail() /* First enqueue ERROR chunk */ --sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) --sctp_gen_sack() --sctp_make_sack() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(sack)) --sctp_outq_tail() /* Then enqueue SACK chunk */ So SACK chunk is enqueued after ERROR chunk. Ah, I see. Since the ERROR and SACK chunks are both control chunks, and since we explicitly add the SACK to the control queue instead of going through the bundle path in sctp_packet_append_chunk the ordering gets wrong. Ok, so the problem makes sense. I think the soultion could be alot easier though. IIRC SACK chunks always live at the head of a packet, so why not just special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, in the else clause of sctp_outq_tail check the chunk_hdr-type to see if its SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I think that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And then you won't have keep track of extra state in the packet configuration. Regards Neil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 7/23/12, Neil Horman nhor...@tuxdriver.com wrote: On Mon, Jul 23, 2012 at 10:30:34AM +0800, xufeng zhang wrote: On 07/23/2012 08:49 AM, Neil Horman wrote: Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail-sctp_outq_chunk-sctp_outq_transmit_chunk-sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. Yes, you are right, but consider the invalid stream identifier's DATA chunk is the first DATA chunk in the association which will need SACK immediately. Here is what I thought of the scenario: sctp_sf_eat_data_6_2() --sctp_eat_data() --sctp_make_op_error() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) --sctp_outq_tail() /* First enqueue ERROR chunk */ --sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) --sctp_gen_sack() --sctp_make_sack() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(sack)) --sctp_outq_tail() /* Then enqueue SACK chunk */ So SACK chunk is enqueued after ERROR chunk. Ah, I see. Since the ERROR and SACK chunks are both control chunks, and since we explicitly add the SACK to the control queue instead of going through the bundle path in sctp_packet_append_chunk the ordering gets wrong. Ok, so the problem makes sense. I think the soultion could be alot easier though. IIRC SACK chunks always live at the head of a packet, so why not just special case it in sctp_outq_tail? I.e. instead of doing a list_add_tail, in the else clause of sctp_outq_tail check the chunk_hdr-type to see if its SCTP_CID_SACK. If it is, use list_add_head rather than list_add_tail. I think that will fix up both the COOKIE_ECHO and ESTABLISHED cases, won't it? And then you won't have keep track of extra state in the packet configuration. (Please ignore the duplicate messages if you received, sorry for this!) Yes, it's a good idea, but I think the premise is not correct: RFC 4960 page 57: D) Upon reception of the COOKIE ECHO chunk, endpoint Z will reply with a COOKIE ACK chunk after building a TCB and moving to the ESTABLISHED state. A COOKIE ACK chunk may be bundled with any pending DATA chunks (and/or SACK chunks), but the COOKIE ACK chunk MUST be the first chunk in the packet. So we can't put SACK chunk always at the head of the packet. Thanks, Xufeng Zhang Regards Neil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
xufeng zhang xufeng.zh...@windriver.com wrote: On 07/19/2012 01:57 PM, xufengzhang.m...@gmail.com wrote: When Invalid Stream Identifier ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling Invalid Stream Identifier ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the Invalid Stream Identifier ERROR happens. Signed-off-by: Xufeng Zhangxufeng.zh...@windriver.com --- include/net/sctp/structs.h |2 ++ net/sctp/output.c | 26 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 88949a9..5adf4de 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -842,6 +842,8 @@ struct sctp_packet { has_sack:1, /* This packet contains a SACK chunk. */ has_auth:1, /* This packet contains an AUTH chunk */ has_data:1, /* This packet contains at least 1 DATA chunk */ +has_isi_err:1, /* This packet contains a Invalid Stream + * Identifier ERROR chunk */ ipfragok:1, /* So let ip fragment this packet */ malloced:1; /* Is it malloced? */ }; diff --git a/net/sctp/output.c b/net/sctp/output.c index 817174e..77fb1ae 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet *packet) packet-has_sack = 0; packet-has_data = 0; packet-has_auth = 0; +packet-has_isi_err = 0; packet-ipfragok = 0; packet-auth = NULL; } @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt, sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, struct sctp_chunk *chunk) { +struct sctp_chunk *lchunk; sctp_xmit_t retval = SCTP_XMIT_OK; __u16 chunk_len = WORD_ROUND(ntohs(chunk-chunk_hdr-length)); @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, packet-has_cookie_echo = 1; break; +case SCTP_CID_ERROR: +if (chunk-subh.err_hdr-cause SCTP_ERROR_INV_STRM) +packet-has_isi_err = 1; +break; + case SCTP_CID_SACK: +/* RFC 4960 + * 6.5 Stream Identifier and Stream Sequence Number + * The endpoint may bundle the ERROR chunk in the same + * packet as the SACK as long as the ERROR follows the SACK. + */ +if (packet-has_isi_err) { +if (list_is_singular(packet-chunk_list)) +list_add(chunk-list,packet-chunk_list); +else { +lchunk = list_first_entry(packet-chunk_list, +struct sctp_chunk, list); +list_add(chunk-list,lchunk-list); +} And I should clarify the above judgment code. AFAIK, there should be two cases for the bundling when invalid stream identifier error happens: 1). COOKIE_ACK ERROR SACK 2). ERROR SACK So I need to deal with the two cases differently. Sorry but I just don't buy that the above are the only 2 cases. What if there are addip chunks as well? What if there are some other extensions also. This code has to be generic enough to handle any condition. - vlad Thanks, Xufeng Zhang +packet-size += chunk_len; +chunk-transport = packet-transport; +packet-has_sack = 1; +goto finish; +} + packet-has_sack = 1; break; -- Sent from my Android phone with SkitMail. Please excuse my brevity. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 07/24/2012 10:27 AM, Vlad Yasevich wrote: xufeng zhangxufeng.zh...@windriver.com wrote: On 07/19/2012 01:57 PM, xufengzhang.m...@gmail.com wrote: When Invalid Stream Identifier ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling Invalid Stream Identifier ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the Invalid Stream Identifier ERROR happens. Signed-off-by: Xufeng Zhangxufeng.zh...@windriver.com --- include/net/sctp/structs.h |2 ++ net/sctp/output.c | 26 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 88949a9..5adf4de 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -842,6 +842,8 @@ struct sctp_packet { has_sack:1, /* This packet contains a SACK chunk. */ has_auth:1, /* This packet contains an AUTH chunk */ has_data:1, /* This packet contains at least 1 DATA chunk */ + has_isi_err:1, /* This packet contains a Invalid Stream +* Identifier ERROR chunk */ ipfragok:1, /* So let ip fragment this packet */ malloced:1; /* Is it malloced? */ }; diff --git a/net/sctp/output.c b/net/sctp/output.c index 817174e..77fb1ae 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet *packet) packet-has_sack = 0; packet-has_data = 0; packet-has_auth = 0; + packet-has_isi_err = 0; packet-ipfragok = 0; packet-auth = NULL; } @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt, sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, struct sctp_chunk *chunk) { + struct sctp_chunk *lchunk; sctp_xmit_t retval = SCTP_XMIT_OK; __u16 chunk_len = WORD_ROUND(ntohs(chunk-chunk_hdr-length)); @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, packet-has_cookie_echo = 1; break; + case SCTP_CID_ERROR: + if (chunk-subh.err_hdr-cause SCTP_ERROR_INV_STRM) + packet-has_isi_err = 1; + break; + case SCTP_CID_SACK: + /* RFC 4960 +* 6.5 Stream Identifier and Stream Sequence Number +* The endpoint may bundle the ERROR chunk in the same +* packet as the SACK as long as the ERROR follows the SACK. +*/ + if (packet-has_isi_err) { + if (list_is_singular(packet-chunk_list)) + list_add(chunk-list,packet-chunk_list); + else { + lchunk = list_first_entry(packet-chunk_list, + struct sctp_chunk, list); + list_add(chunk-list,lchunk-list); + } And I should clarify the above judgment code. AFAIK, there should be two cases for the bundling when invalid stream identifier error happens: 1). COOKIE_ACK ERROR SACK 2). ERROR SACK So I need to deal with the two cases differently. Sorry but I just don't buy that the above are the only 2 cases. What if there are addip chunks as well? What if there are some other extensions also. This code has to be generic enough to handle any condition. Aha, you are right, this may happens. So I think the general solution is to fix this problem in the enqueue side. What do you think? any better suggestion! Thanks, Xufeng Zhang - vlad Thanks, Xufeng Zhang + packet-size += chunk_len; + chunk-transport = packet-transport; + packet-has_sack = 1; + goto finish; + } + packet-has_sack = 1; break; -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On 07/23/2012 08:49 AM, Neil Horman wrote: Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. Yes, you are right, but consider the invalid stream identifier's DATA chunk is the first DATA chunk in the association which will need SACK immediately. Here is what I thought of the scenario: sctp_sf_eat_data_6_2() -->sctp_eat_data() -->sctp_make_op_error() -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) -->sctp_outq_tail() /* First enqueue ERROR chunk */ -->sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) -->sctp_gen_sack() -->sctp_make_sack() -->sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(sack)) -->sctp_outq_tail() /* Then enqueue SACK chunk */ So SACK chunk is enqueued after ERROR chunk. So Can you explain in some more detail how you're getting into this situation? Actually it's triggered by a customer's test case, but we can also reproduce this problem easily by explicitly contaminating the sctp stack: --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -701,7 +701,7 @@ struct sctp_chunk *sctp_make_datafrag_empty(struct sctp_association *asoc, * creating the chunk. */ dp.tsn = 0; - dp.stream = htons(sinfo->sinfo_stream); + dp.stream = htons(sinfo->sinfo_stream) + 10; dp.ppid = sinfo->sinfo_ppid; /* Set the flags for an unordered send. */ Then run sctp_darn application and capture the sctp packet at the same time. Thanks, Xufeng Zhang Thanks! Neil -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
On Thu, Jul 19, 2012 at 01:57:30PM +0800, xufengzhang.m...@gmail.com wrote: > When "Invalid Stream Identifier" ERROR happens after process the > received DATA chunks, this ERROR chunk is enqueued into outqueue > before SACK chunk, so when bundling ERROR chunk with SACK chunk, > the ERROR chunk is always placed first in the packet because of > the chunk's position in the outqueue. > This violates sctp specification: > RFC 4960 6.5. Stream Identifier and Stream Sequence Number > ...The endpoint may bundle the ERROR chunk in the same > packet as the SACK as long as the ERROR follows the SACK. > So we must place SACK first when bundling "Invalid Stream Identifier" > ERROR and SACK in one packet. > Although we can do that by enqueue SACK chunk into outqueue before > ERROR chunk, it will violate the side-effect interpreter processing. > It's easy to do this job when dequeue chunks from the outqueue, > by this way, we introduce a flag 'has_isi_err' which indicate > whether or not the "Invalid Stream Identifier" ERROR happens. > > Signed-off-by: Xufeng Zhang Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail->sctp_outq_chunk->sctp_outq_transmit_chunk->sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. So Can you explain in some more detail how you're getting into this situation? Thanks! Neil -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
From: Date: Thu, 19 Jul 2012 13:57:30 +0800 > When "Invalid Stream Identifier" ERROR happens after process the > received DATA chunks, this ERROR chunk is enqueued into outqueue > before SACK chunk, so when bundling ERROR chunk with SACK chunk, > the ERROR chunk is always placed first in the packet because of > the chunk's position in the outqueue. > This violates sctp specification: > RFC 4960 6.5. Stream Identifier and Stream Sequence Number > ...The endpoint may bundle the ERROR chunk in the same > packet as the SACK as long as the ERROR follows the SACK. > So we must place SACK first when bundling "Invalid Stream Identifier" > ERROR and SACK in one packet. > Although we can do that by enqueue SACK chunk into outqueue before > ERROR chunk, it will violate the side-effect interpreter processing. > It's easy to do this job when dequeue chunks from the outqueue, > by this way, we introduce a flag 'has_isi_err' which indicate > whether or not the "Invalid Stream Identifier" ERROR happens. > > Signed-off-by: Xufeng Zhang Can some SCTP experts please review this? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
From: xufengzhang.m...@gmail.com Date: Thu, 19 Jul 2012 13:57:30 +0800 When Invalid Stream Identifier ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling Invalid Stream Identifier ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the Invalid Stream Identifier ERROR happens. Signed-off-by: Xufeng Zhang xufeng.zh...@windriver.com Can some SCTP experts please review this? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On Thu, Jul 19, 2012 at 01:57:30PM +0800, xufengzhang.m...@gmail.com wrote: When Invalid Stream Identifier ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling Invalid Stream Identifier ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the Invalid Stream Identifier ERROR happens. Signed-off-by: Xufeng Zhang xufeng.zh...@windriver.com Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail-sctp_outq_chunk-sctp_outq_transmit_chunk-sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. So Can you explain in some more detail how you're getting into this situation? Thanks! Neil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
On 07/23/2012 08:49 AM, Neil Horman wrote: Not sure I understand how you came into this error. If we get an invalid stream, we issue an SCTP_REPORT_TSN side effect, followed by an SCTP_CMD_REPLY which sends the error chunk. The reply goes through sctp_outq_tail-sctp_outq_chunk-sctp_outq_transmit_chunk-sctp_outq_append_chunk. That last function checks to see if a sack is already part of the packet, and if there isn't one, appends one, using the updated tsn map. Yes, you are right, but consider the invalid stream identifier's DATA chunk is the first DATA chunk in the association which will need SACK immediately. Here is what I thought of the scenario: sctp_sf_eat_data_6_2() --sctp_eat_data() --sctp_make_op_error() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(err)) --sctp_outq_tail() /* First enqueue ERROR chunk */ --sctp_add_cmd_sf(commands, SCTP_CMD_GEN_SACK, SCTP_FORCE()) --sctp_gen_sack() --sctp_make_sack() --sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(sack)) --sctp_outq_tail() /* Then enqueue SACK chunk */ So SACK chunk is enqueued after ERROR chunk. So Can you explain in some more detail how you're getting into this situation? Actually it's triggered by a customer's test case, but we can also reproduce this problem easily by explicitly contaminating the sctp stack: --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -701,7 +701,7 @@ struct sctp_chunk *sctp_make_datafrag_empty(struct sctp_association *asoc, * creating the chunk. */ dp.tsn = 0; - dp.stream = htons(sinfo-sinfo_stream); + dp.stream = htons(sinfo-sinfo_stream) + 10; dp.ppid = sinfo-sinfo_ppid; /* Set the flags for an unordered send. */ Then run sctp_darn application and capture the sctp packet at the same time. Thanks, Xufeng Zhang Thanks! Neil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
When "Invalid Stream Identifier" ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling "Invalid Stream Identifier" ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the "Invalid Stream Identifier" ERROR happens. Signed-off-by: Xufeng Zhang --- include/net/sctp/structs.h |2 ++ net/sctp/output.c | 26 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 88949a9..5adf4de 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -842,6 +842,8 @@ struct sctp_packet { has_sack:1, /* This packet contains a SACK chunk. */ has_auth:1, /* This packet contains an AUTH chunk */ has_data:1, /* This packet contains at least 1 DATA chunk */ + has_isi_err:1, /* This packet contains a "Invalid Stream +* Identifier" ERROR chunk */ ipfragok:1, /* So let ip fragment this packet */ malloced:1; /* Is it malloced? */ }; diff --git a/net/sctp/output.c b/net/sctp/output.c index 817174e..77fb1ae 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet *packet) packet->has_sack = 0; packet->has_data = 0; packet->has_auth = 0; + packet->has_isi_err = 0; packet->ipfragok = 0; packet->auth = NULL; } @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt, sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, struct sctp_chunk *chunk) { + struct sctp_chunk *lchunk; sctp_xmit_t retval = SCTP_XMIT_OK; __u16 chunk_len = WORD_ROUND(ntohs(chunk->chunk_hdr->length)); @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, packet->has_cookie_echo = 1; break; + case SCTP_CID_ERROR: + if (chunk->subh.err_hdr->cause & SCTP_ERROR_INV_STRM) + packet->has_isi_err = 1; + break; + case SCTP_CID_SACK: + /* RFC 4960 +* 6.5 Stream Identifier and Stream Sequence Number +* The endpoint may bundle the ERROR chunk in the same +* packet as the SACK as long as the ERROR follows the SACK. +*/ + if (packet->has_isi_err) { + if (list_is_singular(>chunk_list)) + list_add(>list, >chunk_list); + else { + lchunk = list_first_entry(>chunk_list, + struct sctp_chunk, list); + list_add(>list, >list); + } + packet->size += chunk_len; + chunk->transport = packet->transport; + packet->has_sack = 1; + goto finish; + } + packet->has_sack = 1; break; -- 1.7.0.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] sctp: Make Invalid Stream Identifier ERROR follows SACK when bundling
When Invalid Stream Identifier ERROR happens after process the received DATA chunks, this ERROR chunk is enqueued into outqueue before SACK chunk, so when bundling ERROR chunk with SACK chunk, the ERROR chunk is always placed first in the packet because of the chunk's position in the outqueue. This violates sctp specification: RFC 4960 6.5. Stream Identifier and Stream Sequence Number ...The endpoint may bundle the ERROR chunk in the same packet as the SACK as long as the ERROR follows the SACK. So we must place SACK first when bundling Invalid Stream Identifier ERROR and SACK in one packet. Although we can do that by enqueue SACK chunk into outqueue before ERROR chunk, it will violate the side-effect interpreter processing. It's easy to do this job when dequeue chunks from the outqueue, by this way, we introduce a flag 'has_isi_err' which indicate whether or not the Invalid Stream Identifier ERROR happens. Signed-off-by: Xufeng Zhang xufeng.zh...@windriver.com --- include/net/sctp/structs.h |2 ++ net/sctp/output.c | 26 ++ 2 files changed, 28 insertions(+), 0 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 88949a9..5adf4de 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -842,6 +842,8 @@ struct sctp_packet { has_sack:1, /* This packet contains a SACK chunk. */ has_auth:1, /* This packet contains an AUTH chunk */ has_data:1, /* This packet contains at least 1 DATA chunk */ + has_isi_err:1, /* This packet contains a Invalid Stream +* Identifier ERROR chunk */ ipfragok:1, /* So let ip fragment this packet */ malloced:1; /* Is it malloced? */ }; diff --git a/net/sctp/output.c b/net/sctp/output.c index 817174e..77fb1ae 100644 --- a/net/sctp/output.c +++ b/net/sctp/output.c @@ -79,6 +79,7 @@ static void sctp_packet_reset(struct sctp_packet *packet) packet-has_sack = 0; packet-has_data = 0; packet-has_auth = 0; + packet-has_isi_err = 0; packet-ipfragok = 0; packet-auth = NULL; } @@ -267,6 +268,7 @@ static sctp_xmit_t sctp_packet_bundle_sack(struct sctp_packet *pkt, sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, struct sctp_chunk *chunk) { + struct sctp_chunk *lchunk; sctp_xmit_t retval = SCTP_XMIT_OK; __u16 chunk_len = WORD_ROUND(ntohs(chunk-chunk_hdr-length)); @@ -316,7 +318,31 @@ sctp_xmit_t sctp_packet_append_chunk(struct sctp_packet *packet, packet-has_cookie_echo = 1; break; + case SCTP_CID_ERROR: + if (chunk-subh.err_hdr-cause SCTP_ERROR_INV_STRM) + packet-has_isi_err = 1; + break; + case SCTP_CID_SACK: + /* RFC 4960 +* 6.5 Stream Identifier and Stream Sequence Number +* The endpoint may bundle the ERROR chunk in the same +* packet as the SACK as long as the ERROR follows the SACK. +*/ + if (packet-has_isi_err) { + if (list_is_singular(packet-chunk_list)) + list_add(chunk-list, packet-chunk_list); + else { + lchunk = list_first_entry(packet-chunk_list, + struct sctp_chunk, list); + list_add(chunk-list, lchunk-list); + } + packet-size += chunk_len; + chunk-transport = packet-transport; + packet-has_sack = 1; + goto finish; + } + packet-has_sack = 1; break; -- 1.7.0.2 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/