diff mbox series

[ovs-dev] conntrack: Allow flush of SCTP protocol

Message ID 20230802094010.111208-1-amusil@redhat.com
State Changes Requested
Headers show
Series [ovs-dev] conntrack: Allow flush of SCTP protocol | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/intel-ovs-compilation success test: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Ales Musil Aug. 2, 2023, 9:40 a.m. UTC
The SCTP protocol ports were excluded from
the netlink encoding. Which resulted in the
lookup failure in kernel, leading to the entry
not being flushed. Allow the flush of SCTP protocol
based on port numbers.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
 lib/netlink-conntrack.c |  3 ++-
 tests/system-traffic.at | 26 ++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 5 deletions(-)

Comments

Simon Horman Aug. 4, 2023, 3:21 p.m. UTC | #1
On Wed, Aug 02, 2023 at 11:40:10AM +0200, Ales Musil wrote:
> The SCTP protocol ports were excluded from
> the netlink encoding. Which resulted in the
> lookup failure in kernel, leading to the entry
> not being flushed. Allow the flush of SCTP protocol
> based on port numbers.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>

Acked-by: Simon Horman <horms@ovn.org>
Aaron Conole Aug. 28, 2023, 2:20 p.m. UTC | #2
Ales Musil <amusil@redhat.com> writes:

> The SCTP protocol ports were excluded from
> the netlink encoding. Which resulted in the
> lookup failure in kernel, leading to the entry
> not being flushed. Allow the flush of SCTP protocol
> based on port numbers.
>
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

>  lib/netlink-conntrack.c |  3 ++-
>  tests/system-traffic.at | 26 ++++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 4fcde9ba1..492bfcffb 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> @@ -579,7 +579,8 @@ nl_ct_put_tuple_proto(struct ofpbuf *buf, const struct ct_dpif_tuple *tuple)
>          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_TYPE, tuple->icmp_type);
>          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_CODE, tuple->icmp_code);
>      } else if (tuple->ip_proto == IPPROTO_TCP ||
> -               tuple->ip_proto == IPPROTO_UDP) {
> +               tuple->ip_proto == IPPROTO_UDP ||
> +               tuple->ip_proto == IPPROTO_SCTP) {
>          nl_msg_put_be16(buf, CTA_PROTO_SRC_PORT, tuple->src_port);
>          nl_msg_put_be16(buf, CTA_PROTO_DST_PORT, tuple->dst_port);
>      } else {
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 945037ec0..78e2f9ab9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2516,6 +2516,7 @@ AT_CLEANUP
>  
>  AT_SETUP([conntrack - ct flush])
>  CHECK_CONNTRACK()
> +CHECK_CONNTRACK_SCTP()
>  OVS_TRAFFIC_VSWITCHD_START()
>  
>  ADD_NAMESPACES(at_ns0, at_ns1)
> @@ -2526,10 +2527,8 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>  AT_DATA([flows.txt], [dnl
>  priority=1,action=drop
>  priority=10,arp,action=normal
> -priority=100,in_port=1,udp,action=ct(commit),2
> -priority=100,in_port=2,udp,action=ct(zone=5,commit),1
> -priority=100,in_port=1,icmp,action=ct(commit),2
> -priority=100,in_port=2,icmp,action=ct(zone=5,commit),1
> +priority=100,in_port=1,ip,action=ct(commit),2
> +priority=100,in_port=2,ip,action=ct(zone=5,commit),1
>  ])
>  
>  AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> @@ -2692,6 +2691,25 @@ udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.
>  
>  AT_CHECK([FLUSH_CMD])
>  
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
> +
> +dnl Test SCTP flush based on port
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500003400010000408464410a0101010a01010200010002000000009178f7d30100001470e18ccc00000000000a000a00000000 actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000950540000000a08004500003400010000408464410a0101020a010101000200010000000098f29e470100001470e18ccc00000000000a000a00000000 actions=resubmit(,0)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed "s/,protoinfo=.*$//" | sort], [0], [dnl
> +sctp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> +])
> +
> +AT_CHECK([FLUSH_CMD 'ct_nw_src=10.1.1.1,ct_nw_proto=132,ct_tp_src=1,ct_tp_dst=2'])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed "s/,protoinfo=.*$//" | sort], [0], [dnl
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> +])
> +
> +AT_CHECK([FLUSH_CMD 'ct_nw_src=10.1.1.2,ct_nw_proto=132,ct_tp_src=2,ct_tp_dst=1'])
> +
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
>  ])
Ales Musil Aug. 29, 2023, 5:19 a.m. UTC | #3
On Wed, Aug 2, 2023 at 11:40 AM Ales Musil <amusil@redhat.com> wrote:

