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()).
> 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
