> -----Original Message-----
> From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Sent: Tuesday, 17 January 2023 13:02
> To: Sriram Yagnaraman <sriram.yagnaraman@xxxxxxxx>
> Cc: netfilter-devel@xxxxxxxxxxxxxxx; Florian Westphal <fw@xxxxxxxxx>;
> Marcelo Ricardo Leitner <mleitner@xxxxxxxxxx>; Long Xin
> <lxin@xxxxxxxxxx>; Claudio Porfiri <claudio.porfiri@xxxxxxxxxxxx>
> Subject: Re: [PATCH 3/3] netfilter: conntrack: unify established states for SCTP
> paths
>
> On Tue, Jan 17, 2023 at 12:54:40PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 16, 2023 at 10:35:56AM +0100, Sriram Yagnaraman wrote:
> > > An SCTP endpoint can start an association through a path and tear it
> > > down over another one. That means the initial path will not see the
> > > shutdown sequence, and the conntrack entry will remain in
> > > ESTABLISHED state for 5 days.
> > >
> > > By merging the HEARTBEAT_ACKED and ESTABLISHED states into one
> > > ESTABLISHED state, there remains no difference between a primary or
> > > secondary path. The timeout for the merged ESTABLISHED state is set
> > > to
> > > 210 seconds (hb_interval * max_path_retrans + rto_max). So, even if
> > > a path doesn't see the shutdown sequence, it will expire in a
> > > reasonable amount of time.
> > >
> > > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@xxxxxxxx>
> > > ---
> > > .../uapi/linux/netfilter/nf_conntrack_sctp.h | 4 +-
> > > .../linux/netfilter/nfnetlink_cttimeout.h | 4 +-
> > > net/netfilter/nf_conntrack_proto_sctp.c | 90 ++++++++-----------
> > > net/netfilter/nf_conntrack_standalone.c | 16 ----
> > > 4 files changed, 42 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > index c742469afe21..150fc3c056ea 100644
> > > --- a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > +++ b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > @@ -15,8 +15,8 @@ enum sctp_conntrack {
> > > SCTP_CONNTRACK_SHUTDOWN_RECD,
> > > SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
> > > SCTP_CONNTRACK_HEARTBEAT_SENT,
> > > - SCTP_CONNTRACK_HEARTBEAT_ACKED,
> > > - SCTP_CONNTRACK_DATA_SENT,
> > > + SCTP_CONNTRACK_HEARTBEAT_ACKED, /* no longer used */
> > > + SCTP_CONNTRACK_DATA_SENT, /* no longer used */
> >
> > _DATA_SENT was added in the previous development cycle, to my
> > knowledged it has been present in 6.1-rc only. Then I think you can
>
> Actually, I mean 6.2-rc releases.
>
> > post a patch to revert this explaining why there is no need for
> > _DATA_SENT anymore. You can revert it before this patch (with my
> > suggestion, your series will contain with 4 patches).
I only removed the DATA_SENT state, SCTP tracker still reacts to DATA/SACK chunks to move to HEARTBEAT_SENT state. But I realize that reacting to DATA/SACK works only for new connections, while on connection re-use we still depend on HEARTBEAT for the secondary paths. Perhaps, I should revert the whole patch as you suggest.
> >
> > One question of mine: Did you extract the new established timeout from
> > RFC, where this formula came from?
> >
> > 210 seconds = hb_interval * max_path_retrans + rto_max
Took it from the HEARTBEAT_ACKED state timeout, explained in the patch that introduced the state: https://lore.kernel.org/all/20150714122311.8DA8EA0C9A@xxxxxxxxxxxxxxx/
An SCTP endpoint will retry a path every "hb_interval" seconds for "max_path_retrans" times with a timeout of "rto_max", before treating it as unreachable. So, I am guessing that is the science behind the formula.
> >
> > And thanks, if this works for you, I prefer this incremental approach
> > by improving the existing SCTP tracker.
Thanks for taking the time to review. I will come back with more improvements once this series is approved :)