> The SCTP protocol ports were excluded from
> the netlink encoding. Which resulted in the
> lookup failure in kernel, leading to the entry
> not being flushed. Allow the flush of SCTP protocol
> based on port numbers.
>
>
I forgot to add "Reported-at: https://bugzilla.redhat.com/2228037".  Please
append it during merge.


> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
>
 lib/netlink-conntrack.c |  3 ++-
>  tests/system-traffic.at | 26 ++++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 4fcde9ba1..492bfcffb 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> @@ -579,7 +579,8 @@ nl_ct_put_tuple_proto(struct ofpbuf *buf, const struct
> ct_dpif_tuple *tuple)
>          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_TYPE, tuple->icmp_type);
>          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_CODE, tuple->icmp_code);
>      } else if (tuple->ip_proto == IPPROTO_TCP ||
> -               tuple->ip_proto == IPPROTO_UDP) {
> +               tuple->ip_proto == IPPROTO_UDP ||
> +               tuple->ip_proto == IPPROTO_SCTP) {
>          nl_msg_put_be16(buf, CTA_PROTO_SRC_PORT, tuple->src_port);
>          nl_msg_put_be16(buf, CTA_PROTO_DST_PORT, tuple->dst_port);
>      } else {
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 945037ec0..78e2f9ab9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2516,6 +2516,7 @@ AT_CLEANUP
>
>  AT_SETUP([conntrack - ct flush])
>  CHECK_CONNTRACK()
> +CHECK_CONNTRACK_SCTP()
>  OVS_TRAFFIC_VSWITCHD_START()
>
>  ADD_NAMESPACES(at_ns0, at_ns1)
> @@ -2526,10 +2527,8 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>  AT_DATA([flows.txt], [dnl
>  priority=1,action=drop
>  priority=10,arp,action=normal
> -priority=100,in_port=1,udp,action=ct(commit),2
> -priority=100,in_port=2,udp,action=ct(zone=5,commit),1
> -priority=100,in_port=1,icmp,action=ct(commit),2
> -priority=100,in_port=2,icmp,action=ct(zone=5,commit),1
> +priority=100,in_port=1,ip,action=ct(commit),2
> +priority=100,in_port=2,ip,action=ct(zone=5,commit),1
>  ])
>
>  AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> @@ -2692,6 +2691,25 @@
> udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.
>
>  AT_CHECK([FLUSH_CMD])
>
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
> +
> +dnl Test SCTP flush based on port
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1
> packet=50540000000a50540000000908004500003400010000408464410a0101010a01010200010002000000009178f7d30100001470e18ccc00000000000a000a00000000
> actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000950540000000a08004500003400010000408464410a0101020a010101000200010000000098f29e470100001470e18ccc00000000000a000a00000000
> actions=resubmit(,0)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed
> "s/,protoinfo=.*$//" | sort], [0], [dnl
>
> +sctp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
>
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> +])
> +
> +AT_CHECK([FLUSH_CMD
> 'ct_nw_src=10.1.1.1,ct_nw_proto=132,ct_tp_src=1,ct_tp_dst=2'])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed
> "s/,protoinfo=.*$//" | sort], [0], [dnl
>
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> +])
> +
> +AT_CHECK([FLUSH_CMD
> 'ct_nw_src=10.1.1.2,ct_nw_proto=132,ct_tp_src=2,ct_tp_dst=1'])
> +
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
>  ])
>
> --
> 2.41.0
>
>
Thanks,
Ales
Ilya Maximets Sept. 1, 2023, 7:38 p.m. UTC | #4
Hi, Ales.  Thanks for the patch!

A few issues though I see with it.  Not with the code, but with a commit
message and positioning of the change.  See below.

Nit: 'conntrack' area in the subject usually means conntrack.c,  which is
a userspace conntrack inplementation, or maybe the common conntrack changes
that applies to either implementation.  'netlink-conntrack' makes more sense
in the current case.

On 8/2/23 11:40, Ales Musil wrote:
> The SCTP protocol ports were excluded from
> the netlink encoding. Which resulted in the
> lookup failure in kernel,

This is not true.  The request is never sent to the kernel, ovs-vswitchd
decides that it can't encode the request and fails the operation.

> leading to the entry
> not being flushed. Allow the flush of SCTP protocol
> based on port numbers.

