On Fri, Sep 1, 2023 at 9:38 PM Ilya Maximets <[email protected]> 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 <[email protected]> > > --- > > 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 -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
