On Thu, 4 Dec 2025 at 05:41, Mike Pattrick <[email protected]> wrote:
> On Wed, Nov 12, 2025 at 12:05 PM David Marchand <[email protected]>
> wrote:
>> diff --git a/lib/packets.c b/lib/packets.c
>> index a0bb2ad482..c05b4abcc8 100644
>> --- a/lib/packets.c
>> +++ b/lib/packets.c
>> @@ -2085,6 +2085,109 @@ out:
>> }
>> }
>>
>> +/* This helper computes a "constant" UDP checksum without looking at the
>> + * L4 payload.
>> + *
>> + * This is possible when L4 is either TCP or UDP: the L4 payload checksum
>> + * is either computed in SW or in HW later, but its contribution to the
>> + * outer checksum is cancelled by the L4 payload being part of the global
>> + * packet sum. */
>> +bool
>> +packet_udp_tunnel_csum(struct dp_packet *p)
>> +{
>> + const ovs_be16 *inner_l4_csum_p;
>> + struct ip_header *inner_ip;
>> + const void *inner_l4_data;
>> + struct udp_header *udp;
>> + ovs_be16 inner_l4_csum;
>> + uint32_t partial_csum;
>> + struct ip_header *ip;
>> + uint32_t inner_csum;
>> + void *inner_l4;
>> +
>> + inner_ip = dp_packet_inner_l3(p);
>> + inner_l4 = dp_packet_inner_l4(p);
>> + ip = dp_packet_l3(p);
>> + udp = dp_packet_l4(p);
>> +
>> + if (!dp_packet_inner_l4_proto_tcp(p)
>> + && !dp_packet_inner_l4_proto_udp(p)) {
>> + return false;
>> + }
>> +
>> + if (!dp_packet_inner_l4_checksum_valid(p)) {
>> + /* We have no idea about the contribution of the payload data
>> + * and what the L4 checksum put in the packet data looks like.
>> + * Simpler is to let a full checksum happen. */
>> + return false;
>> + }
>> +
>> + if (dp_packet_inner_l4_proto_tcp(p)) {
>> + inner_l4_csum_p = &(((struct tcp_header *) inner_l4)->tcp_csum);
>> + inner_l4_data = dp_packet_get_inner_tcp_payload(p);
>> + } else {
>> + ovs_assert(dp_packet_inner_l4_proto_udp(p));
>
>
> This seems redundant as both proto_tcp and proto_udp are checked above. Why
> not just have one if() else if() else to handle tcp, udp, and other?
Mm, I can't find a good reason to explain why I went this way.
>>
>> + inner_l4_csum_p = &(((struct udp_header *) inner_l4)->udp_csum);
>> + inner_l4_data = (char *) inner_l4 + sizeof (struct udp_header);
>> + if (*inner_l4_csum_p == 0) {
>> + /* There is no nested checksum.
>> + * No choice but compute a full checksum. */
>> + return false;
>> + }
>> + }
>> +
>> + if (IP_VER(inner_ip->ip_ihl_ver) == 4) {
>> + inner_csum = packet_csum_pseudoheader(inner_ip);
>> + } else {
>> + struct ovs_16aligned_ip6_hdr *inner_ip6 = dp_packet_inner_l3(p);
>> +
>> + inner_csum = packet_csum_pseudoheader6(inner_ip6);
>> + }
>> +
>> + ovs_assert(inner_l4_data);
>
>
> dp_packet_get_inner_tcp_payload could return NULL on a malformed TCP header,
> why not return false in that case? This I don't think we would be guaranteed
> to have an already validated tcp header here.
True that this helper may get used in unforeseen situations in the future.
Ok I'll change this as a check combined with the refactoring from above.
>
>>
>> + inner_csum = csum_continue(inner_csum, inner_l4,
>> + (char *) inner_l4_csum_p - (char *) inner_l4);
>> + inner_l4_csum = csum_finish(csum_continue(inner_csum, inner_l4_csum_p +
>> 1,
>> + (char *) inner_l4_data - (char *)(inner_l4_csum_p + 1)));
>> + if (dp_packet_inner_l4_proto_udp(p) && !inner_l4_csum) {
>> + inner_l4_csum = htons(0xffff);
>> + }
>> +
>> + udp->udp_csum = 0;
>> + if (IP_VER(ip->ip_ihl_ver) == 4) {
>>
>> + partial_csum = packet_csum_pseudoheader(ip);
>> + } else {
>> + struct ovs_16aligned_ip6_hdr *ip6 = dp_packet_l3(p);
>> +
>> + partial_csum = packet_csum_pseudoheader6(ip6);
>> + }
>> +
>> + partial_csum = csum_continue(partial_csum, udp,
>> + (char *) inner_ip - (char *) udp);
>> + if (IP_VER(inner_ip->ip_ihl_ver) != 4
>
>
> inner_ip->ip_ihl_ver is accessed repeatedly, would it be better as a local
> variable?
No strong opinion.
--
David Marchand
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
