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.
Thanks!
Best regards, Ilya Maximets.
>
>> 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
