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