[PATCH v2] sctp: Fix sk_ack_backlog wrap-around problem

[Xufeng Zhang]

Consider the scenario:
For a TCP-style socket, while processing the COOKIE_ECHO chunk in
sctp_sf_do_5_1D_ce(), after it has passed a series of sanity check,
a new association would be created in sctp_unpack_cookie(), but afterwards,
some processing maybe failed, and sctp_association_free() will be called to
free the previously allocated association, in sctp_association_free(),
sk_ack_backlog value is decremented for this socket, since the initial
value for sk_ack_backlog is 0, after the decrement, it will be 65535,
a wrap-around problem happens, and if we want to establish new associations
afterward in the same socket, ABORT would be triggered since sctp deem the
accept queue as full.
Fix this issue by only decrementing sk_ack_backlog for associations in
the endpoint's list.

Fix-suggested-by: Neil Horman 
Signed-off-by: Xufeng Zhang 
---
Change for v2:
  Drop the redundant test for temp suggested by Vlad Yasevich.

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

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 39579c3..0b8 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -330,7 +330,7 @@ void sctp_association_free(struct sctp_association *asoc)
/* Only real associations count against the endpoint, so
 * don't bother for if this is a temporary association.
 */
-   if (!asoc->temp) {
+   if (!list_empty(&asoc->asocs)) {
list_del(&asoc->asocs);
 
/* Decrement the backlog value for a TCP-style listening
-- 
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/

Re: [PATCH] sctp: Fix sk_ack_backlog wrap-around problem

[Xufeng Zhang]

On 06/12/2014 12:55 AM, Vlad Yasevich wrote:

On 06/11/2014 08:55 AM, Vlad Yasevich wrote:
   

On 06/10/2014 10:37 PM, Xufeng Zhang wrote:
 

Consider the scenario:
For a TCP-style socket, while processing the COOKIE_ECHO chunk in
sctp_sf_do_5_1D_ce(), after it has passed a series of sanity check,
a new association would be created in sctp_unpack_cookie(), but afterwards,
some processing maybe failed, and sctp_association_free() will be called to
free the previously allocated association, in sctp_association_free(),
sk_ack_backlog value is decremented for this socket, since the initial
value for sk_ack_backlog is 0, after the decrement, it will be 65535,
a wrap-around problem happens, and if we want to establish new associations
afterward in the same socket, ABORT would be triggered since sctp deem the
accept queue as full.
Fix this issue by only decrementing sk_ack_backlog for associations in
the endpoint's list.

Fix-suggested-by: Neil Horman
Signed-off-by: Xufeng Zhang
---
  net/sctp/associola.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 39579c3..60564f2 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -330,7 +330,7 @@ void sctp_association_free(struct sctp_association *asoc)
/* Only real associations count against the endpoint, so
 * don't bother for if this is a temporary association.
 */
-   if (!asoc->temp) {
+   if (!asoc->temp&&  !list_empty(&asoc->asocs)) {
list_del(&asoc->asocs);

/* Decrement the backlog value for a TCP-style listening

   

I am not crazy about this patch.   It's been suggested before that may
be duplicate cookie processing should really be creating a temporary
association since that's is how that association is being used.
 

I had another look at the description for triggering this issue and
realized that I was thinking about something else when looking at
this solution.

There is however no need to test both the list and temp value.
We can simply always test the that list is not empty before doing
list_del().
   


Thanks a lot for the comment!
I'll send V2 later.


Thanks,
Xufeng



-vlad

   

  It
might be nice at that approach.  It actually benefits us as the
association destruction would happen immediately instead of being delayed.
 


   


--
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: Fix sk_ack_backlog wrap-around problem

[Xufeng Zhang]

Consider the scenario:
For a TCP-style socket, while processing the COOKIE_ECHO chunk in
sctp_sf_do_5_1D_ce(), after it has passed a series of sanity check,
a new association would be created in sctp_unpack_cookie(), but afterwards,
some processing maybe failed, and sctp_association_free() will be called to
free the previously allocated association, in sctp_association_free(),
sk_ack_backlog value is decremented for this socket, since the initial
value for sk_ack_backlog is 0, after the decrement, it will be 65535,
a wrap-around problem happens, and if we want to establish new associations
afterward in the same socket, ABORT would be triggered since sctp deem the
accept queue as full.
Fix this issue by only decrementing sk_ack_backlog for associations in
the endpoint's list.

Fix-suggested-by: Neil Horman 
Signed-off-by: Xufeng Zhang 
---
 net/sctp/associola.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 39579c3..60564f2 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -330,7 +330,7 @@ void sctp_association_free(struct sctp_association *asoc)
/* Only real associations count against the endpoint, so
 * don't bother for if this is a temporary association.
 */
-   if (!asoc->temp) {
+   if (!asoc->temp && !list_empty(&asoc->asocs)) {
list_del(&asoc->asocs);
 
/* Decrement the backlog value for a TCP-style listening
-- 
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/

sctp bug: sk_ack_backlog wrap-around problem

[Xufeng Zhang]

Hi Vlad and Neil,

I found a sk_ack_backlog wrap-around problem during handling received
COOKIE_ECHO.

Consider the scenario:
For a TCP-style socket, while processing the COOKIE_ECHO chunk in
sctp_sf_do_5_1D_ce(),
after it has passed a series of sanity check, a new association would
be created in
sctp_unpack_cookie(), but afterwards, some processing maybe failed,
and sctp_association_free()
will be called to free the previously allocated association, in
sctp_association_free(),
sk_ack_backlog value is decremented for this socket, since the initial
value for sk_ack_backlog is 0,
after the decrement, it will be 65535, a wrap-around problem happens,
and if we want to establish
new associations afterward in the same socket, ABORT would be
triggered since sctp deem the
accept queue as full.

Do you have any idea to fix such kind of issue?

Thanks a lot!
Xufeng
--
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: [RFC][PATCH] af_key: return error when meet errors on sendmsg() syscall

[Xufeng Zhang]

On 05/12/2014 01:11 PM, David Miller wrote:



So it makes sense to return errors for send() syscall.

Signed-off-by: Xufeng Zhang
 

I disagree.

If pfkey_error() is successful, the error will be reported in the AF_KEY
message that is broadcast, there is no reason for sendmsg to return an
error.  The message was sucessfully sent, there was no problem with it's
passage into the AF_KEY layer.

Like netlink, operational responses come in packets, not error codes.

However, if pfkey_error() fails, we must do pass back the original
error code because it's a last ditch effort to prevent information
from being lost.

That's why 'err' must be preserved when pfkey_error() returns zero.
   


I know what you mean, but isn't the kernel API aimed to facilitate the
implementation of user space?
Since send the message to the kernel and receive the error report message
are asynchronous, I don't think it's easy to recover from the EINTR error
by parsing the error report message.


Thanks,
Xufeng




--
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/

[RFC][PATCH] af_key: return error when meet errors on sendmsg() syscall

[Xufeng Zhang]

Current implementation for pfkey_sendmsg() always return success
no matter whether or not error happens during this syscall,
this is incompatible with the general send()/sendmsg() API:
  man send
RETURN VALUE
  On success, these calls return the number of characters sent.
  On error, -1 is returned, and errno is set appropriately.

One side effect this problem introduces is that we can't determine
when to resend the message when the previous send() fails because
it was interrupted by signals.
We detect such a problem when racoon is sending SADBADD message to
add SAD entry in the kernel, but sometimes kernel is responding with
"Interrupted system call"(-EINTR) error.

Check the send implementation of strongswan, it has below logic:
  pfkey_send_socket()
  {
...
while (TRUE)
{
len = send(socket, in, in_len, 0);

if (len != in_len)
{
case EINTR:
/* interrupted, try again */
continue;
...
}
}
...
}
So it makes sense to return errors for send() syscall.  

Signed-off-by: Xufeng Zhang 
---
 net/key/af_key.c |6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index f3c8307..9e4bc8c 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -335,7 +335,7 @@ static int pfkey_error(const struct sadb_msg *orig, int 
err, struct sock *sk)
 
pfkey_broadcast(skb, GFP_KERNEL, BROADCAST_ONE, sk, sock_net(sk));
 
-   return 0;
+   return -err;
 }
 
 static const u8 sadb_ext_min_len[] = {
@@ -3644,8 +3644,8 @@ static int pfkey_sendmsg(struct kiocb *kiocb,
mutex_unlock(&net->xfrm.xfrm_cfg_mutex);
 
 out:
-   if (err && hdr && pfkey_error(hdr, err, sk) == 0)
-   err = 0;
+   if (err && hdr)
+   err = pfkey_error(hdr, err, sk);
kfree_skb(skb);
 
return err ? : len;
-- 
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 V2] sctp: reset flowi4_oif parameter on route lookup

[Xufeng Zhang]

commit 813b3b5db83 (ipv4: Use caller's on-stack flowi as-is
in output route lookups.) introduces another regression which
is very similar to the problem of commit e6b45241c (ipv4: reset
flowi parameters on route connect) wants to fix:
Before we call ip_route_output_key() in sctp_v4_get_dst() to
get a dst that matches a bind address as the source address,
we have already called this function previously and the flowi
parameters have been initialized including flowi4_oif, so when
we call this function again, the process in __ip_route_output_key()
will be different because of the setting of flowi4_oif, and we'll
get a networking device which corresponds to the inputted flowi4_oif
as the output device, this is wrong because we'll never hit this
place if the previously returned source address of dst match one
of the bound addresses.

To reproduce this problem, a vlan setting is enough:
  # ifconfig eth0 up
  # route del default
  # vconfig add eth0 2
  # vconfig add eth0 3
  # ifconfig eth0.2 10.0.1.14 netmask 255.255.255.0
  # route add default gw 10.0.1.254 dev eth0.2
  # ifconfig eth0.3 10.0.0.14 netmask 255.255.255.0
  # ip rule add from 10.0.0.14 table 4
  # ip route add table 4 default via 10.0.0.254 src 10.0.0.14 dev eth0.3
  # sctp_darn -H 10.0.0.14 -P 36422 -h 10.1.4.134 -p 36422 -s -I
You'll detect that all the flow are routed to eth0.2(10.0.1.254).

Signed-off-by: Xufeng Zhang 
Signed-off-by: Julian Anastasov 
---
Changelog:

v1->v2:
 Suggested by Julian Anastasov, reset flowi parameters by 
flowi4_update_output().

 net/sctp/protocol.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index c09757f..44cbb54 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -491,8 +491,13 @@ static void sctp_v4_get_dst(struct sctp_transport *t, 
union sctp_addr *saddr,
continue;
if ((laddr->state == SCTP_ADDR_SRC) &&
(AF_INET == laddr->a.sa.sa_family)) {
-   fl4->saddr = laddr->a.v4.sin_addr.s_addr;
fl4->fl4_sport = laddr->a.v4.sin_port;
+   flowi4_update_output(fl4,
+asoc->base.sk->sk_bound_dev_if,
+RT_CONN_FLAGS(asoc->base.sk),
+daddr->v4.sin_addr.s_addr,
+laddr->a.v4.sin_addr.s_addr);
+
rt = ip_route_output_key(sock_net(sk), fl4);
if (!IS_ERR(rt)) {
dst = &rt->dst;
-- 
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/

Re: [PATCH] sctp: reset flowi4_oif parameter on route lookup

[Xufeng Zhang]

On 04/25/2014 03:57 PM, Julian Anastasov wrote:

Hello,

On Fri, 25 Apr 2014, Xufeng Zhang wrote:

   


diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index c09757f..f8a5968 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -493,6 +493,10 @@ static void sctp_v4_get_dst(struct sctp_transport *t, 
union sctp_addr *saddr,
(AF_INET == laddr->a.sa.sa_family)) {
fl4->saddr = laddr->a.v4.sin_addr.s_addr;
fl4->fl4_sport = laddr->a.v4.sin_port;
+   /* Reset flowi4_oif so that it won't be incorrectly used
+* as input parameter to routing lookups.
+*/
+   fl4->flowi4_oif = 0;
 

May be something like this is more correct:

flowi4_update_output(fl4,
 asoc->base.sk->sk_bound_dev_if,
 RT_CONN_FLAGS(asoc->base.sk),
 daddr->v4.sin_addr.s_addr,
 laddr->a.v4.sin_addr.s_addr);
   


Thanks! you are right, I should use the available API.
I'll send V2 later.


Thanks,
Xufeng


   

rt = ip_route_output_key(sock_net(sk), fl4);
if (!IS_ERR(rt)) {
dst =&rt->dst;
 

Regards

--
Julian Anastasov

   


--
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: reset flowi4_oif parameter on route lookup

[Xufeng Zhang]

commit 813b3b5db83 (ipv4: Use caller's on-stack flowi as-is
in output route lookups.) introduces another regression which
is very similar to the problem of commit e6b45241c (ipv4: reset
flowi parameters on route connect) wants to fix:
Before we call ip_route_output_key() in sctp_v4_get_dst() to
get a dst that matches a bind address as the source address,
we have already called this function previously and the flowi
parameters have been initialized including flowi4_oif, so when
we call this function again, the process in ip_route_output_slow()
will be different because of the setting of flowi4_oif, and we'll
get a networking device which corresponds to the inputted flowi4_oif
as the output device, this is wrong because we'll never hit this
place if the previously returned source address of dst match one
of the bound addresses.

To reproduce this problem, a vlan setting is enough:
  # ifconfig eth0 up
  # route del default
  # vconfig add eth0 2
  # vconfig add eth0 3
  # ifconfig eth0.2 10.0.1.14 netmask 255.255.255.0
  # route add default gw 10.0.1.254 dev eth0.2
  # ifconfig eth0.3 10.0.0.14 netmask 255.255.255.0
  # ip rule add from 10.0.0.14 table 4
  # ip route add table 4 default via 10.0.0.254 src 10.0.0.14 dev eth0.3
  # sctp_darn -H 10.0.0.14 -P 36422 -h 10.1.4.134 -p 36422 -s -I
You'll detect that all the flow are routed to eth0.2(10.0.1.254).

Signed-off-by: Xufeng Zhang 
---
 net/sctp/protocol.c |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index c09757f..f8a5968 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -493,6 +493,10 @@ static void sctp_v4_get_dst(struct sctp_transport *t, 
union sctp_addr *saddr,
(AF_INET == laddr->a.sa.sa_family)) {
fl4->saddr = laddr->a.v4.sin_addr.s_addr;
fl4->fl4_sport = laddr->a.v4.sin_port;
+   /* Reset flowi4_oif so that it won't be incorrectly used
+* as input parameter to routing lookups.
+*/
+   fl4->flowi4_oif = 0;
rt = ip_route_output_key(sock_net(sk), fl4);
if (!IS_ERR(rt)) {
dst = &rt->dst;
-- 
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 RFC] sctp: Update HEARTBEAT timer immediately after user changed HB.interval

[Xufeng Zhang]

For an established association, if user has updated the HB.interval
parameter by setsockopt(), this new heartbeat interval will not
take effect until:
  - the expiry of the heartbeat timer and new hearbeat is sent.
  - DATA chunk has been sent and the transport resets the timer.
This could not meet the requirement of the user who need to
get HEARTBEAT sent at the specified time.

Thus, we need to update the heartbeat timer immediately after
user has changed HB.interval.

Signed-off-by: Xufeng Zhang 
---
 net/sctp/socket.c |5 +
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 9e91d6e..699ae1e 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2344,6 +2344,11 @@ static int sctp_apply_peer_addr_params(struct 
sctp_paddrparams *params,
if (trans) {
trans->hbinterval =
msecs_to_jiffies(params->spp_hbinterval);
+
+   /* Update the heartbeat timer immediately. */
+   if (!mod_timer(&trans->hb_timer,
+   sctp_transport_timeout(trans)))
+   sctp_transport_hold(trans);
} else if (asoc) {
asoc->hbinterval =
msecs_to_jiffies(params->spp_hbinterval);
-- 
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/

[V2 PATCH] sctp: Restore 'resent' bit to avoid retransmitted chunks for RTT measurements

[Xufeng Zhang]

Currently retransmitted DATA chunks could also be used for
RTT measurements since there are no flag to identify whether
the transmitted DATA chunk is a new one or a retransmitted one.
This problem is introduced by commit ae19c5486 ("sctp: remove
'resent' bit from the chunk") which inappropriately removed the
'resent' bit completely, instead of doing this, we should set
the resent bit only for the retransmitted DATA chunks.

Signed-off-by: Xufeng Zhang 
---
v1->v2:
Rmoved initialization for resent bit.
Combined two if clause

 include/net/sctp/structs.h |1 +
 net/sctp/output.c  |3 ++-
 net/sctp/outqueue.c|3 +++
 3 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 2174d8d..ea0ca5f 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -629,6 +629,7 @@ struct sctp_chunk {
 #define SCTP_NEED_FRTX 0x1
 #define SCTP_DONT_FRTX 0x2
__u16   rtt_in_progress:1,  /* This chunk used for RTT calc? */
+   resent:1,   /* Has this chunk ever been resent. */
has_tsn:1,  /* Does this chunk have a TSN yet? */
has_ssn:1,  /* Does this chunk have a SSN yet? */
singleton:1,/* Only chunk in the packet? */
diff --git a/net/sctp/output.c b/net/sctp/output.c
index e650978..0e2644d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -474,10 +474,11 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 * for a given destination transport address.
 */
 
-   if (!tp->rto_pending) {
+   if (!chunk->resent && !tp->rto_pending) {
chunk->rtt_in_progress = 1;
tp->rto_pending = 1;
}
+
has_data = 1;
}
 
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 94df758..70f4f56 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -446,6 +446,8 @@ void sctp_retransmit_mark(struct sctp_outq *q,
transport->rto_pending = 0;
}
 
+   chunk->resent = 1;
+
/* Move the chunk to the retransmit queue. The chunks
 * on the retransmit queue are always kept in order.
 */
@@ -1375,6 +1377,7 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 * instance).
 */
if (!tchunk->tsn_gap_acked &&
+   !tchunk->resent &&
tchunk->rtt_in_progress) {
tchunk->rtt_in_progress = 0;
rtt = jiffies - tchunk->sent_at;
-- 
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/

Re: [PATCH] sctp: Restore 'resent' bit to avoid retransmitted chunks for RTT measurements

[Xufeng Zhang]

On 11/22/2013 10:18 PM, Vlad Yasevich wrote:

On 11/22/2013 03:30 AM, Xufeng Zhang wrote:
   

From: Signed-off-by: Xufeng Zhang

Currently retransmitted DATA chunks could also be used for
RTT measurements since there are no flag to identify whether
the transmitted DATA chunk is a new one or a retransmitted one.
This problem is introduced by commit ae19c5486 ("sctp: remove
'resent' bit from the chunk") which inappropriately removed the
'resent' bit completely, instead of doing this, we should set
the resent bit only for the retransmitted DATA chunks.

Signed-off-by: Xufeng Zhang
---
  include/net/sctp/structs.h |1 +
  net/sctp/output.c  |   23 +--
  net/sctp/outqueue.c|3 +++
  net/sctp/sm_make_chunk.c   |1 +
  4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 2174d8d..ea0ca5f 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -629,6 +629,7 @@ struct sctp_chunk {
  #define SCTP_NEED_FRTX 0x1
  #define SCTP_DONT_FRTX 0x2
__u16   rtt_in_progress:1,  /* This chunk used for RTT calc? */
+   resent:1,   /* Has this chunk ever been resent. */
has_tsn:1,  /* Does this chunk have a TSN yet? */
has_ssn:1,  /* Does this chunk have a SSN yet? */
singleton:1,/* Only chunk in the packet? */
diff --git a/net/sctp/output.c b/net/sctp/output.c
index e650978..32c214d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -467,17 +467,20 @@ int sctp_packet_transmit(struct sctp_packet *packet)
list_for_each_entry_safe(chunk, tmp,&packet->chunk_list, list) {
list_del_init(&chunk->list);
if (sctp_chunk_is_data(chunk)) {
-   /* 6.3.1 C4) When data is in flight and when allowed
-* by rule C5, a new RTT measurement MUST be made each
-* round trip.  Furthermore, new RTT measurements
-* SHOULD be made no more than once per round-trip
-* for a given destination transport address.
-*/
-
-   if (!tp->rto_pending) {
-   chunk->rtt_in_progress = 1;
-   tp->rto_pending = 1;
+   if (!chunk->resent) {
+   /* 6.3.1 C4) When data is in flight and when 
allowed
+* by rule C5, a new RTT measurement MUST be 
made each
+* round trip.  Furthermore, new RTT 
measurements
+* SHOULD be made no more than once per 
round-trip
+* for a given destination transport address.
+*/
+
+   if (!tp->rto_pending) {
 

Could be combined as
 if (!chunk->resent&&  !tp->rto_pending) {
   


Got it!

   

+   chunk->rtt_in_progress = 1;
+   tp->rto_pending = 1;
+   }
}
+
has_data = 1;
}

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 94df758..70f4f56 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -446,6 +446,8 @@ void sctp_retransmit_mark(struct sctp_outq *q,
transport->rto_pending = 0;
}

+   chunk->resent = 1;
+
/* Move the chunk to the retransmit queue. The chunks
 * on the retransmit queue are always kept in order.
 */
@@ -1375,6 +1377,7 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 * instance).
 */
if (!tchunk->tsn_gap_acked&&
+   !tchunk->resent&&
tchunk->rtt_in_progress) {
tchunk->rtt_in_progress = 0;
rtt = jiffies - tchunk->sent_at;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fe69032..8fe89f8 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1321,6 +1321,7 @@ struct sctp_chunk *sctp_chunkify(struct sk_buff *skb,
INIT_LIST_HEAD(&retval->list);
retval->skb  = skb;
retval->asoc = (struct sctp_association *)asoc;
+   retval->resent  = 0;
 

Not needed due to zeroed out malloc.
   


Thank you!
Will send V2 later.



Thanks,
Xufeng


-vlad

   

retval->singleton= 1;

  

[PATCH] sctp: Restore 'resent' bit to avoid retransmitted chunks for RTT measurements

[Xufeng Zhang]

From: Signed-off-by: Xufeng Zhang 

Currently retransmitted DATA chunks could also be used for
RTT measurements since there are no flag to identify whether
the transmitted DATA chunk is a new one or a retransmitted one.
This problem is introduced by commit ae19c5486 ("sctp: remove
'resent' bit from the chunk") which inappropriately removed the
'resent' bit completely, instead of doing this, we should set 
the resent bit only for the retransmitted DATA chunks.

Signed-off-by: Xufeng Zhang 
---
 include/net/sctp/structs.h |1 +
 net/sctp/output.c  |   23 +--
 net/sctp/outqueue.c|3 +++
 net/sctp/sm_make_chunk.c   |1 +
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 2174d8d..ea0ca5f 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -629,6 +629,7 @@ struct sctp_chunk {
 #define SCTP_NEED_FRTX 0x1
 #define SCTP_DONT_FRTX 0x2
__u16   rtt_in_progress:1,  /* This chunk used for RTT calc? */
+   resent:1,   /* Has this chunk ever been resent. */
has_tsn:1,  /* Does this chunk have a TSN yet? */
has_ssn:1,  /* Does this chunk have a SSN yet? */
singleton:1,/* Only chunk in the packet? */
diff --git a/net/sctp/output.c b/net/sctp/output.c
index e650978..32c214d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -467,17 +467,20 @@ int sctp_packet_transmit(struct sctp_packet *packet)
list_for_each_entry_safe(chunk, tmp, &packet->chunk_list, list) {
list_del_init(&chunk->list);
if (sctp_chunk_is_data(chunk)) {
-   /* 6.3.1 C4) When data is in flight and when allowed
-* by rule C5, a new RTT measurement MUST be made each
-* round trip.  Furthermore, new RTT measurements
-* SHOULD be made no more than once per round-trip
-* for a given destination transport address.
-*/
-
-   if (!tp->rto_pending) {
-   chunk->rtt_in_progress = 1;
-   tp->rto_pending = 1;
+   if (!chunk->resent) {
+   /* 6.3.1 C4) When data is in flight and when 
allowed
+* by rule C5, a new RTT measurement MUST be 
made each
+* round trip.  Furthermore, new RTT 
measurements
+* SHOULD be made no more than once per 
round-trip
+* for a given destination transport address.
+*/
+
+   if (!tp->rto_pending) {
+   chunk->rtt_in_progress = 1;
+   tp->rto_pending = 1;
+   }
}
+
has_data = 1;
}
 
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 94df758..70f4f56 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -446,6 +446,8 @@ void sctp_retransmit_mark(struct sctp_outq *q,
transport->rto_pending = 0;
}
 
+   chunk->resent = 1;
+
/* Move the chunk to the retransmit queue. The chunks
 * on the retransmit queue are always kept in order.
 */
@@ -1375,6 +1377,7 @@ static void sctp_check_transmitted(struct sctp_outq *q,
 * instance).
 */
if (!tchunk->tsn_gap_acked &&
+   !tchunk->resent &&
tchunk->rtt_in_progress) {
tchunk->rtt_in_progress = 0;
rtt = jiffies - tchunk->sent_at;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fe69032..8fe89f8 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1321,6 +1321,7 @@ struct sctp_chunk *sctp_chunkify(struct sk_buff *skb,
INIT_LIST_HEAD(&retval->list);
retval->skb = skb;
retval->asoc= (struct sctp_association *)asoc;
+   retval->resent  = 0;
retval->singleton   = 1;
 
retval->fast_retransmit = SCTP_CAN_FRTX;
-- 
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/

Re: [PATCH] sctp: set association state to established in dupcook_a handler

[Xufeng Zhang]

Hi David,

Is this fix fine to you for -stable?


Thanks,
Xufeng

On 06/05/2013 10:55 AM, Xufeng Zhang wrote:

On 06/03/2013 03:52 PM, Xufeng Zhang wrote:
3.4-stable review patch.  If anyone has any objections, please let me 
know.

Sorry Greg, David -- I did not fully understand all the details
of the stable kernel process earlier.

I have since checked the networking stable queue here:

http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

to confirm this upstream commit 9839ff0d has not yet been queued.

It can be applied to kernel versions<3.0, 3.4>, and is
present in mainline for 3.8+ kernels.

I think it makes sense to queue for stable because it
fixes the SCTP association can get stuck in SCTP_STATE_SHUTDOWN_PENDING
state forever in the below test case:
1). Set SCTP parameters on A (slow detection of link failure):
net.sctp.association_max_retrans = 5
net.sctp.path_max_retrans = 5
net.sctp.hb_interval = 3
2). Set SCTP parameters on B (fast detection of link failure):
net.sctp.association_max_retrans = 2
net.sctp.path_max_retrans = 2
net.sctp.hb_interval = 1000
3). Start sctp_darn on both sides:
A(interactive):
sctp_darn -H 192.168.100.10 -P 256 -h 192.168.100.100 -p 256 -I -s
B:
sctp_darn -H 192.168.100.100 -P 256 -h 192.168.100.10 -p 256 -l&
4). Send data on A to establish the SCTP association:
snd=5
5). Block SCTP traffic on B (simulates a network failure):
iptables -t filter -I INPUT 1 -p sctp --dport 256 -j DROP
iptables -t filter -I OUTPUT 1 -p sctp --dport 256 -j DROP
Then quickly send data on A and then shutdown (A goes into 
SHUTDOWN_PENDING state):

snd=5
snd=5
shutdown
6). Wait for link to drop on B (Recieved SCTP_COMM_LOST), and quickly 
kill the listener,

open the firewall for SCTP, then start an SCTP sender:
kill $PID
iptables -t filter -D INPUT 1
iptables -t filter -D OUTPUT 1
sctp_darn -H 192.168.100.100 -P 256 -h 192.168.100.10 -p 256 -s
Press  to send data to trigger sending INIT to A, the SHUTDOWN 
on A will failed and

the association on A remains in SHUTDOWN_PENDING (5) state indefinitely.


However if David doesn't think it is worth bothering with for
net stable, then that is of course fine too.


Thanks,
Xufeng




--


From: Xufeng Zhang

[ Upstream commit 9839ff0dead906e85e4d17490aeff87a5859a157 ]

While sctp handling a duplicate COOKIE-ECHO and the action is
'Association restart', sctp_sf_do_dupcook_a() will processing
the unexpected COOKIE-ECHO for peer restart, but it does not set
the association state to SCTP_STATE_ESTABLISHED, so the association
could stuck in SCTP_STATE_SHUTDOWN_PENDING state forever.
This violates the sctp specification:
   RFC 4960 5.2.4. Handle a COOKIE ECHO when a TCB Exists
   Action
   A) In this case, the peer may have restarted. .
  After this, the endpoint shall enter the ESTABLISHED state.

To resolve this problem, adding a SCTP_CMD_NEW_STATE cmd to the
command list before SCTP_CMD_REPLY cmd, this will set the restart
association to SCTP_STATE_ESTABLISHED state properly and also avoid
I-bit being set in the DATA chunk header when COOKIE_ACK is bundled
with DATA chunks.

Signed-off-by: Xufeng Zhang
Acked-by: Neil Horman
Acked-by: Vlad Yasevich
Signed-off-by: David S. Miller
---
  net/sctp/sm_statefuns.c |4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index cb1c430..ab08f65 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1747,8 +1747,10 @@ static sctp_disposition_t 
sctp_sf_do_dupcook_a(const struct sctp_endpoint *ep,


  /* Update the content of current association. */
  sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, 
SCTP_ASOC(new_asoc));

-sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
  sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
+sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
+SCTP_STATE(SCTP_STATE_ESTABLISHED));
+sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
  return SCTP_DISPOSITION_CONSUME;

  nomem_ev:





--
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: set association state to established in dupcook_a handler

[Xufeng Zhang]

On 06/03/2013 03:52 PM, Xufeng Zhang wrote:

3.4-stable review patch.  If anyone has any objections, please let me know.
   

Sorry Greg, David -- I did not fully understand all the details
of the stable kernel process earlier.

I have since checked the networking stable queue here:

http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

to confirm this upstream commit 9839ff0d has not yet been queued.

It can be applied to kernel versions<3.0, 3.4>, and is
present in mainline for 3.8+ kernels.

I think it makes sense to queue for stable because it
fixes the SCTP association can get stuck in SCTP_STATE_SHUTDOWN_PENDING
state forever in the below test case:
1). Set SCTP parameters on A (slow detection of link failure):
net.sctp.association_max_retrans = 5
net.sctp.path_max_retrans = 5
net.sctp.hb_interval = 3
2). Set SCTP parameters on B (fast detection of link failure):
net.sctp.association_max_retrans = 2
net.sctp.path_max_retrans = 2
net.sctp.hb_interval = 1000
3). Start sctp_darn on both sides:
A(interactive):
sctp_darn -H 192.168.100.10 -P 256 -h 192.168.100.100 -p 256 -I -s
B:
sctp_darn -H 192.168.100.100 -P 256 -h 192.168.100.10 -p 256 -l&
4). Send data on A to establish the SCTP association:
snd=5
5). Block SCTP traffic on B (simulates a network failure):
iptables -t filter -I INPUT 1 -p sctp --dport 256 -j DROP
iptables -t filter -I OUTPUT 1 -p sctp --dport 256 -j DROP
Then quickly send data on A and then shutdown (A goes into SHUTDOWN_PENDING 
state):
snd=5
snd=5
shutdown
6). Wait for link to drop on B (Recieved SCTP_COMM_LOST), and quickly kill the 
listener,
open the firewall for SCTP, then start an SCTP sender:
kill $PID
iptables -t filter -D INPUT 1
iptables -t filter -D OUTPUT 1
sctp_darn -H 192.168.100.100 -P 256 -h 192.168.100.10 -p 256 -s
Press  to send data to trigger sending INIT to A, the SHUTDOWN on A will 
failed and
the association on A remains in SHUTDOWN_PENDING (5) state indefinitely.


However if David doesn't think it is worth bothering with for
net stable, then that is of course fine too.


Thanks,
Xufeng




--


From: Xufeng Zhang

[ Upstream commit 9839ff0dead906e85e4d17490aeff87a5859a157 ]

While sctp handling a duplicate COOKIE-ECHO and the action is
'Association restart', sctp_sf_do_dupcook_a() will processing
the unexpected COOKIE-ECHO for peer restart, but it does not set
the association state to SCTP_STATE_ESTABLISHED, so the association
could stuck in SCTP_STATE_SHUTDOWN_PENDING state forever.
This violates the sctp specification:
   RFC 4960 5.2.4. Handle a COOKIE ECHO when a TCB Exists
   Action
   A) In this case, the peer may have restarted. .
  After this, the endpoint shall enter the ESTABLISHED state.

To resolve this problem, adding a SCTP_CMD_NEW_STATE cmd to the
command list before SCTP_CMD_REPLY cmd, this will set the restart
association to SCTP_STATE_ESTABLISHED state properly and also avoid
I-bit being set in the DATA chunk header when COOKIE_ACK is bundled
with DATA chunks.

Signed-off-by: Xufeng Zhang
Acked-by: Neil Horman
Acked-by: Vlad Yasevich
Signed-off-by: David S. Miller
---
  net/sctp/sm_statefuns.c |4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index cb1c430..ab08f65 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1747,8 +1747,10 @@ static sctp_disposition_t sctp_sf_do_dupcook_a(const 
struct sctp_endpoint *ep,

/* Update the content of current association. */
sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
-   sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
+   sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
+   SCTP_STATE(SCTP_STATE_ESTABLISHED));
+   sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
return SCTP_DISPOSITION_CONSUME;

  nomem_ev:
   


--
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: set association state to established in dupcook_a handler

[Xufeng Zhang]

On 06/05/2013 08:13 AM, Ben Hutchings wrote:

On Tue, 2013-06-04 at 10:00 +0800, Xufeng Zhang wrote:
   

On 06/03/2013 10:28 PM, Greg KH wrote:
 

On Mon, Jun 03, 2013 at 03:52:58PM +0800, Xufeng Zhang wrote:

   

3.4-stable review patch.  If anyone has any objections, please let me know.

 

Really?  What are you going to do with it if no one objects?

   

Sorry, I don't have any experience with stable patch and I consult
stable_kernel_rules.txt, there are not much special rules and here I just
want to specify the kernel version I wish it to be applied to.

 

I know I'm sure not going to do anything with it...

(hint, this isn't how you ask for networking stable patches to be
applied.)

   

Ok, thanks for your hint, I'll resend the patch.
 

The heading you used should only be used by a stable branch maintainer
sending out a series of patches they've already accepted, prior to
making a new release.  If you want a fix to be added to be applied to
one or more stable branches, you should write in the form of a request.
And if you're just not sure, then you should write in the form of a
question.

Networking is a special case in that the subsystem maintainer, David
Miller, generally collects together and backports fixes for all the
official 3.x branches.  So you should send your request to him (but do
cc the stable and subsystem lists as well).
   

Thank you, Ben!
Now it's more clear.


Thanks,
Xufeng

Ben.

   


--
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: set association state to established in dupcook_a handler

[Xufeng Zhang]

On 06/03/2013 10:28 PM, Greg KH wrote:

On Mon, Jun 03, 2013 at 03:52:58PM +0800, Xufeng Zhang wrote:
   

3.4-stable review patch.  If anyone has any objections, please let me know.
 

Really?  What are you going to do with it if no one objects?
   

Sorry, I don't have any experience with stable patch and I consult
stable_kernel_rules.txt, there are not much special rules and here I just
want to specify the kernel version I wish it to be applied to.


I know I'm sure not going to do anything with it...

(hint, this isn't how you ask for networking stable patches to be
applied.)
   

Ok, thanks for your hint, I'll resend the patch.


Thanks,
Xufeng

greg k-h

   


--
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: set association state to established in dupcook_a handler

[Xufeng Zhang]

3.4-stable review patch.  If anyone has any objections, please let me know.

--


From: Xufeng Zhang 

[ Upstream commit 9839ff0dead906e85e4d17490aeff87a5859a157 ]

While sctp handling a duplicate COOKIE-ECHO and the action is
'Association restart', sctp_sf_do_dupcook_a() will processing
the unexpected COOKIE-ECHO for peer restart, but it does not set
the association state to SCTP_STATE_ESTABLISHED, so the association
could stuck in SCTP_STATE_SHUTDOWN_PENDING state forever.
This violates the sctp specification:
  RFC 4960 5.2.4. Handle a COOKIE ECHO when a TCB Exists
  Action
  A) In this case, the peer may have restarted. .
 After this, the endpoint shall enter the ESTABLISHED state.

To resolve this problem, adding a SCTP_CMD_NEW_STATE cmd to the
command list before SCTP_CMD_REPLY cmd, this will set the restart
association to SCTP_STATE_ESTABLISHED state properly and also avoid
I-bit being set in the DATA chunk header when COOKIE_ACK is bundled
with DATA chunks.

Signed-off-by: Xufeng Zhang 
Acked-by: Neil Horman 
Acked-by: Vlad Yasevich 
Signed-off-by: David S. Miller 
---
 net/sctp/sm_statefuns.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index cb1c430..ab08f65 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1747,8 +1747,10 @@ static sctp_disposition_t sctp_sf_do_dupcook_a(const 
struct sctp_endpoint *ep,
 
/* Update the content of current association. */
sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
-   sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
+   sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
+   SCTP_STATE(SCTP_STATE_ESTABLISHED));
+   sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
return SCTP_DISPOSITION_CONSUME;
 
 nomem_ev:
-- 
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/

Re: [PATCH] sctp: don't break the loop while meeting the active_path so as to find the matched transport

[Xufeng Zhang]

>>
>> Thanks for your review, Neil!
>>
>> I know what you mean, yes, it's most probably that the searched TSN was
>> transmitted in the currently active_path, but what should we do if not.
>>
>> Check the comment in sctp_assoc_lookup_tsn() function:
>> /* Let's be hopeful and check the active_path first. */
>> /* If not found, go search all the other transports. */
>>
>> It has checked the active_path first and then traverse all the other
>> transports in
>> the transport_addr_list except the active_path.
>>
>> So what I want to fix here is the inconsistency between the function
>> should do and
>> the code actually does.
>>
> I understand what you're doing, and I agree that the fix is functional
> (Hence
> my "This works" statement in my last note).  What I'm suggesting is that,
> since
> you're messing about in that code anyway that you clean it up while your at
> it,
> so that we don't need to have the if (transport == active) check at all.
> We
> trade in some extra work in a non-critical path (sctp_assoc_set_primary),
> for
> the removal of an extra for loop operation and a conditional check in a
> much
> hotter path.  Something like this (completely untested), is what I was
> thinking

Aha, seems I have some misunderstanding previously, now I got your point.
Yeah, it's better to do the clean up by this way, and this fix looks fine to me,
but I didn't have a test case to test this, actually this problem was detected
by code review, so I would like to leave the rest of this work to
determine by you.

Thank you very much for your clarification!


Thanks,
Xufeng

>
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 43cd0dd..8ae873c 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -505,6 +505,9 @@ void sctp_assoc_set_primary(struct sctp_association
> *asoc,
>
>   asoc->peer.primary_path = transport;
>
> + list_del_rcu(&transport->transports);
> + list_add_rcu(&transport->transports, &asoc->peer.transport_addr_list);
> +
>   /* Set a default msg_name for events. */
>   memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
>  sizeof(union sctp_addr));
> @@ -1040,7 +1043,6 @@ struct sctp_chunk *sctp_get_ecne_prepend(struct
> sctp_association *asoc)
>  struct sctp_transport *sctp_assoc_lookup_tsn(struct sctp_association
> *asoc,
>__u32 tsn)
>  {
> - struct sctp_transport *active;
>   struct sctp_transport *match;
>   struct sctp_transport *transport;
>   struct sctp_chunk *chunk;
> @@ -1057,29 +1059,16 @@ struct sctp_transport *sctp_assoc_lookup_tsn(struct
> sctp_association *asoc,
>* The general strategy is to search each transport's transmitted
>* list.   Return which transport this TSN lives on.
>*
> -  * Let's be hopeful and check the active_path first.
> -  * Another optimization would be to know if there is only one
> -  * outbound path and not have to look for the TSN at all.
> +  * Note, that sctp_assoc_set_primary does a move to front operation
> +  * on the active_path transport, so this code implicitly checks
> +  * the active_path first, as we most commonly expect to find our TSN
> +  * there.
>*
>*/
>
> - active = asoc->peer.active_path;
> -
> - list_for_each_entry(chunk, &active->transmitted,
> - transmitted_list) {
> -
> - if (key == chunk->subh.data_hdr->tsn) {
> - match = active;
> - goto out;
> - }
> - }
> -
> - /* If not found, go search all the other transports. */
>   list_for_each_entry(transport, &asoc->peer.transport_addr_list,
>   transports) {
>
> - if (transport == active)
> - break;
>   list_for_each_entry(chunk, &transport->transmitted,
>   transmitted_list) {
>   if (key == chunk->subh.data_hdr->tsn) {
>
--
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: don't break the loop while meeting the active_path so as to find the matched transport

[Xufeng Zhang]

On 3/8/13, Neil Horman  wrote:
> On Fri, Mar 08, 2013 at 03:39:37PM +0800, Xufeng Zhang wrote:
>> sctp_assoc_lookup_tsn() function searchs which transport a certain TSN
>> was sent on, if not found in the active_path transport, then go search
>> all the other transports in the peer's transport_addr_list, however, we
>> should continue to the next entry rather than break the loop when meet
>> the active_path transport.
>>
>> Signed-off-by: Xufeng Zhang 
>> ---
>>  net/sctp/associola.c |2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 43cd0dd..d2709e2 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1079,7 +1079,7 @@ struct sctp_transport *sctp_assoc_lookup_tsn(struct
>> sctp_association *asoc,
>>  transports) {
>>
>>  if (transport == active)
>> -break;
>> +continue;
>>  list_for_each_entry(chunk, &transport->transmitted,
>>  transmitted_list) {
>>  if (key == chunk->subh.data_hdr->tsn) {
>> --
>> 1.7.0.2
>>
>>
>
> This works, but what might be better would be if we did a move to front
> heuristic in sctp_assoc_set_primary.  E.g. when we set the active_path, move
> the
> requisite transport to the front of the transport_addr_list.  If we did
> that,
> then we could just do one for loop in sctp_assoc_lookup_tsn and wind up
> implicitly check the active path first without having to check it seprately
> and
> skip it in the second for loop.

Thanks for your review, Neil!

I know what you mean, yes, it's most probably that the searched TSN was
transmitted in the currently active_path, but what should we do if not.

Check the comment in sctp_assoc_lookup_tsn() function:
/* Let's be hopeful and check the active_path first. */
/* If not found, go search all the other transports. */

It has checked the active_path first and then traverse all the other
transports in
the transport_addr_list except the active_path.

So what I want to fix here is the inconsistency between the function
should do and
the code actually does.



Thanks,
Xufeng


>
> 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: don't break the loop while meeting the active_path so as to find the matched transport

[Xufeng Zhang]

sctp_assoc_lookup_tsn() function searchs which transport a certain TSN
was sent on, if not found in the active_path transport, then go search
all the other transports in the peer's transport_addr_list, however, we
should continue to the next entry rather than break the loop when meet
the active_path transport.

Signed-off-by: Xufeng Zhang 
---
 net/sctp/associola.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 43cd0dd..d2709e2 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1079,7 +1079,7 @@ struct sctp_transport *sctp_assoc_lookup_tsn(struct 
sctp_association *asoc,
transports) {
 
if (transport == active)
-   break;
+   continue;
list_for_each_entry(chunk, &transport->transmitted,
transmitted_list) {
if (key == chunk->subh.data_hdr->tsn) {
-- 
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/

Re: [PATCH] sctp: set association state to established in dupcook_a handler

[Xufeng Zhang]

On 1/23/13, Vlad Yasevich  wrote:
> On 01/23/2013 08:46 AM, Neil Horman wrote:
>> On Wed, Jan 23, 2013 at 03:38:40PM +0800, xufengzhang.m...@gmail.com
>> wrote:
>>> From: Xufeng Zhang 
>>>
>>> While sctp handling a duplicate COOKIE-ECHO and the action is
>>> 'Association restart', sctp_sf_do_dupcook_a() will processing
>>> the unexpected COOKIE-ECHO for peer restart, but it does not set
>>> the association state to SCTP_STATE_ESTABLISHED, so the association
>>> could stuck in SCTP_STATE_SHUTDOWN_PENDING state forever.
>>> This violates the sctp specification:
>>>RFC 4960 5.2.4. Handle a COOKIE ECHO when a TCB Exists
>>>Action
>>>A) In this case, the peer may have restarted. .
>>>   After this, the endpoint shall enter the ESTABLISHED state.
>>>
>>> Fix this problem by adding a SCTP_CMD_NEW_STATE cmd to the command
>>> list so as to set the restart association to SCTP_STATE_ESTABLISHED
>>> state properly.
>>>
>>> Signed-off-by: Xufeng Zhang 
>>> ---
>>>   net/sctp/sm_statefuns.c |2 ++
>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
>>> index 618ec7e..528f1c8 100644
>>> --- a/net/sctp/sm_statefuns.c
>>> +++ b/net/sctp/sm_statefuns.c
>>> @@ -1779,6 +1779,8 @@ static sctp_disposition_t
>>> sctp_sf_do_dupcook_a(struct net *net,
>>>
>>> /* Update the content of current association. */
>>> sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC,
>>> SCTP_ASOC(new_asoc));
>>> +   sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
>>> +   SCTP_STATE(SCTP_STATE_ESTABLISHED));
>>> sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
>>> sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
>>> return SCTP_DISPOSITION_CONSUME;
>>> --
>>> 1.7.0.2
>>>
>>>
>>
>> Looks reasonable to me, thanks
>>
>> nit: The RFC indicate the association should enter the ESTABLISHED state
>> after
>> preforming all other actions, so it seems that the state change should
>> occur
>> after the ULP event is sent
>>
>
> I have a slight concern here that if we change state last, then any data
> that may have been bundled with COOKIE-ACK as part of CMD_REPLY will get
> the SACK_IMMEDIATE flag set since we are still in the SHUTDOWN_PENDING
> state.
>
> I would be more correct (and would match sctp_sf_do_5_1D_ce) to
> do it in this order:
>UPDATE_ASSOC  - resets all the congestion/association variables
>EVENT_UP  - send RESTART to USER
>NEW_STATE  - set ESTABLISHED state (as per spec)
>REPLY  - Send cookie-ack along with any pending data.

Yep, I agree with you, I'll send V2 patch based on your suggestion.
Thanks for your review!


Thanks,
Xufeng

>
> -vlad
>> 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: set association state to established in dupcook_a handler

[Xufeng Zhang]

On 1/23/13, Neil Horman  wrote:
> On Wed, Jan 23, 2013 at 03:38:40PM +0800, xufengzhang.m...@gmail.com wrote:
>> From: Xufeng Zhang 
>>
>> While sctp handling a duplicate COOKIE-ECHO and the action is
>> 'Association restart', sctp_sf_do_dupcook_a() will processing
>> the unexpected COOKIE-ECHO for peer restart, but it does not set
>> the association state to SCTP_STATE_ESTABLISHED, so the association
>> could stuck in SCTP_STATE_SHUTDOWN_PENDING state forever.
>> This violates the sctp specification:
>>   RFC 4960 5.2.4. Handle a COOKIE ECHO when a TCB Exists
>>   Action
>>   A) In this case, the peer may have restarted. .
>>  After this, the endpoint shall enter the ESTABLISHED state.
>>
>> Fix this problem by adding a SCTP_CMD_NEW_STATE cmd to the command
>> list so as to set the restart association to SCTP_STATE_ESTABLISHED
>> state properly.
>>
>> Signed-off-by: Xufeng Zhang 
>> ---
>>  net/sctp/sm_statefuns.c |2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
>> index 618ec7e..528f1c8 100644
>> --- a/net/sctp/sm_statefuns.c
>> +++ b/net/sctp/sm_statefuns.c
>> @@ -1779,6 +1779,8 @@ static sctp_disposition_t
>> sctp_sf_do_dupcook_a(struct net *net,
>>
>>  /* Update the content of current association. */
>>  sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
>> +sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
>> +SCTP_STATE(SCTP_STATE_ESTABLISHED));
>>  sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
>>  sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
>>  return SCTP_DISPOSITION_CONSUME;
>> --
>> 1.7.0.2
>>
>>
>
> Looks reasonable to me, thanks
>
> nit: The RFC indicate the association should enter the ESTABLISHED state
> after
> preforming all other actions, so it seems that the state change should
> occur
> after the ULP event is sent

Good catch! I'll do what vlad suggested.
Thanks for your review!


Thanks,
Xufeng

>
> 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]

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 

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

[xufeng zhang]

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 th

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

[Xufeng Zhang]

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_out

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

[Xufeng Zhang]

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,
>> + &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());

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

[Xufeng Zhang]

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,
> +  &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 */
&g

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

[Xufeng Zhang]

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]

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]

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

[Xufeng Zhang]

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

[Xufeng Zhang]

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

[Xufeng Zhang]

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]

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_t

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

[Xufeng Zhang]

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]

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(&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;

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

[Xufeng Zhang]

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

[xufeng zhang]

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(&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.


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  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]

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/