While this is true, this description is placing the change into a
'new feature' category and it made me spend a significant amount of time
thinking if it should be backported.  It also doesn't show a full picture.

I think, there is a stronger argument for this change to be a bug fix
rather than a feature.  Mainly because SCTP entries in the kernel conntrack
will prevent OVS from flushing other types of entries.  

For example, if we run the following command while having both SCTP and
TCP conntrack entries with the same source IP in the kernel:

  ovs-ofctl ct-flush br0 'ct_nw_src=10.1.1.1'

the ct_dpif_flush_tuple() function will iterate over each entry and try to
delete it, if the source address matches, but as soon as it will find the
first matching SCTP entry, it will exit with failure leaving remaining
TCP entries in the kernel.  And it will not be possible to clean up these
dangling TCP entries without executing a full conntrack flush.
This is an issue on its own, ct_dpif_flush_tuple() probably shouldn't
bail on first failure, hard to tell.  But adding support for SCTP will
fix that issue for now, until a new protocol support is added to the kernel.

N.b: A test for this use-case would be nice to have, if possible.

So, it might make sense to focus the commit message on this issue and the
fact that conntrack implementation in OVS generally supports SCTP except
for this particular use case.

One more nit below.

Best regards, Ilya Maximets.

> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
>  lib/netlink-conntrack.c |  3 ++-
>  tests/system-traffic.at | 26 ++++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 4fcde9ba1..492bfcffb 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> @@ -579,7 +579,8 @@ nl_ct_put_tuple_proto(struct ofpbuf *buf, const struct ct_dpif_tuple *tuple)
>          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_TYPE, tuple->icmp_type);
>          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_CODE, tuple->icmp_code);
>      } else if (tuple->ip_proto == IPPROTO_TCP ||
> -               tuple->ip_proto == IPPROTO_UDP) {
> +               tuple->ip_proto == IPPROTO_UDP ||
> +               tuple->ip_proto == IPPROTO_SCTP) {
>          nl_msg_put_be16(buf, CTA_PROTO_SRC_PORT, tuple->src_port);
>          nl_msg_put_be16(buf, CTA_PROTO_DST_PORT, tuple->dst_port);
>      } else {
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 945037ec0..78e2f9ab9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2516,6 +2516,7 @@ AT_CLEANUP
>  
>  AT_SETUP([conntrack - ct flush])
>  CHECK_CONNTRACK()
> +CHECK_CONNTRACK_SCTP()
>  OVS_TRAFFIC_VSWITCHD_START()
>  
>  ADD_NAMESPACES(at_ns0, at_ns1)
> @@ -2526,10 +2527,8 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>  AT_DATA([flows.txt], [dnl
>  priority=1,action=drop
>  priority=10,arp,action=normal
> -priority=100,in_port=1,udp,action=ct(commit),2
> -priority=100,in_port=2,udp,action=ct(zone=5,commit),1
> -priority=100,in_port=1,icmp,action=ct(commit),2
> -priority=100,in_port=2,icmp,action=ct(zone=5,commit),1
> +priority=100,in_port=1,ip,action=ct(commit),2
> +priority=100,in_port=2,ip,action=ct(zone=5,commit),1
>  ])
>  
>  AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> @@ -2692,6 +2691,25 @@ udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.
>  
>  AT_CHECK([FLUSH_CMD])
>  
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
> +
> +dnl Test SCTP flush based on port

Nit: A period at the end of a sentence.

> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500003400010000408464410a0101010a01010200010002000000009178f7d30100001470e18ccc00000000000a000a00000000 actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000950540000000a08004500003400010000408464410a0101020a010101000200010000000098f29e470100001470e18ccc00000000000a000a00000000 actions=resubmit(,0)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed "s/,protoinfo=.*$//" | sort], [0], [dnl
> +sctp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> +])
> +
> +AT_CHECK([FLUSH_CMD 'ct_nw_src=10.1.1.1,ct_nw_proto=132,ct_tp_src=1,ct_tp_dst=2'])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed "s/,protoinfo=.*$//" | sort], [0], [dnl
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> +])
> +
> +AT_CHECK([FLUSH_CMD 'ct_nw_src=10.1.1.2,ct_nw_proto=132,ct_tp_src=2,ct_tp_dst=1'])
> +
>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
>  ])
>
Ales Musil Sept. 4, 2023, 12:48 p.m. UTC | #5
On Fri, Sep 1, 2023 at 9:38 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> Hi, Ales.  Thanks for the patch!
>
> A few issues though I see with it.  Not with the code, but with a commit
> message and positioning of the change.  See below.
>
> Nit: 'conntrack' area in the subject usually means conntrack.c,  which is
> a userspace conntrack inplementation, or maybe the common conntrack changes
> that applies to either implementation.  'netlink-conntrack' makes more
> sense
> in the current case.
>
> On 8/2/23 11:40, Ales Musil wrote:
> > The SCTP protocol ports were excluded from
> > the netlink encoding. Which resulted in the
> > lookup failure in kernel,
>
> This is not true.  The request is never sent to the kernel, ovs-vswitchd
> decides that it can't encode the request and fails the operation.
>
> > leading to the entry
> > not being flushed. Allow the flush of SCTP protocol
> > based on port numbers.
>
> While this is true, this description is placing the change into a
> 'new feature' category and it made me spend a significant amount of time
> thinking if it should be backported.  It also doesn't show a full picture.
>
> I think, there is a stronger argument for this change to be a bug fix
> rather than a feature.  Mainly because SCTP entries in the kernel conntrack
> will prevent OVS from flushing other types of entries.
>
> For example, if we run the following command while having both SCTP and
> TCP conntrack entries with the same source IP in the kernel:
>
>   ovs-ofctl ct-flush br0 'ct_nw_src=10.1.1.1'
>
> the ct_dpif_flush_tuple() function will iterate over each entry and try to
> delete it, if the source address matches, but as soon as it will find the
> first matching SCTP entry, it will exit with failure leaving remaining
> TCP entries in the kernel.  And it will not be possible to clean up these
> dangling TCP entries without executing a full conntrack flush.
> This is an issue on its own, ct_dpif_flush_tuple() probably shouldn't
> bail on first failure, hard to tell.  But adding support for SCTP will
> fix that issue for now, until a new protocol support is added to the
> kernel.
>
> N.b: A test for this use-case would be nice to have, if possible.
>
> So, it might make sense to focus the commit message on this issue and the
> fact that conntrack implementation in OVS generally supports SCTP except
> for this particular use case.
>
> One more nit below.
>
> Best regards, Ilya Maximets.
>
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > ---
> >  lib/netlink-conntrack.c |  3 ++-
> >  tests/system-traffic.at | 26 ++++++++++++++++++++++----
> >  2 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> > index 4fcde9ba1..492bfcffb 100644
> > --- a/lib/netlink-conntrack.c
> > +++ b/lib/netlink-conntrack.c
> > @@ -579,7 +579,8 @@ nl_ct_put_tuple_proto(struct ofpbuf *buf, const
> struct ct_dpif_tuple *tuple)
> >          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_TYPE, tuple->icmp_type);
> >          nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_CODE, tuple->icmp_code);
> >      } else if (tuple->ip_proto == IPPROTO_TCP ||
> > -               tuple->ip_proto == IPPROTO_UDP) {
> > +               tuple->ip_proto == IPPROTO_UDP ||
> > +               tuple->ip_proto == IPPROTO_SCTP) {
> >          nl_msg_put_be16(buf, CTA_PROTO_SRC_PORT, tuple->src_port);
> >          nl_msg_put_be16(buf, CTA_PROTO_DST_PORT, tuple->dst_port);
> >      } else {
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 945037ec0..78e2f9ab9 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -2516,6 +2516,7 @@ AT_CLEANUP
> >
> >  AT_SETUP([conntrack - ct flush])
> >  CHECK_CONNTRACK()
> > +CHECK_CONNTRACK_SCTP()
> >  OVS_TRAFFIC_VSWITCHD_START()
> >
> >  ADD_NAMESPACES(at_ns0, at_ns1)
> > @@ -2526,10 +2527,8 @@ ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> >  AT_DATA([flows.txt], [dnl
> >  priority=1,action=drop
> >  priority=10,arp,action=normal
> > -priority=100,in_port=1,udp,action=ct(commit),2
> > -priority=100,in_port=2,udp,action=ct(zone=5,commit),1
> > -priority=100,in_port=1,icmp,action=ct(commit),2
> > -priority=100,in_port=2,icmp,action=ct(zone=5,commit),1
> > +priority=100,in_port=1,ip,action=ct(commit),2
> > +priority=100,in_port=2,ip,action=ct(zone=5,commit),1
> >  ])
> >
> >  AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> > @@ -2692,6 +2691,25 @@
> udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.
> >
> >  AT_CHECK([FLUSH_CMD])
> >
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
> > +
> > +dnl Test SCTP flush based on port
>
> Nit: A period at the end of a sentence.
>
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1
> packet=50540000000a50540000000908004500003400010000408464410a0101010a01010200010002000000009178f7d30100001470e18ccc00000000000a000a00000000
> actions=resubmit(,0)"])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000950540000000a08004500003400010000408464410a0101020a010101000200010000000098f29e470100001470e18ccc00000000000a000a00000000
> actions=resubmit(,0)"])
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed
> "s/,protoinfo=.*$//" | sort], [0], [dnl
> >
> +sctp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
> >
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> > +])
> > +
> > +AT_CHECK([FLUSH_CMD
> 'ct_nw_src=10.1.1.1,ct_nw_proto=132,ct_tp_src=1,ct_tp_dst=2'])
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed
> "s/,protoinfo=.*$//" | sort], [0], [dnl
> >
> +sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
> > +])
> > +
> > +AT_CHECK([FLUSH_CMD
> 'ct_nw_src=10.1.1.2,ct_nw_proto=132,ct_tp_src=2,ct_tp_dst=1'])
> > +
> >  AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
> >  ])
> >
>
>

