diff mbox series

[ovs-dev,v2] netlink-conntrack: Fix partial match of entries with SCTP

Message ID 20230904140907.288022-1-amusil@redhat.com
State Accepted
Commit bac34b26a72161a686d432022364f3e9db94f385
Headers show
Series [ovs-dev,v2] netlink-conntrack: Fix partial match of entries with SCTP | expand

Checks

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

Commit Message

Ales Musil Sept. 4, 2023, 2:09 p.m. UTC 
The SCTP protocol ports were excluded from the netlink
encoding. In that case the nl_ct_flush_tuple() would
return EOPNOTSUPP, that could result in some CT entries
not being properly flushed if we would hit SCTP entry earlier
than others.

This at the same time allows to flush SCTP on its own
in during partial match. This should still be considered
a bug, because OvS currently supports SCTP CT entries,
and it should also support partial flush for them the same
way it supports partial flush for TCP/UDP.

Reported-at: https://bugzilla.redhat.com/2228037
Signed-off-by: Ales Musil <amusil@redhat.com>
---
v2: - Rebase on top of master.
    - Update commit message based on Ilya's comments.
    - Add missing period in the tests.
---
 lib/netlink-conntrack.c |  3 ++-
 tests/system-traffic.at | 26 ++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 5 deletions(-)

Comments

Ilya Maximets Sept. 9, 2023, 1:08 a.m. UTC | #1 
On 9/4/23 16:09, Ales Musil wrote:
> The SCTP protocol ports were excluded from the netlink
> encoding. In that case the nl_ct_flush_tuple() would
> return EOPNOTSUPP, that could result in some CT entries
> not being properly flushed if we would hit SCTP entry earlier
> than others.
> 
> This at the same time allows to flush SCTP on its own
> in during partial match. This should still be considered
> a bug, because OvS currently supports SCTP CT entries,
> and it should also support partial flush for them the same
> way it supports partial flush for TCP/UDP.
> 
> Reported-at: https://bugzilla.redhat.com/2228037
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
> v2: - Rebase on top of master.
>     - Update commit message based on Ilya's comments.
>     - Add missing period in the tests.
> ---
>  lib/netlink-conntrack.c |  3 ++-
>  tests/system-traffic.at | 26 ++++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 5 deletions(-)

Thanks!  Applied and backported down to 3.1.

Best regards, Ilya Maximets.
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 808c492a2..418cd32fe 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])
 ])