>On Tue, Jul 2, 2024 at 10:56 PM Jun Wang <[email protected]> wrote:
>>
>> We encountered a scenario where, if the received packet contains
>> padding bytes, and we then add Geneve tunnel encapsulation without
>> carrying the padding bytes, it results in checksum errors when sending
>> out. Therefore, adding an inner_l2_pad is necessary.
>>
>> For example, this type of packet format:
>> 0000 06 6c 6a 71 e1 d3 6c e2 d3 8b ea 24 81 00 03 e9
>> 0010 08 00 45 00 00 28 9d e5 40 00 3b 06 6e 64 0a 6f
>> 0020 05 14 0a fe 19 06 01 bb 22 ae 59 c0 8c 61 8e 26
>> 0030 14 e3 50 10 00 7f ce 3a 00 00 00 00 9e 38 cf 64
>
> Hello Jun,
>
> Thank you for this submission. This is an interesting case and I don't
> know that we have an appropriate test case for micrograms like this.
> Was this the
Yes, it does require the appropriate test case, but I am not very familiar with
this area.
> One question I have is shouldn't we remove the padding while we
> encapsulate, not keep it? The encapsulation should always push the
> frame size past 64 bytes.
I have also considered this approach. Theoretically, padding bytes
should be invalid and need to be removed. Shouldn't this be handled
at the packet reception stage rather than during the subsequent
encapsulation phase?
Alternatively, from another perspective, shouldn't we avoid modifying
any lengths, including meaningless padding bytes?
>Thanks,
>M
>
>>
>> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
>>
>> Signed-off-by: Jun Wang <[email protected]>
>> ---
>> lib/dp-packet.h | 21 ++++++++++++++++++++-
>> lib/netdev-native-tnl.c | 3 +++
>> 2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index a75b1c5..d583b28 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -176,6 +176,8 @@ struct dp_packet {
>> ovs_be32 packet_type; /* Packet type as defined in OpenFlow */
>> uint16_t csum_start; /* Position to start checksumming from.
>> */
>> uint16_t csum_offset; /* Offset to place checksum. */
>> + uint16_t inner_l2_pad_size; /* Detected inner l2 padding size.
>> + * Padding is non-pullable. */
>> union {
>> struct pkt_metadata md;
>> uint64_t data[DP_PACKET_CONTEXT_SIZE / 8];
>> @@ -209,7 +211,10 @@ static inline void *dp_packet_eth(const struct
>> dp_packet *);
>> static inline void dp_packet_reset_offsets(struct dp_packet *);
>> static inline void dp_packet_reset_offload(struct dp_packet *);
>> static inline uint16_t dp_packet_l2_pad_size(const struct dp_packet *);
>> +static inline uint16_t dp_packet_inner_l2_pad_size(const struct dp_packet
>> *);
>> static inline void dp_packet_set_l2_pad_size(struct dp_packet *, uint16_t);
>> +static inline void dp_packet_set_inner_l2_pad_size(struct dp_packet *,
>> + uint16_t);
>> static inline void *dp_packet_l2_5(const struct dp_packet *);
>> static inline void dp_packet_set_l2_5(struct dp_packet *, void *);
>> static inline void *dp_packet_l3(const struct dp_packet *);
>> @@ -435,6 +440,7 @@ static inline void
>> dp_packet_reset_offsets(struct dp_packet *b)
>> {
>> b->l2_pad_size = 0;
>> + b->inner_l2_pad_size = 0;
>> b->l2_5_ofs = UINT16_MAX;
>> b->l3_ofs = UINT16_MAX;
>> b->l4_ofs = UINT16_MAX;
>> @@ -448,6 +454,12 @@ dp_packet_l2_pad_size(const struct dp_packet *b)
>> return b->l2_pad_size;
>> }
>>
>> +static inline uint16_t
>> +dp_packet_inner_l2_pad_size(const struct dp_packet *b)
>> +{
>> + return b->inner_l2_pad_size;
>> +}
>> +
>> static inline void
>> dp_packet_set_l2_pad_size(struct dp_packet *b, uint16_t pad_size)
>> {
>> @@ -455,6 +467,13 @@ dp_packet_set_l2_pad_size(struct dp_packet *b, uint16_t
>> pad_size)
>> b->l2_pad_size = pad_size;
>> }
>>
>> +static inline void
>> +dp_packet_set_inner_l2_pad_size(struct dp_packet *b, uint16_t pad_size)
>> +{
>> + ovs_assert(pad_size <= dp_packet_size(b));
>> + b->inner_l2_pad_size = pad_size;
>> +}
>> +
>> static inline void *
>> dp_packet_l2_5(const struct dp_packet *b)
>> {
>> @@ -543,7 +562,7 @@ dp_packet_inner_l4_size(const struct dp_packet *b)
>> return OVS_LIKELY(b->inner_l4_ofs != UINT16_MAX)
>> ? (const char *) dp_packet_tail(b)
>> - (const char *) dp_packet_inner_l4(b)
>> - - dp_packet_l2_pad_size(b)
>> + - dp_packet_inner_l2_pad_size(b)
>> : 0;
>> }
>>
>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>> index 0f9f07f..96ffdc1 100644
>> --- a/lib/netdev-native-tnl.c
>> +++ b/lib/netdev-native-tnl.c
>> @@ -156,6 +156,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet,
>> const void *header,
>> struct eth_header *eth;
>> struct ip_header *ip;
>> struct ovs_16aligned_ip6_hdr *ip6;
>> + uint16_t l2_pad_size;
>>
>> eth = dp_packet_push_uninit(packet, size);
>> *ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
>> @@ -163,7 +164,9 @@ netdev_tnl_push_ip_header(struct dp_packet *packet,
>> const void *header,
>> memcpy(eth, header, size);
>> /* The encapsulated packet has type Ethernet. Adjust dp_packet. */
>> packet->packet_type = htonl(PT_ETH);
>> + l2_pad_size = dp_packet_l2_pad_size(packet);
>> dp_packet_reset_offsets(packet);
>> + dp_packet_set_inner_l2_pad_size(packet, l2_pad_size);
>> packet->l3_ofs = sizeof (struct eth_header);
>>
>> if (netdev_tnl_is_header_ipv6(header)) {
>> --
>> 1.8.3.1
>>
>>
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
Jun Wang
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
