On Thu, 8 Jan 2026 at 09:37, Eelco Chaudron <[email protected]> wrote:
> On 15 Dec 2025, at 14:57, David Marchand via dev wrote:
>
> > If we can predict that the segmented traffic will have the same headers,
> > it is possible to rely on HW segmentation.
> >
> > TCP over IPv4 geneve (with checksum on tunnel) on a mlx5 nic:
> > Before: 7.80 Gbits/sec, 100% cpu (SW segmentation)
> > After: 10.8 Gbits/sec, 27% cpu (HW segmentation)
> >
> > For this optimisation, no touching of original tso_segsz should be
> > allowed, so revert the commit
> > 844a7cfa6edd ("netdev-dpdk: Use guest TSO segmentation size hint.").
> >
> > Yet, there might still be some non optimal cases where the L4 payload
> > size would not be a multiple of tso_segsz.
> > For this condition, "partially" segment: split the "last" segment and keep
> > n-1 segments data in the original packet which will be segmented by HW.
> >
>
> Hi David,
>
> Coverity reports an issue with this series. The abstract is below, and I
> think a simple cast could fix the sign problemns, for example:
I'll have a try on the sign extension warning and post a patch.
>
> dp_packet_set_size(p, hdr_len +
> (size_t)(n_segs - 1) * tso_segsz);
>
> The memory access ones, I did not investigate. Could you check and send a
> patch with fixes if needed?
[snip]
> ** CID 556378: Memory - illegal accesses (OVERRUN)
>
>
> _____________________________________________________________________________________________
> *** CID 556378: Memory - illegal accesses (OVERRUN)
> /lib/packets.c: 2154 in packet_udp_tunnel_csum()
> 2148
> 2149 inner_csum = packet_csum_pseudoheader6(inner_ip6);
> 2150 }
> 2151
> 2152 inner_csum = csum_continue(inner_csum, inner_l4,
> 2153 (char *) inner_l4_csum_p - (char *) inner_l4);
> >>> CID 556378: Memory - illegal accesses (OVERRUN)
> >>> Overrunning array of 2 bytes at byte offset 2 by dereferencing
> >>> pointer "inner_l4_csum_p + 1".
> 2154 inner_l4_csum = csum_finish(csum_continue(inner_csum,
> inner_l4_csum_p + 1,
> 2155 (char *) inner_l4_data - (char *)(inner_l4_csum_p + 1)));
> 2156 /* Important: for inner UDP, a null inner_l4_csum here should in
> theory be
> 2157 * replaced with 0xffff. However, since the only use of
> inner_l4_csum is
> 2158 * for the final outer checksum with a csum_add16() below, we
> can skip this
> 2159 * entirely because adding 0xffff will have the same effect as
> adding 0x0
Hum, this comes from the fact that inner_l4_csum_p points at an
explicit uint16_t field in the udp header.
I will try and see if switching to some pointer arithmetics
(referencing udp header + offsetof csum) pleases coverity...
>
> ** CID 556377: Memory - illegal accesses (USE_AFTER_FREE)
>
>
> _____________________________________________________________________________________________
> *** CID 556377: Memory - illegal accesses (USE_AFTER_FREE)
> /lib/dp-packet-gso.c: 207 in dp_packet_gso__()
> 201 dp_packet_batch_add(curr_batch, p);
> 202
> 203 if (n_segs == 1) {
> 204 goto out;
> 205 }
> 206
> >>> CID 556377: Memory - illegal accesses (USE_AFTER_FREE)
> >>> Calling "dp_packet_tunnel" dereferences freed pointer "p".
> 207 if (dp_packet_tunnel(p)) {
> 208 hdr_len = (char *) dp_packet_get_inner_tcp_payload(p)
> 209 - (char *) dp_packet_eth(p);
> 210 data_len = dp_packet_get_inner_tcp_payload_length(p);
> 211 } else {
> 212 hdr_len = (char *) dp_packet_get_tcp_payload(p)
This one is a false positive.
3. Condition dp_packet_batch_is_full(curr_batch), taking true branch.
198 if (dp_packet_batch_is_full(curr_batch)) {
199 curr_batch++;
200 }
4. freed_arg: dp_packet_batch_add frees p.[hide details]
201 dp_packet_batch_add(curr_batch, p);
Coverity fails to understand that we switched to a new empty batch.
Can you waive CID 556377?
Thanks for the report.
--
David Marchand
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
