| Message ID | 20220717144211.949713-1-elibr@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 97211927f1cfa64a7835ada6827b033a5336891d |
| Headers | show |
| Series | [ovs-dev] conntrack: Fix conntrack multiple new state | 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 | fail | test: fail |
Hello Eli, Eli Britstein via dev <ovs-dev@openvswitch.org> writes: > A connection is established if we see packets from both directions. > The cited commit [1] fixed the issue of sending twice in one direction, > but still an issue if more than that. > Fix it. > The patch LGTM. Just a very minor nit: I guess "[1]" could be removed from the description. "The cited commit" seems enough. In any case, Acked-by: Paolo Valerio <pvalerio@redhat.com> > Fixes: a867c010ee91 ("conntrack: Fix conntrack new state") > Signed-off-by: Eli Britstein <elibr@nvidia.com> > --- > lib/conntrack-other.c | 7 ++++--- > tests/system-traffic.at | 9 +++++++++ > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c > index d3b4601858..7f3e63c384 100644 > --- a/lib/conntrack-other.c > +++ b/lib/conntrack-other.c > @@ -48,18 +48,19 @@ other_conn_update(struct conntrack *ct, struct conn *conn_, > struct dp_packet *pkt OVS_UNUSED, bool reply, long long now) > { > struct conn_other *conn = conn_other_cast(conn_); > - enum ct_update_res ret = CT_UPDATE_VALID; > > if (reply && conn->state != OTHERS_BIDIR) { > conn->state = OTHERS_BIDIR; > } else if (conn->state == OTHERS_FIRST) { > conn->state = OTHERS_MULTIPLE; > - ret = CT_UPDATE_VALID_NEW; > } > > conn_update_expiration(ct, &conn->up, other_timeouts[conn->state], now); > > - return ret; > + if (conn->state == OTHERS_BIDIR) { > + return CT_UPDATE_VALID; > + } > + return CT_UPDATE_VALID_NEW; > } > > static bool > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 89107ab624..182a78847e 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -3078,6 +3078,15 @@ NXST_FLOW reply: > table=1, priority=100,ct_state=+est+trk,in_port=1 actions=output:2 > ]) > > +dnl Send a 3rd UDP packet on port 1 > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"]) > + > +dnl There still should not be any packet that matches the established ct_state. > +AT_CHECK([ovs-ofctl dump-flows br0 "table=1 in_port=1,ct_state=+trk+est" | ofctl_strip], [0], [dnl > +NXST_FLOW reply: > + table=1, priority=100,ct_state=+est+trk,in_port=1 actions=output:2 > +]) > + > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > -- > 2.26.2.1730.g385c171 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 7/25/22 11:23, Paolo Valerio wrote: > Hello Eli, > > Eli Britstein via dev <ovs-dev@openvswitch.org> writes: > >> A connection is established if we see packets from both directions. >> The cited commit [1] fixed the issue of sending twice in one direction, >> but still an issue if more than that. >> Fix it. >> > > The patch LGTM. > Just a very minor nit: I guess "[1]" could be removed from the > description. "The cited commit" seems enough. > > In any case, > > Acked-by: Paolo Valerio <pvalerio@redhat.com> > >> Fixes: a867c010ee91 ("conntrack: Fix conntrack new state") >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> --- Thanks! Applied and backported down to 2.13. Best regards, Ilya Maximets.
diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c index d3b4601858..7f3e63c384 100644 --- a/lib/conntrack-other.c +++ b/lib/conntrack-other.c @@ -48,18 +48,19 @@ other_conn_update(struct conntrack *ct, struct conn *conn_, struct dp_packet *pkt OVS_UNUSED, bool reply, long long now) { struct conn_other *conn = conn_other_cast(conn_); - enum ct_update_res ret = CT_UPDATE_VALID; if (reply && conn->state != OTHERS_BIDIR) { conn->state = OTHERS_BIDIR; } else if (conn->state == OTHERS_FIRST) { conn->state = OTHERS_MULTIPLE; - ret = CT_UPDATE_VALID_NEW; } conn_update_expiration(ct, &conn->up, other_timeouts[conn->state], now); - return ret; + if (conn->state == OTHERS_BIDIR) { + return CT_UPDATE_VALID; + } + return CT_UPDATE_VALID_NEW; } static bool diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 89107ab624..182a78847e 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -3078,6 +3078,15 @@ NXST_FLOW reply: table=1, priority=100,ct_state=+est+trk,in_port=1 actions=output:2 ]) +dnl Send a 3rd UDP packet on port 1 +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"]) + +dnl There still should not be any packet that matches the established ct_state. +AT_CHECK([ovs-ofctl dump-flows br0 "table=1 in_port=1,ct_state=+trk+est" | ofctl_strip], [0], [dnl +NXST_FLOW reply: + table=1, priority=100,ct_state=+est+trk,in_port=1 actions=output:2 +]) + OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP
A connection is established if we see packets from both directions. The cited commit [1] fixed the issue of sending twice in one direction, but still an issue if more than that. Fix it. Fixes: a867c010ee91 ("conntrack: Fix conntrack new state") Signed-off-by: Eli Britstein <elibr@nvidia.com> --- lib/conntrack-other.c | 7 ++++--- tests/system-traffic.at | 9 +++++++++ 2 files changed, 13 insertions(+), 3 deletions(-)