On 2/13/25 23:31, Ilya Maximets wrote: > On 2/13/25 18:01, Paolo Valerio wrote: >> Ilya Maximets <[email protected]> writes: >> >>> 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. >>> >> >> Thank you for the review! >> >>>> >>>> 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. >>> >> >> It should not be missing as IGMP infomations don't get explicitly >> extracted and so should always be zero (the non-zeroed ones are >> the ones in extract_l4()). > > Makes sense. Applied and backported down to 3.3. > If needed we may backport that down to 2.17 later.
I did a few extra rounds of testing (CI doesn't cover system tests on older branches) and backported this further to 2.17, since we'll be making final releases for older branches soon. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
