On 2/8/25 22:12, Paolo Valerio wrote:
> Ales reported that ct_tp_{src,dst} matches are working only for the
> first frag for the userspace datapath, whereas they are always working
> for later frags in the case of kernel datapath.
>
> The ipf propagates the info in packets metadata, but
> miniflow_extract() has no handling for them.
>
> Fix it by pushing the relevant fields in the miniflow.
> tp_{src,dst} are not set for later frags, so fill them with padding as
> ct_tp_{src,dst} are not aligned:
>
> struct flow {
> [...]
> ovs_be16 tp_src; /* 656 2 */
> ovs_be16 tp_dst; /* 658 2 */
> ovs_be16 ct_tp_src; /* 660 2 */
> ovs_be16 ct_tp_dst; /* 662 2 */
> [...]
> }
>
> The patch also includes two tests to exercise the behavior.
>
> Reported-at: https://issues.redhat.com/browse/FDP-124
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Paolo Valerio <[email protected]>
> ---
> v2:
> - Rearranged fragmented IPv6 packets in the test (Ilya)
> - Added tshark dissection output for all packets (Ilya)
> - while at it, changed the IPv4/UDP packet using a payload with
> repetitions to make it smaller
> - updated IPv6 test
> ---
> lib/flow.c | 21 ++++-
> tests/system-traffic.at | 171 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 189 insertions(+), 3 deletions(-)
Thanks, Paolo! The change looks good to me in gneral, I only have one small
comment below.
>
> diff --git a/lib/flow.c b/lib/flow.c
> index 0eb34892f..68d673123 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -402,6 +402,14 @@ parse_ethertype(const void **datap, size_t *sizep)
> return htons(FLOW_DL_TYPE_NONE);
> }
>
> +static inline bool
> +icmp6_is_nd(const struct icmp6_data_header *icmp6)
> +{
> + return (icmp6->icmp6_base.icmp6_code == 0 &&
> + (icmp6->icmp6_base.icmp6_type == ND_NEIGHBOR_SOLICIT ||
> + icmp6->icmp6_base.icmp6_type == ND_NEIGHBOR_ADVERT));
> +}
> +
> /* Returns 'true' if the packet is an ND packet. In that case the
> '*nd_target'
> * and 'arp_buf[]' are filled in. If the packet is not an ND packet, 'false'
> * is returned and no values are filled in on '*nd_target' or 'arp_buf[]'. */
> @@ -412,9 +420,7 @@ parse_icmpv6(const void **datap, size_t *sizep,
> const union ovs_16aligned_in6_addr **nd_target,
> struct eth_addr arp_buf[2], uint8_t *opt_type)
> {
> - if (icmp6->icmp6_base.icmp6_code != 0 ||
> - (icmp6->icmp6_base.icmp6_type != ND_NEIGHBOR_SOLICIT &&
> - icmp6->icmp6_base.icmp6_type != ND_NEIGHBOR_ADVERT)) {
> + if (!icmp6_is_nd(icmp6)) {
> return false;
> }
>
> @@ -1166,6 +1172,15 @@ miniflow_extract(struct dp_packet *packet, struct
> miniflow *dst)
> }
> }
> }
> + } else if (ct_nw_proto_p &&
> + (*ct_nw_proto_p == IPPROTO_TCP ||
> + *ct_nw_proto_p == IPPROTO_UDP ||
> + *ct_nw_proto_p == IPPROTO_SCTP ||
> + *ct_nw_proto_p == IPPROTO_ICMP ||
Are we missing IGMP here as well? If so, I can add it while applying
the change.
Best regards, Ilya Maximets.
> + (*ct_nw_proto_p == IPPROTO_ICMPV6 && !icmp6_is_nd(data)))) {
> + miniflow_pad_from_64(mf, ct_tp_src);
> + miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
> + miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
> }
<snip>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
