| Message ID | 20240312100255.498965-1-pvalerio@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] conntrack: Do not use icmp reverse helper for icmpv6. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3/12/24 11:02, Paolo Valerio wrote: > In the flush tuple code path, while populating the conn_key, > reverse_icmp_type() gets called for both icmp and icmpv6 cases, > while, depending on the proto, its respective helper should be > called, instead. Thanks for the fix! Some minor nits below. > > The above leads to an abort: > > [...] > 0x00007f3d461888ff in __GI_abort () at abort.c:79 > 0x000000000064eeb7 in reverse_icmp_type (type=128 '\200') at lib/conntrack.c:1795 > 0x0000000000650a63 in tuple_to_conn_key (tuple=0x7ffe0db5c620, zone=0, key=0x7ffe0db5c520) > at lib/conntrack.c:2590 > 0x00000000006510f7 in conntrack_flush_tuple (ct=0x25715a0, tuple=0x7ffe0db5c620, zone=0) at lib/conntrack.c:2787 > 0x00000000004b5988 in dpif_netdev_ct_flush (dpif=0x25e4640, zone=0x7ffe0db5c6a4, tuple=0x7ffe0db5c620) > at lib/dpif-netdev.c:9618 > 0x000000000049938a in ct_dpif_flush_tuple (dpif=0x25e4640, zone=0x0, match=0x7ffe0db5c7e0) at lib/ct-dpif.c:331 > 0x000000000049942a in ct_dpif_flush (dpif=0x25e4640, zone=0x0, match=0x7ffe0db5c7e0) at lib/ct-dpif.c:361 > 0x0000000000657b9a in dpctl_flush_conntrack (argc=2, argv=0x254ceb0, dpctl_p=0x7ffe0db5c8a0) at lib/dpctl.c:1797 > 0x000000000065af36 in dpctl_unixctl_handler (conn=0x25c48d0, argc=2, argv=0x254ceb0, > [...] Could you, please, strip out some unnecessary information from the trace? For example, function addresses in hex are not actually needed and most of the function arguments are not needed as well. Only a few of the arguments are actually important. Removing those will shorten the lines and make the trace more clear for the reader. > > Fix it by calling reverse_icmp6_type() when needed. > Furthermore, self tests have been modified in order to exercise and > check this behavior. > > Fixes: 271e48a0e244 ("conntrack: Support conntrack flush by ct 5-tuple") > Reported-at: https://issues.redhat.com/browse/FDP-447 > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- > lib/conntrack.c | 4 +++- > tests/system-traffic.at | 10 +++++++++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 5786424f6..a62f27d24 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2586,7 +2586,9 @@ tuple_to_conn_key(const struct ct_dpif_tuple *tuple, uint16_t zone, > key->src.icmp_type = tuple->icmp_type; > key->src.icmp_code = tuple->icmp_code; > key->dst.icmp_id = tuple->icmp_id; > - key->dst.icmp_type = reverse_icmp_type(tuple->icmp_type); > + key->dst.icmp_type = (tuple->ip_proto == IPPROTO_ICMP) ? > + reverse_icmp_type(tuple->icmp_type) : > + reverse_icmp6_type(tuple->icmp_type); Please, wrap the lines before ?:, not after. And align the branches of the ternary to the beginning of a condition, i.e.: + key->dst.icmp_type = (tuple->ip_proto == IPPROTO_ICMP) + ? reverse_icmp_type(tuple->icmp_type) + : reverse_icmp6_type(tuple->icmp_type); Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 3/12/24 11:02, Paolo Valerio wrote: >> In the flush tuple code path, while populating the conn_key, >> reverse_icmp_type() gets called for both icmp and icmpv6 cases, >> while, depending on the proto, its respective helper should be >> called, instead. > > Thanks for the fix! > > Some minor nits below. > >> >> The above leads to an abort: >> >> [...] >> 0x00007f3d461888ff in __GI_abort () at abort.c:79 >> 0x000000000064eeb7 in reverse_icmp_type (type=128 '\200') at lib/conntrack.c:1795 >> 0x0000000000650a63 in tuple_to_conn_key (tuple=0x7ffe0db5c620, zone=0, key=0x7ffe0db5c520) >> at lib/conntrack.c:2590 >> 0x00000000006510f7 in conntrack_flush_tuple (ct=0x25715a0, tuple=0x7ffe0db5c620, zone=0) at lib/conntrack.c:2787 >> 0x00000000004b5988 in dpif_netdev_ct_flush (dpif=0x25e4640, zone=0x7ffe0db5c6a4, tuple=0x7ffe0db5c620) >> at lib/dpif-netdev.c:9618 >> 0x000000000049938a in ct_dpif_flush_tuple (dpif=0x25e4640, zone=0x0, match=0x7ffe0db5c7e0) at lib/ct-dpif.c:331 >> 0x000000000049942a in ct_dpif_flush (dpif=0x25e4640, zone=0x0, match=0x7ffe0db5c7e0) at lib/ct-dpif.c:361 >> 0x0000000000657b9a in dpctl_flush_conntrack (argc=2, argv=0x254ceb0, dpctl_p=0x7ffe0db5c8a0) at lib/dpctl.c:1797 >> 0x000000000065af36 in dpctl_unixctl_handler (conn=0x25c48d0, argc=2, argv=0x254ceb0, >> [...] > > Could you, please, strip out some unnecessary information from > the trace? For example, function addresses in hex are not > actually needed and most of the function arguments are not > needed as well. Only a few of the arguments are actually important. > Removing those will shorten the lines and make the trace more > clear for the reader. > >> >> Fix it by calling reverse_icmp6_type() when needed. >> Furthermore, self tests have been modified in order to exercise and >> check this behavior. >> >> Fixes: 271e48a0e244 ("conntrack: Support conntrack flush by ct 5-tuple") >> Reported-at: https://issues.redhat.com/browse/FDP-447 >> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> >> --- >> lib/conntrack.c | 4 +++- >> tests/system-traffic.at | 10 +++++++++- >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 5786424f6..a62f27d24 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -2586,7 +2586,9 @@ tuple_to_conn_key(const struct ct_dpif_tuple *tuple, uint16_t zone, >> key->src.icmp_type = tuple->icmp_type; >> key->src.icmp_code = tuple->icmp_code; >> key->dst.icmp_id = tuple->icmp_id; >> - key->dst.icmp_type = reverse_icmp_type(tuple->icmp_type); >> + key->dst.icmp_type = (tuple->ip_proto == IPPROTO_ICMP) ? >> + reverse_icmp_type(tuple->icmp_type) : >> + reverse_icmp6_type(tuple->icmp_type); > > Please, wrap the lines before ?:, not after. And align the branches > of the ternary to the beginning of a condition, i.e.: > > + key->dst.icmp_type = (tuple->ip_proto == IPPROTO_ICMP) > + ? reverse_icmp_type(tuple->icmp_type) > + : reverse_icmp6_type(tuple->icmp_type); > Thank you Ilya. I sent a v2 with your suggestions: https://patchwork.ozlabs.org/project/openvswitch/patch/20240328165608.273344-1-pvalerio@redhat.com/ > Best regards, Ilya Maximets.
diff --git a/lib/conntrack.c b/lib/conntrack.c index 5786424f6..a62f27d24 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2586,7 +2586,9 @@ tuple_to_conn_key(const struct ct_dpif_tuple *tuple, uint16_t zone, key->src.icmp_type = tuple->icmp_type; key->src.icmp_code = tuple->icmp_code; key->dst.icmp_id = tuple->icmp_id; - key->dst.icmp_type = reverse_icmp_type(tuple->icmp_type); + key->dst.icmp_type = (tuple->ip_proto == IPPROTO_ICMP) ? + reverse_icmp_type(tuple->icmp_type) : + reverse_icmp6_type(tuple->icmp_type); key->dst.icmp_code = tuple->icmp_code; } else { key->src.port = tuple->src_port; diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 2d12d558e..87de0692a 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -3103,7 +3103,10 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=<cleared>,type=0,code=0) ]) -AT_CHECK([ovs-appctl dpctl/flush-conntrack]) +AT_CHECK([ovs-appctl dpctl/flush-conntrack 'ct_nw_src=10.1.1.1,ct_nw_dst=10.1.1.2']) + +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl +]) dnl Pings from ns1->ns0 should fail. NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 | FORMAT_PING], [0], [dnl @@ -3244,6 +3247,11 @@ AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl icmpv6,orig=(src=fc00::1,dst=fc00::2,id=<cleared>,type=128,code=0),reply=(src=fc00::2,dst=fc00::1,id=<cleared>,type=129,code=0) ]) +AT_CHECK([ovs-appctl dpctl/flush-conntrack 'ct_ipv6_src=fc00::1,ct_ipv6_dst=fc00::2']) + +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fc00::2)], [0], [dnl +]) + OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP
In the flush tuple code path, while populating the conn_key, reverse_icmp_type() gets called for both icmp and icmpv6 cases, while, depending on the proto, its respective helper should be called, instead. The above leads to an abort: [...] 0x00007f3d461888ff in __GI_abort () at abort.c:79 0x000000000064eeb7 in reverse_icmp_type (type=128 '\200') at lib/conntrack.c:1795 0x0000000000650a63 in tuple_to_conn_key (tuple=0x7ffe0db5c620, zone=0, key=0x7ffe0db5c520) at lib/conntrack.c:2590 0x00000000006510f7 in conntrack_flush_tuple (ct=0x25715a0, tuple=0x7ffe0db5c620, zone=0) at lib/conntrack.c:2787 0x00000000004b5988 in dpif_netdev_ct_flush (dpif=0x25e4640, zone=0x7ffe0db5c6a4, tuple=0x7ffe0db5c620) at lib/dpif-netdev.c:9618 0x000000000049938a in ct_dpif_flush_tuple (dpif=0x25e4640, zone=0x0, match=0x7ffe0db5c7e0) at lib/ct-dpif.c:331 0x000000000049942a in ct_dpif_flush (dpif=0x25e4640, zone=0x0, match=0x7ffe0db5c7e0) at lib/ct-dpif.c:361 0x0000000000657b9a in dpctl_flush_conntrack (argc=2, argv=0x254ceb0, dpctl_p=0x7ffe0db5c8a0) at lib/dpctl.c:1797 0x000000000065af36 in dpctl_unixctl_handler (conn=0x25c48d0, argc=2, argv=0x254ceb0, [...] Fix it by calling reverse_icmp6_type() when needed. Furthermore, self tests have been modified in order to exercise and check this behavior. Fixes: 271e48a0e244 ("conntrack: Support conntrack flush by ct 5-tuple") Reported-at: https://issues.redhat.com/browse/FDP-447 Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/conntrack.c | 4 +++- tests/system-traffic.at | 10 +++++++++- 2 files changed, 12 insertions(+), 2 deletions(-)