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:
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?
Thanks,
Eelco
** CID 556379: (SIGN_EXTENSION)
/lib/dp-packet-gso.c: 251 in dp_packet_gso__()
/lib/dp-packet-gso.c: 238 in dp_packet_gso__()
/lib/dp-packet-gso.c: 225 in dp_packet_gso__()
/lib/dp-packet-gso.c: 239 in dp_packet_gso__()
_____________________________________________________________________________________________
*** CID 556379: (SIGN_EXTENSION)
/lib/dp-packet-gso.c: 251 in dp_packet_gso__()
245 }
246 dp_packet_batch_add(curr_batch, seg);
247
248 first_seg:
249 if (partial_seg) {
250 if (dp_packet_gso_partial_nr_segs(p) != 1) {
>>> CID 556379: (SIGN_EXTENSION)
>>> Suspicious implicit sign extension: "tso_segsz" with type "uint16_t"
>>> (16 bits, unsigned) is promoted in "(n_segs - 1) * tso_segsz" to type "int"
>>> (32 bits, signed), then sign-extended to type "unsigned long" (64 bits,
>>> unsigned). If "(n_segs - 1) * tso_segsz" is greater than 0x7FFFFFFF, the
>>> upper bits of the result will all be 1.
251 dp_packet_set_size(p, hdr_len + (n_segs - 1) * tso_segsz);
252 if (n_segs == 2) {
253 /* No need to ask HW segmentation, we already did the
job. */
254 dp_packet_set_tso_segsz(p, 0);
255 }
256 }
/lib/dp-packet-gso.c: 238 in dp_packet_gso__()
232 }
233 dp_packet_batch_add(curr_batch, seg);
234 }
235
236 last_seg:
237 /* Create the last segment. */
>>> CID 556379: (SIGN_EXTENSION)
>>> Suspicious implicit sign extension: "tso_segsz" with type "uint16_t"
>>> (16 bits, unsigned) is promoted in "(n_segs - 1) * tso_segsz" to type "int"
>>> (32 bits, signed), then sign-extended to type "unsigned long" (64 bits,
>>> unsigned). If "(n_segs - 1) * tso_segsz" is greater than 0x7FFFFFFF, the
>>> upper bits of the result will all be 1.
238 seg = dp_packet_gso_seg_new(p, hdr_len, hdr_len + (n_segs - 1) *
tso_segsz,
239 data_len - (n_segs - 1) * tso_segsz);
240 dp_packet_gso_update_segment(seg, n_segs - 1, n_segs, tso_segsz,
udp_tnl,
241 gre_tnl);
242
243 if (dp_packet_batch_is_full(curr_batch)) {
/lib/dp-packet-gso.c: 225 in dp_packet_gso__()
219 goto last_seg;
220 }
221 goto first_seg;
222 }
223
224 for (int i = 1; i < n_segs - 1; i++) {
>>> CID 556379: (SIGN_EXTENSION)
>>> Suspicious implicit sign extension: "tso_segsz" with type "uint16_t"
>>> (16 bits, unsigned) is promoted in "i * tso_segsz" to type "int" (32 bits,
>>> signed), then sign-extended to type "unsigned long" (64 bits, unsigned).
>>> If "i * tso_segsz" is greater than 0x7FFFFFFF, the upper bits of the result
>>> will all be 1.
225 seg = dp_packet_gso_seg_new(p, hdr_len, hdr_len + i * tso_segsz,
226 tso_segsz);
227 dp_packet_gso_update_segment(seg, i, n_segs, tso_segsz, udp_tnl,
228 gre_tnl);
229
230 if (dp_packet_batch_is_full(curr_batch)) {
/lib/dp-packet-gso.c: 239 in dp_packet_gso__()
233 dp_packet_batch_add(curr_batch, seg);
234 }
235
236 last_seg:
237 /* Create the last segment. */
238 seg = dp_packet_gso_seg_new(p, hdr_len, hdr_len + (n_segs - 1) *
tso_segsz,
>>> CID 556379: (SIGN_EXTENSION)
>>> Suspicious implicit sign extension: "tso_segsz" with type "uint16_t"
>>> (16 bits, unsigned) is promoted in "(n_segs - 1) * tso_segsz" to type "int"
>>> (32 bits, signed), then sign-extended to type "unsigned long" (64 bits,
>>> unsigned). If "(n_segs - 1) * tso_segsz" is greater than 0x7FFFFFFF, the
>>> upper bits of the result will all be 1.
239 data_len - (n_segs - 1) * tso_segsz);
240 dp_packet_gso_update_segment(seg, n_segs - 1, n_segs, tso_segsz,
udp_tnl,
241 gre_tnl);
242
243 if (dp_packet_batch_is_full(curr_batch)) {
244 curr_batch++;
** 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
** 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)
> Reported-at: https://issues.redhat.com/browse/FDP-1897
> Signed-off-by: David Marchand <[email protected]>
> ---
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
