On 23 Mar 2026, at 16:35, Mike Pattrick wrote:
> On Fri, Mar 20, 2026 at 8:43 AM Eelco Chaudron <[email protected]> wrote: > >> >> >> On 17 Mar 2026, at 19:55, Mike Pattrick via dev wrote: >> >>> When a batch is full, dp_packet_batch_add() frees the packet instead of >>> adding it to the batch. In dp_packet_gso__(), the code calls >>> dp_packet_batch_add() to add the original packet 'p' back into the >>> batch, but does not check if the packet was freed. If the packet was >>> freed, subsequent operations on 'p' (such as dp_packet_tunnel(), >>> dp_packet_get_inner_tcp_payload(), and dp_packet_set_size()) cause >>> undefined behavior. >>> >>> Fix by checking the return value and exiting early if the packet was >>> freed. >>> >>> Found with clang analyze. >>> >>> Fixes: ef762327f6d3 ("dp-packet-gso: Refactor software segmentation >> code.") >>> Signed-off-by: Mike Pattrick <[email protected]> >>> --- >>> lib/dp-packet-gso.c | 4 +++- >>> lib/dp-packet.h | 12 +++++++----- >>> 2 files changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c >>> index bceb851fb..0d4d52daf 100644 >>> --- a/lib/dp-packet-gso.c >>> +++ b/lib/dp-packet-gso.c >>> @@ -199,7 +199,9 @@ dp_packet_gso__(struct dp_packet *p, struct >> dp_packet_batch **batches, >>> if (dp_packet_batch_is_full(curr_batch)) { >>> curr_batch++; >>> } >>> - dp_packet_batch_add(curr_batch, p); >> >> Hi Mike, >> >> Thanks for the patch. However, I don't believe this change is necessary. >> >> Just one line above the proposed change, the code checks if the batch is >> full via dp_packet_batch_is_full(curr_batch). If it is, it moves to the >> next (empty) batch in the array. >> >> This appears to be a false positive from the static analyzer (I have also >> marked this as a false positive in Coverity previously). Furthermore, if >> we were to adopt this check, it is missing from the other invocations of >> dp_packet_batch_add() later in the same function. >> > > I do agree it's a false positive, however, personally I think checking the > return code is fairly light touch. If not desired, we could squash this > patch. My preference would be to drop it, as we change the API, and we only use it in this place for a false positive. //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