Hi Ilya,

thank you for the review. It makes sense I'll rework the commit message and
send it in v2. I'll try to make a test for that behavior, but I'm afraid it
won't work that well. It has huge potential to be flaky as it is highly
dependent on the order CT entries that we receive from kernel/userspace.

Thanks,
Ales
diff mbox series

Patch

diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
index 4fcde9ba1..492bfcffb 100644
--- a/lib/netlink-conntrack.c
+++ b/lib/netlink-conntrack.c
@@ -579,7 +579,8 @@  nl_ct_put_tuple_proto(struct ofpbuf *buf, const struct ct_dpif_tuple *tuple)
         nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_TYPE, tuple->icmp_type);
         nl_msg_put_u8(buf, CTA_PROTO_ICMPV6_CODE, tuple->icmp_code);
     } else if (tuple->ip_proto == IPPROTO_TCP ||
-               tuple->ip_proto == IPPROTO_UDP) {
+               tuple->ip_proto == IPPROTO_UDP ||
+               tuple->ip_proto == IPPROTO_SCTP) {
         nl_msg_put_be16(buf, CTA_PROTO_SRC_PORT, tuple->src_port);
         nl_msg_put_be16(buf, CTA_PROTO_DST_PORT, tuple->dst_port);
     } else {
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 945037ec0..78e2f9ab9 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2516,6 +2516,7 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - ct flush])
 CHECK_CONNTRACK()
