netdev - Re: [PATCH] net: sctp: recover a tranport when an ack comes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20131115202536.GD29877@hmsreliant.think-freely.org>
Date:	Fri, 15 Nov 2013 15:25:36 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Chang <changxiangzhong@...il.com>
Cc:	Vlad Yasevich <vyasevich@...il.com>, davem@...emloft.net,
	linux-sctp@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, dreibh@...ula.no, ernstgr@...ula.no
Subject: Re: [PATCH] net: sctp: recover a tranport when an ack comes

On Fri, Nov 15, 2013 at 08:59:42PM +0100, Chang wrote:
> I've tried to catch you guys up~
> 
> Here's a quick question:
> 
> Where are the default [transport->pf_retrans] and
> [transport->pathmaxrtx] set? I could figure out that they could be
> set through setsockopt(SCTP_PEER_ADDR_THLDS, ...) (but it seems like
> the SCTP library has not supported such option yet, maybe that's due
Yeah, it looks like Daniel hasn't implemented option 31 yet.  You could
certainly issue that socket option on your own thought, without the help of the
library.

> to my library is out of date). So by default both of the two
> threshold are zero.
> 
Thats not tre, pathmaxrtx should be initalized to 5, which is what the per-net
sysctl get initalized to in sctp_net_init.

> Here's my question: Does it go conflict with "the recommended value
> for Path.Max.Retrans in the standard is 5"?
> 
pathmaxrtx is initalized to 5 in sctp_init_sock, so we're in compliance with the
RFC hre.

> Thanks!
> 
> On 11/15/2013 03:56 PM, Neil Horman wrote:
> >On Fri, Nov 15, 2013 at 09:00:58AM -0500, Vlad Yasevich wrote:
> >>On 11/15/2013 07:30 AM, Neil Horman wrote:
> >>>On Thu, Nov 14, 2013 at 09:34:55PM -0500, Vlad Yasevich wrote:
> >>>>On 11/14/2013 03:40 PM, Chang Xiangzhong wrote:
> >>>>>Expected Behavior:
> >>>>>When hearing an ack from a tranport/path, set its state to normal/on if it's
> >>>>>in abnormal(__partial_failure__ or inactive) state.
> >>>>>
> >>>>>state machine of tranport->state
> >>>>>Whenever a T3_RTX timer expires, then transport->error_count++.
> >>>>>When (association->pf_retrans < transport->error_count < tranport->pathmaxrtx)
> >>>>>	transport->state = SCTP_PF //partial failure
> >>>>>
> >>>>>When a heartbeat-ack comes or conventional ack acknowledged its availability,
> >>>>>	transport->state = SCTP_ON
> >>>>>
> >>>>>Signed-off-by: Chang Xiangzhong <changxiangzhong@...il.com>
> >>>>>Fixes: 5aa93bcf66f ("sctp: Implement quick failover draft from tsvwg")
> >>>>I don't think this is right.  The spec states:
> >>>>  8.  ACKs for retransmissions do not transition a PF destination back
> >>>>        to Active state, since a sender cannot disambiguate whether the
> >>>>        ack was for the original transmission or the retransmission(s).
> >>>>
> >>>>
> >>>>Now, the proper way to this would would be modify
> >>>>sctp_assoc_control_transport() to transition the transport state to
> >>>>ACTIVE if it was PF transport that was chosen to send data.
> >>>>
> >>>>-vlad
> >>>>
> >>>I agree, this patch doesn't agree with the spec, the only time we transition
> >>>from PF to ACTIVE should be on receipt of ack of new data.
> >>
> >>You mean HB ACK, right?  The 02 spec that see on the ietf site doesn't
> >>mention anything about transition on SACKs.  Also, there is no way to
> >>tell right now if the ack is for new or retransmitted data.  We could
> >>mark chunks that are retransmitted though.
> >>
> >Yes, sorry I wasn't clear, I was speaknig about HB Acks.
> >
> >>>I'm not even sure if
> >>>we should allow PF transports to be selected to send new data.  Currently a
> >>>potentially failed transport will get ignored when specified, and the stack will
> >>>use the active path in its place.  Only if all transports are PF will a PF
> >>>transport be chosen.
> >>Not even that :(.  If all transports are PF, we are going to camp on
> >>the primary path instead of choosing a PF transport with the lowest
> >>error count.
> >>
> >Yes, we don't do the smallest error count check, though I have to wonder if
> >thats really worthwhile.  If all your transports are PF, you're a step away from
> >a connection reset anyway.
> >Neil
> >
> >>-vlad
> >>
> >>>Neil
> >>>
> >>>>>---
> >>>>>  net/sctp/outqueue.c |    1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>>diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> >>>>>index 94df758..2557fa5 100644
> >>>>>--- a/net/sctp/outqueue.c
> >>>>>+++ b/net/sctp/outqueue.c
> >>>>>@@ -1517,6 +1517,7 @@ static void sctp_check_transmitted(struct sctp_outq *q,
> >>>>>  			 * active if it is not so marked.
> >>>>>  			 */
> >>>>>  			if ((transport->state == SCTP_INACTIVE ||
> >>>>>+			     transport->state == SCTP_PF ||
> >>>>>  			     transport->state == SCTP_UNCONFIRMED) &&
> >>>>>  			    sctp_cmp_addr_exact(&transport->ipaddr, saddr)) {
> >>>>>  				sctp_assoc_control_transport(
> >>>>>
> >>>>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> >>the body of a message to majordomo@...r.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html