| Message ID | 20240313110856.1010325-1-pvalerio@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] conntrack: Fix SNAT with exhaustion system test. | expand |
| 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 |
On 3/13/24 12:08, Paolo Valerio wrote: > Recent kernels introduced a mechanism that allows to evict colliding > entries in a closing state whereas they were previously considered as > parts of a non-recoverable clash. > This new behavior makes "conntrack - SNAT with port range with > exhaustion test" fail, as it relies on the previous assumptions. > > Fix it by creating and not advancing the first entry in SYN_SENT to > avoid early eviction. > > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Reported-at: https://issues.redhat.com/browse/FDP-486 > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- Hi, Paolo. Thanks for the fix! Some small comments inline. > tests/system-traffic.at | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 2d12d558e..04559f5e8 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -6388,7 +6388,6 @@ OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > AT_SETUP([conntrack - SNAT with port range with exhaustion]) > -OVS_CHECK_GITHUB_ACTION() > CHECK_CONNTRACK() > CHECK_CONNTRACK_NAT() > OVS_TRAFFIC_VSWITCHD_START() > @@ -6398,11 +6397,11 @@ ADD_NAMESPACES(at_ns0, at_ns1) > ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address 80:88:88:88:88:88]) > ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > +NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address 80:89:89:89:89:89]) > > dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0. > AT_DATA([flows.txt], [dnl > -in_port=1,tcp,action=ct(commit,zone=1,nat(src=10.1.1.240:34568,random)),2 > -in_port=2,ct_state=-trk,tcp,tp_dst=34567,action=ct(table=0,zone=1,nat) Do you know why this flow was there in the first place? > +in_port=1,tcp,action=ct(commit,zone=1,nat(src=10.1.1.240:34568)),2 > in_port=2,ct_state=-trk,tcp,tp_dst=34568,action=ct(table=0,zone=1,nat) > in_port=2,ct_state=+trk,ct_zone=1,tcp,action=1 > dnl > @@ -6426,17 +6425,25 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) > > dnl HTTP requests from p0->p1 should work fine. > OVS_START_L7([at_ns1], [http]) > -NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 1 -T 1 --retry-connrefused -v -o wget0.log]) > + > +dnl Send a valid SYN to make conntrack pick it up. > +dnl The source port used is 123 to prevent unwanted reuse in the next HTTP request. > +AT_CHECK([ovs-ofctl packet-out br0 "packet=80898989898980888888888808004500002800010000400664cb0a0101010a010102007b005000000000000000005002200079130000 actions=ct(commit,zone=1,nat(src=10.1.1.240:34568))"]) Can we use 'ovs-ofctl compose-packet --bare' instead of open-coding bytes? Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 3/13/24 12:08, Paolo Valerio wrote: >> Recent kernels introduced a mechanism that allows to evict colliding >> entries in a closing state whereas they were previously considered as >> parts of a non-recoverable clash. >> This new behavior makes "conntrack - SNAT with port range with >> exhaustion test" fail, as it relies on the previous assumptions. >> >> Fix it by creating and not advancing the first entry in SYN_SENT to >> avoid early eviction. >> >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >> Reported-at: https://issues.redhat.com/browse/FDP-486 >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- > > Hi, Paolo. Thanks for the fix! > Hi Ilya, Thanks for the feedback! > Some small comments inline. > >> tests/system-traffic.at | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index 2d12d558e..04559f5e8 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -6388,7 +6388,6 @@ OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> AT_SETUP([conntrack - SNAT with port range with exhaustion]) >> -OVS_CHECK_GITHUB_ACTION() >> CHECK_CONNTRACK() >> CHECK_CONNTRACK_NAT() >> OVS_TRAFFIC_VSWITCHD_START() >> @@ -6398,11 +6397,11 @@ ADD_NAMESPACES(at_ns0, at_ns1) >> ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address 80:88:88:88:88:88]) >> ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> +NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address 80:89:89:89:89:89]) >> >> dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0. >> AT_DATA([flows.txt], [dnl >> -in_port=1,tcp,action=ct(commit,zone=1,nat(src=10.1.1.240:34568,random)),2 >> -in_port=2,ct_state=-trk,tcp,tp_dst=34567,action=ct(table=0,zone=1,nat) > > Do you know why this flow was there in the first place? > AFAICT, this seemed to me part of C/P ("conntrack - SNAT with port range"). While at it, I preferred to clean it a bit as this (along with a couple of minor things) was not required. >> +in_port=1,tcp,action=ct(commit,zone=1,nat(src=10.1.1.240:34568)),2 >> in_port=2,ct_state=-trk,tcp,tp_dst=34568,action=ct(table=0,zone=1,nat) >> in_port=2,ct_state=+trk,ct_zone=1,tcp,action=1 >> dnl >> @@ -6426,17 +6425,25 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) >> >> dnl HTTP requests from p0->p1 should work fine. >> OVS_START_L7([at_ns1], [http]) >> -NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 1 -T 1 --retry-connrefused -v -o wget0.log]) >> + >> +dnl Send a valid SYN to make conntrack pick it up. >> +dnl The source port used is 123 to prevent unwanted reuse in the next HTTP request. >> +AT_CHECK([ovs-ofctl packet-out br0 "packet=80898989898980888888888808004500002800010000400664cb0a0101010a010102007b005000000000000000005002200079130000 actions=ct(commit,zone=1,nat(src=10.1.1.240:34568))"]) > > Can we use 'ovs-ofctl compose-packet --bare' instead of open-coding bytes? > sure, I'll send a v2. > Best regards, Ilya Maximets.
diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 2d12d558e..04559f5e8 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -6388,7 +6388,6 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([conntrack - SNAT with port range with exhaustion]) -OVS_CHECK_GITHUB_ACTION() CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() OVS_TRAFFIC_VSWITCHD_START() @@ -6398,11 +6397,11 @@ ADD_NAMESPACES(at_ns0, at_ns1) ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address 80:88:88:88:88:88]) ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") +NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address 80:89:89:89:89:89]) dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0. AT_DATA([flows.txt], [dnl -in_port=1,tcp,action=ct(commit,zone=1,nat(src=10.1.1.240:34568,random)),2 -in_port=2,ct_state=-trk,tcp,tp_dst=34567,action=ct(table=0,zone=1,nat) +in_port=1,tcp,action=ct(commit,zone=1,nat(src=10.1.1.240:34568)),2 in_port=2,ct_state=-trk,tcp,tp_dst=34568,action=ct(table=0,zone=1,nat) in_port=2,ct_state=+trk,ct_zone=1,tcp,action=1 dnl @@ -6426,17 +6425,25 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) dnl HTTP requests from p0->p1 should work fine. OVS_START_L7([at_ns1], [http]) -NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 1 -T 1 --retry-connrefused -v -o wget0.log]) + +dnl Send a valid SYN to make conntrack pick it up. +dnl The source port used is 123 to prevent unwanted reuse in the next HTTP request. +AT_CHECK([ovs-ofctl packet-out br0 "packet=80898989898980888888888808004500002800010000400664cb0a0101010a010102007b005000000000000000005002200079130000 actions=ct(commit,zone=1,nat(src=10.1.1.240:34568))"]) + +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | uniq], [0], [dnl +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),zone=1,protoinfo=(state=<cleared>) +]) NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 1 -T 1 --retry-connrefused -v -o wget0.log], [4]) -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/' | uniq], [0], [dnl -tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.2XX,sport=<cleared>,dport=<cleared>),zone=1,protoinfo=(state=<cleared>) +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | uniq], [0], [dnl +tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),zone=1,protoinfo=(state=<cleared>) ]) OVS_TRAFFIC_VSWITCHD_STOP(["dnl /Unable to NAT due to tuple space exhaustion - if DoS attack, use firewalling and\/or zone partitioning./d -/Dropped .* log messages in last .* seconds \(most recently, .* seconds ago\) due to excessive rate/d"]) +/Dropped .* log messages in last .* seconds \(most recently, .* seconds ago\) due to excessive rate/d +/|WARN|.* execute ct.* failed/d"]) AT_CLEANUP AT_SETUP([conntrack - more complex SNAT])
Recent kernels introduced a mechanism that allows to evict colliding entries in a closing state whereas they were previously considered as parts of a non-recoverable clash. This new behavior makes "conntrack - SNAT with port range with exhaustion test" fail, as it relies on the previous assumptions. Fix it by creating and not advancing the first entry in SYN_SENT to avoid early eviction. Suggested-by: Ilya Maximets <i.maximets@ovn.org> Reported-at: https://issues.redhat.com/browse/FDP-486 Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- tests/system-traffic.at | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)