+CHECK_CONNTRACK_SCTP()
 OVS_TRAFFIC_VSWITCHD_START()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
@@ -2526,10 +2527,8 @@  ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
 AT_DATA([flows.txt], [dnl
 priority=1,action=drop
 priority=10,arp,action=normal
-priority=100,in_port=1,udp,action=ct(commit),2
-priority=100,in_port=2,udp,action=ct(zone=5,commit),1
-priority=100,in_port=1,icmp,action=ct(commit),2
-priority=100,in_port=2,icmp,action=ct(zone=5,commit),1
+priority=100,in_port=1,ip,action=ct(commit),2
+priority=100,in_port=2,ip,action=ct(zone=5,commit),1
 ])
 
 AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
@@ -2692,6 +2691,25 @@  udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.
 
 AT_CHECK([FLUSH_CMD])
 
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
+
+dnl Test SCTP flush based on port
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500003400010000408464410a0101010a01010200010002000000009178f7d30100001470e18ccc00000000000a000a00000000 actions=resubmit(,0)"])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000950540000000a08004500003400010000408464410a0101020a010101000200010000000098f29e470100001470e18ccc00000000000a000a00000000 actions=resubmit(,0)"])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed "s/,protoinfo=.*$//" | sort], [0], [dnl
+sctp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
+sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
+])
+
+AT_CHECK([FLUSH_CMD 'ct_nw_src=10.1.1.1,ct_nw_proto=132,ct_tp_src=1,ct_tp_dst=2'])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1" | sed "s/,protoinfo=.*$//" | sort], [0], [dnl
+sctp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),zone=5
+])
+
+AT_CHECK([FLUSH_CMD 'ct_nw_src=10.1.1.2,ct_nw_proto=132,ct_tp_src=2,ct_tp_dst=1'])
+
 AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "10\.1\.1\.1"], [1])
 ])