| Message ID | 6ee630f777cada3259b29e732e7ea9321a99197b.1696172868.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [nf] netfilter: handle the connecting collision properly in nf_conntrack_proto_sctp | expand |
On Sun, Oct 1, 2023 at 11:07 AM Xin Long <lucien.xin@gmail.com> wrote: > > In Scenario A and B below, as the delayed INIT_ACK always changes the peer > vtag, SCTP ct with the incorrect vtag may cause packet loss. > > Scenario A: INIT_ACK is delayed until the peer receives its own INIT_ACK > > 192.168.1.2 > 192.168.1.1: [INIT] [init tag: 1328086772] > 192.168.1.1 > 192.168.1.2: [INIT] [init tag: 1414468151] > 192.168.1.2 > 192.168.1.1: [INIT ACK] [init tag: 1328086772] > 192.168.1.1 > 192.168.1.2: [INIT ACK] [init tag: 1650211246] * > 192.168.1.2 > 192.168.1.1: [COOKIE ECHO] > 192.168.1.1 > 192.168.1.2: [COOKIE ECHO] > 192.168.1.2 > 192.168.1.1: [COOKIE ACK] > > Scenario B: INIT_ACK is delayed until the peer completes its own handshake > > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408] > 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885] > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT ACK] [init tag: 3922216408] > 192.168.1.1 > 192.168.1.2: sctp (1) [COOKIE ECHO] > 192.168.1.2 > 192.168.1.1: sctp (1) [COOKIE ACK] > 192.168.1.1 > 192.168.1.2: sctp (1) [INIT ACK] [init tag: 3914796021] * > > This patch fixes it as below: > > In SCTP_CID_INIT processing: > - clear ct->proto.sctp.init[!dir] if ct->proto.sctp.init[dir] && > ct->proto.sctp.init[!dir]. (Scenario E) > - set ct->proto.sctp.init[dir]. > > In SCTP_CID_INIT_ACK processing: > - drop it if !ct->proto.sctp.init[!dir] && ct->proto.sctp.vtag[!dir] && > ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario B, Scenario C) > - drop it if ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] && > ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario A) > > In SCTP_CID_COOKIE_ACK processing: > - clear ct->proto.sctp.init[dir] and ct->proto.sctp.init[!dir]. (Scenario D) > > Also, it's important to allow the ct state to move forward with cookie_echo > and cookie_ack from the opposite dir for the collision scenarios. > > There are also other Scenarios where it should allow the packet through, > addressed by the processing above: > > Scenario C: new CT is created by INIT_ACK. > > Scenario D: start INIT on the existing ESTABLISHED ct. > > Scenario E: start INIT after the old collision on the existing ESTABLISHED ct. > > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408] > 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885] > (both side are stopped, then start new connection again in hours) > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 242308742] > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > include/linux/netfilter/nf_conntrack_sctp.h | 1 + > net/netfilter/nf_conntrack_proto_sctp.c | 41 ++++++++++++++++----- > 2 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/include/linux/netfilter/nf_conntrack_sctp.h b/include/linux/netfilter/nf_conntrack_sctp.h > index 625f491b95de..fb31312825ae 100644 > --- a/include/linux/netfilter/nf_conntrack_sctp.h > +++ b/include/linux/netfilter/nf_conntrack_sctp.h > @@ -9,6 +9,7 @@ struct ip_ct_sctp { > enum sctp_conntrack state; > > __be32 vtag[IP_CT_DIR_MAX]; > + u8 init[IP_CT_DIR_MAX]; > u8 last_dir; > u8 flags; > }; > diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c > index b6bcc8f2f46b..91aee286d503 100644 > --- a/net/netfilter/nf_conntrack_proto_sctp.c > +++ b/net/netfilter/nf_conntrack_proto_sctp.c > @@ -112,7 +112,7 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = { > /* shutdown_ack */ {sSA, sCL, sCW, sCE, sES, sSA, sSA, sSA, sSA}, > /* error */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't have Stale cookie*/ > /* cookie_echo */ {sCL, sCL, sCE, sCE, sES, sSS, sSR, sSA, sCL},/* 5.2.4 - Big TODO */ > -/* cookie_ack */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't come in orig dir */ > +/* cookie_ack */ {sCL, sCL, sCW, sES, sES, sSS, sSR, sSA, sCL},/* Can't come in orig dir */ > /* shutdown_comp*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sCL, sCL}, > /* heartbeat */ {sHS, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS}, > /* heartbeat_ack*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS}, > @@ -126,7 +126,7 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = { > /* shutdown */ {sIV, sCL, sCW, sCE, sSR, sSS, sSR, sSA, sIV}, > /* shutdown_ack */ {sIV, sCL, sCW, sCE, sES, sSA, sSA, sSA, sIV}, > /* error */ {sIV, sCL, sCW, sCL, sES, sSS, sSR, sSA, sIV}, > -/* cookie_echo */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sIV},/* Can't come in reply dir */ > +/* cookie_echo */ {sIV, sCL, sCE, sCE, sES, sSS, sSR, sSA, sIV},/* Can't come in reply dir */ > /* cookie_ack */ {sIV, sCL, sCW, sES, sES, sSS, sSR, sSA, sIV}, > /* shutdown_comp*/ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sCL, sIV}, > /* heartbeat */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS}, > @@ -412,6 +412,9 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, > /* (D) vtag must be same as init_vtag as found in INIT_ACK */ > if (sh->vtag != ct->proto.sctp.vtag[dir]) > goto out_unlock; > + } else if (sch->type == SCTP_CID_COOKIE_ACK) { > + ct->proto.sctp.init[dir] = 0; > + ct->proto.sctp.init[!dir] = 0; > } else if (sch->type == SCTP_CID_HEARTBEAT) { > if (ct->proto.sctp.vtag[dir] == 0) { > pr_debug("Setting %d vtag %x for dir %d\n", sch->type, sh->vtag, dir); > @@ -461,16 +464,18 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, > } > > /* If it is an INIT or an INIT ACK note down the vtag */ > - if (sch->type == SCTP_CID_INIT || > - sch->type == SCTP_CID_INIT_ACK) { > - struct sctp_inithdr _inithdr, *ih; > + if (sch->type == SCTP_CID_INIT) { > + struct sctp_inithdr _ih, *ih; > > - ih = skb_header_pointer(skb, offset + sizeof(_sch), > - sizeof(_inithdr), &_inithdr); > + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih); > if (ih == NULL) > goto out_unlock; > - pr_debug("Setting vtag %x for dir %d\n", > - ih->init_tag, !dir); > + > + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir]) > + ct->proto.sctp.init[!dir] = 0; > + ct->proto.sctp.init[dir] = 1; > + > + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir); > ct->proto.sctp.vtag[!dir] = ih->init_tag; > > /* don't renew timeout on init retransmit so > @@ -481,6 +486,24 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, > old_state == SCTP_CONNTRACK_CLOSED && > nf_ct_is_confirmed(ct)) > ignore = true; > + } else if (sch->type == SCTP_CID_INIT_ACK) { > + struct sctp_inithdr _ih, *ih; > + u32 vtag; > + > + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih); > + if (ih == NULL) > + goto out_unlock; > + > + vtag = ct->proto.sctp.vtag[!dir]; > + if (!ct->proto.sctp.init[!dir] && vtag && vtag != ih->init_tag) > + goto out_unlock; > + /* collision */ > + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] && > + vtag != ih->init_tag) > + goto out_unlock; > + > + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir); > + ct->proto.sctp.vtag[!dir] = ih->init_tag; > } > > ct->proto.sctp.state = new_state; > -- > 2.39.1 > a reproducer is attached. Thanks.
Xin Long <lucien.xin@gmail.com> wrote: > a reproducer is attached. > > Thanks. Do you think its worth it to turn this into a selftest?
On Mon, Oct 2, 2023 at 11:18 AM Florian Westphal <fw@strlen.de> wrote: > > Xin Long <lucien.xin@gmail.com> wrote: > > a reproducer is attached. > > > > Thanks. > > Do you think its worth it to turn this into a selftest? I think so, it's a typical SCTP collision scenario, if it's okay to you I'd like to add this to: tools/testing/selftests/netfilter/conntrack_sctp_collision.sh should I repost this netfilter patch together with this selftest or I can post this selftest later? Thanks.
Xin Long <lucien.xin@gmail.com> wrote: > On Mon, Oct 2, 2023 at 11:18 AM Florian Westphal <fw@strlen.de> wrote: > > > > Xin Long <lucien.xin@gmail.com> wrote: > > > a reproducer is attached. > > > > > > Thanks. > > > > Do you think its worth it to turn this into a selftest? > I think so, it's a typical SCTP collision scenario, if it's okay to you > I'd like to add this to: > > tools/testing/selftests/netfilter/conntrack_sctp_collision.sh LGTM, thanks! > should I repost this netfilter patch together with this selftest or I > can post this selftest later? Posting it later is fine.
On Sun, Oct 01, 2023 at 11:07:48AM -0400, Xin Long wrote: > In Scenario A and B below, as the delayed INIT_ACK always changes the peer > vtag, SCTP ct with the incorrect vtag may cause packet loss. > > Scenario A: INIT_ACK is delayed until the peer receives its own INIT_ACK > > 192.168.1.2 > 192.168.1.1: [INIT] [init tag: 1328086772] > 192.168.1.1 > 192.168.1.2: [INIT] [init tag: 1414468151] > 192.168.1.2 > 192.168.1.1: [INIT ACK] [init tag: 1328086772] > 192.168.1.1 > 192.168.1.2: [INIT ACK] [init tag: 1650211246] * > 192.168.1.2 > 192.168.1.1: [COOKIE ECHO] > 192.168.1.1 > 192.168.1.2: [COOKIE ECHO] > 192.168.1.2 > 192.168.1.1: [COOKIE ACK] > > Scenario B: INIT_ACK is delayed until the peer completes its own handshake > > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408] > 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885] > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT ACK] [init tag: 3922216408] > 192.168.1.1 > 192.168.1.2: sctp (1) [COOKIE ECHO] > 192.168.1.2 > 192.168.1.1: sctp (1) [COOKIE ACK] > 192.168.1.1 > 192.168.1.2: sctp (1) [INIT ACK] [init tag: 3914796021] * > > This patch fixes it as below: > > In SCTP_CID_INIT processing: > - clear ct->proto.sctp.init[!dir] if ct->proto.sctp.init[dir] && > ct->proto.sctp.init[!dir]. (Scenario E) > - set ct->proto.sctp.init[dir]. > > In SCTP_CID_INIT_ACK processing: > - drop it if !ct->proto.sctp.init[!dir] && ct->proto.sctp.vtag[!dir] && > ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario B, Scenario C) > - drop it if ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] && > ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario A) > > In SCTP_CID_COOKIE_ACK processing: > - clear ct->proto.sctp.init[dir] and ct->proto.sctp.init[!dir]. (Scenario D) > > Also, it's important to allow the ct state to move forward with cookie_echo > and cookie_ack from the opposite dir for the collision scenarios. > > There are also other Scenarios where it should allow the packet through, > addressed by the processing above: > > Scenario C: new CT is created by INIT_ACK. > > Scenario D: start INIT on the existing ESTABLISHED ct. > > Scenario E: start INIT after the old collision on the existing ESTABLISHED ct. > > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408] > 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885] > (both side are stopped, then start new connection again in hours) > 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 242308742] > > Signed-off-by: Xin Long <lucien.xin@gmail.com> Hi, as a fix I wonder if this warrants a Fixes tag. Perhaps our old friend: Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
On Sun, Oct 01, 2023 at 11:07:48AM -0400, Xin Long wrote: ... > @@ -481,6 +486,24 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, > old_state == SCTP_CONNTRACK_CLOSED && > nf_ct_is_confirmed(ct)) > ignore = true; > + } else if (sch->type == SCTP_CID_INIT_ACK) { > + struct sctp_inithdr _ih, *ih; > + u32 vtag; > + > + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih); > + if (ih == NULL) > + goto out_unlock; > + > + vtag = ct->proto.sctp.vtag[!dir]; > + if (!ct->proto.sctp.init[!dir] && vtag && vtag != ih->init_tag) > + goto out_unlock; > + /* collision */ > + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] && > + vtag != ih->init_tag) The type of vtag is u32. But the type of ct->proto.sctp.vtag[!dir] and init_tag is __be32. This doesn't seem right (and makes Sparse unhappy). > + goto out_unlock; > + > + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir); > + ct->proto.sctp.vtag[!dir] = ih->init_tag; > } > > ct->proto.sctp.state = new_state; > -- > 2.39.1 > >
On Tue, Oct 3, 2023 at 8:51 AM Simon Horman <horms@kernel.org> wrote: > > On Sun, Oct 01, 2023 at 11:07:48AM -0400, Xin Long wrote: > > ... > > > @@ -481,6 +486,24 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, > > old_state == SCTP_CONNTRACK_CLOSED && > > nf_ct_is_confirmed(ct)) > > ignore = true; > > + } else if (sch->type == SCTP_CID_INIT_ACK) { > > + struct sctp_inithdr _ih, *ih; > > + u32 vtag; > > + > > + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih); > > + if (ih == NULL) > > + goto out_unlock; > > + > > + vtag = ct->proto.sctp.vtag[!dir]; > > + if (!ct->proto.sctp.init[!dir] && vtag && vtag != ih->init_tag) > > + goto out_unlock; > > + /* collision */ > > + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] && > > + vtag != ih->init_tag) > > The type of vtag is u32. But the type of ct->proto.sctp.vtag[!dir] and init_tag > is __be32. This doesn't seem right (and makes Sparse unhappy). You're right, I will fix it and re-post with tag: Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.") Thanks. > > > + goto out_unlock; > > + > > + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir); > > + ct->proto.sctp.vtag[!dir] = ih->init_tag; > > } > > > > ct->proto.sctp.state = new_state; > > -- > > 2.39.1 > > > >
Xin Long <lucien.xin@gmail.com> wrote: > > The type of vtag is u32. But the type of ct->proto.sctp.vtag[!dir] and init_tag > > is __be32. This doesn't seem right (and makes Sparse unhappy). > You're right, I will fix it and re-post with tag: > > Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.") I'm fine with this, the bug is likely inherited from ipt_conntrack_sctp.c, but that doesn't exist anymore. Would you also fix up the __be32/u32 confusion? Better to not add more sparse warnings... Thanks!
On Tue, Oct 3, 2023 at 10:23 AM Florian Westphal <fw@strlen.de> wrote: > > Xin Long <lucien.xin@gmail.com> wrote: > > > The type of vtag is u32. But the type of ct->proto.sctp.vtag[!dir] and init_tag > > > is __be32. This doesn't seem right (and makes Sparse unhappy). > > You're right, I will fix it and re-post with tag: > > > > Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.") > > I'm fine with this, the bug is likely inherited from > ipt_conntrack_sctp.c, but that doesn't exist anymore. ah, I see. > > Would you also fix up the __be32/u32 confusion? > > Better to not add more sparse warnings... > yes, I will fix the __be32 one too. Thanks.
diff --git a/include/linux/netfilter/nf_conntrack_sctp.h b/include/linux/netfilter/nf_conntrack_sctp.h index 625f491b95de..fb31312825ae 100644 --- a/include/linux/netfilter/nf_conntrack_sctp.h +++ b/include/linux/netfilter/nf_conntrack_sctp.h @@ -9,6 +9,7 @@ struct ip_ct_sctp { enum sctp_conntrack state; __be32 vtag[IP_CT_DIR_MAX]; + u8 init[IP_CT_DIR_MAX]; u8 last_dir; u8 flags; }; diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c index b6bcc8f2f46b..91aee286d503 100644 --- a/net/netfilter/nf_conntrack_proto_sctp.c +++ b/net/netfilter/nf_conntrack_proto_sctp.c @@ -112,7 +112,7 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = { /* shutdown_ack */ {sSA, sCL, sCW, sCE, sES, sSA, sSA, sSA, sSA}, /* error */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't have Stale cookie*/ /* cookie_echo */ {sCL, sCL, sCE, sCE, sES, sSS, sSR, sSA, sCL},/* 5.2.4 - Big TODO */ -/* cookie_ack */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't come in orig dir */ +/* cookie_ack */ {sCL, sCL, sCW, sES, sES, sSS, sSR, sSA, sCL},/* Can't come in orig dir */ /* shutdown_comp*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sCL, sCL}, /* heartbeat */ {sHS, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS}, /* heartbeat_ack*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS}, @@ -126,7 +126,7 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = { /* shutdown */ {sIV, sCL, sCW, sCE, sSR, sSS, sSR, sSA, sIV}, /* shutdown_ack */ {sIV, sCL, sCW, sCE, sES, sSA, sSA, sSA, sIV}, /* error */ {sIV, sCL, sCW, sCL, sES, sSS, sSR, sSA, sIV}, -/* cookie_echo */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sIV},/* Can't come in reply dir */ +/* cookie_echo */ {sIV, sCL, sCE, sCE, sES, sSS, sSR, sSA, sIV},/* Can't come in reply dir */ /* cookie_ack */ {sIV, sCL, sCW, sES, sES, sSS, sSR, sSA, sIV}, /* shutdown_comp*/ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sCL, sIV}, /* heartbeat */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS}, @@ -412,6 +412,9 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, /* (D) vtag must be same as init_vtag as found in INIT_ACK */ if (sh->vtag != ct->proto.sctp.vtag[dir]) goto out_unlock; + } else if (sch->type == SCTP_CID_COOKIE_ACK) { + ct->proto.sctp.init[dir] = 0; + ct->proto.sctp.init[!dir] = 0; } else if (sch->type == SCTP_CID_HEARTBEAT) { if (ct->proto.sctp.vtag[dir] == 0) { pr_debug("Setting %d vtag %x for dir %d\n", sch->type, sh->vtag, dir); @@ -461,16 +464,18 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, } /* If it is an INIT or an INIT ACK note down the vtag */ - if (sch->type == SCTP_CID_INIT || - sch->type == SCTP_CID_INIT_ACK) { - struct sctp_inithdr _inithdr, *ih; + if (sch->type == SCTP_CID_INIT) { + struct sctp_inithdr _ih, *ih; - ih = skb_header_pointer(skb, offset + sizeof(_sch), - sizeof(_inithdr), &_inithdr); + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih); if (ih == NULL) goto out_unlock; - pr_debug("Setting vtag %x for dir %d\n", - ih->init_tag, !dir); + + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir]) + ct->proto.sctp.init[!dir] = 0; + ct->proto.sctp.init[dir] = 1; + + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir); ct->proto.sctp.vtag[!dir] = ih->init_tag; /* don't renew timeout on init retransmit so @@ -481,6 +486,24 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct, old_state == SCTP_CONNTRACK_CLOSED && nf_ct_is_confirmed(ct)) ignore = true; + } else if (sch->type == SCTP_CID_INIT_ACK) { + struct sctp_inithdr _ih, *ih; + u32 vtag; + + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih); + if (ih == NULL) + goto out_unlock; + + vtag = ct->proto.sctp.vtag[!dir]; + if (!ct->proto.sctp.init[!dir] && vtag && vtag != ih->init_tag) + goto out_unlock; + /* collision */ + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] && + vtag != ih->init_tag) + goto out_unlock; + + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir); + ct->proto.sctp.vtag[!dir] = ih->init_tag; } ct->proto.sctp.state = new_state;
In Scenario A and B below, as the delayed INIT_ACK always changes the peer vtag, SCTP ct with the incorrect vtag may cause packet loss. Scenario A: INIT_ACK is delayed until the peer receives its own INIT_ACK 192.168.1.2 > 192.168.1.1: [INIT] [init tag: 1328086772] 192.168.1.1 > 192.168.1.2: [INIT] [init tag: 1414468151] 192.168.1.2 > 192.168.1.1: [INIT ACK] [init tag: 1328086772] 192.168.1.1 > 192.168.1.2: [INIT ACK] [init tag: 1650211246] * 192.168.1.2 > 192.168.1.1: [COOKIE ECHO] 192.168.1.1 > 192.168.1.2: [COOKIE ECHO] 192.168.1.2 > 192.168.1.1: [COOKIE ACK] Scenario B: INIT_ACK is delayed until the peer completes its own handshake 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408] 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885] 192.168.1.2 > 192.168.1.1: sctp (1) [INIT ACK] [init tag: 3922216408] 192.168.1.1 > 192.168.1.2: sctp (1) [COOKIE ECHO] 192.168.1.2 > 192.168.1.1: sctp (1) [COOKIE ACK] 192.168.1.1 > 192.168.1.2: sctp (1) [INIT ACK] [init tag: 3914796021] * This patch fixes it as below: In SCTP_CID_INIT processing: - clear ct->proto.sctp.init[!dir] if ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir]. (Scenario E) - set ct->proto.sctp.init[dir]. In SCTP_CID_INIT_ACK processing: - drop it if !ct->proto.sctp.init[!dir] && ct->proto.sctp.vtag[!dir] && ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario B, Scenario C) - drop it if ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] && ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario A) In SCTP_CID_COOKIE_ACK processing: - clear ct->proto.sctp.init[dir] and ct->proto.sctp.init[!dir]. (Scenario D) Also, it's important to allow the ct state to move forward with cookie_echo and cookie_ack from the opposite dir for the collision scenarios. There are also other Scenarios where it should allow the packet through, addressed by the processing above: Scenario C: new CT is created by INIT_ACK. Scenario D: start INIT on the existing ESTABLISHED ct. Scenario E: start INIT after the old collision on the existing ESTABLISHED ct. 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408] 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885] (both side are stopped, then start new connection again in hours) 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 242308742] Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/linux/netfilter/nf_conntrack_sctp.h | 1 + net/netfilter/nf_conntrack_proto_sctp.c | 41 ++++++++++++++++----- 2 files changed, 33 insertions(+), 9 deletions(-)