Ilya Maximets <[email protected]> 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 <[email protected]> >> --- >> 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/[email protected]/ > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
