On Tue, Oct 5, 2021 at 8:19 PM Aaron Conole <acon...@redhat.com> wrote: > > Since 640d4db788ed ("ipf: Fix a use-after-free error, ...") the ipf > framework unconditionally allocates a new dp_packet to track > individual fragments. This prevents a use-after-free. However, an > additional issue was present - even when the packet buffer is cloned, > if the ip fragment handling code keeps it, the original buffer is > leaked during the refill loop. Even in the original processing code, > the hardcoded dnsteal branches would always leak a packet buffer from > the refill loop. > > This can be confirmed with valgrind: > > ==717566== 16,672 (4,480 direct, 12,192 indirect) bytes in 8 blocks are > definitely lost in loss record 390 of 390 > ==717566== at 0x484086F: malloc (vg_replace_malloc.c:380) > ==717566== by 0x537BFD: xmalloc__ (util.c:137) > ==717566== by 0x537BFD: xmalloc (util.c:172) > ==717566== by 0x46DDD4: dp_packet_new (dp-packet.c:153) > ==717566== by 0x46DDD4: dp_packet_new_with_headroom (dp-packet.c:163) > ==717566== by 0x550AA6: netdev_linux_batch_rxq_recv_sock.constprop.0 > (netdev-linux.c:1262) > ==717566== by 0x5512AF: netdev_linux_rxq_recv (netdev-linux.c:1511) > ==717566== by 0x4AB7E0: netdev_rxq_recv (netdev.c:727) > ==717566== by 0x47F00D: dp_netdev_process_rxq_port (dpif-netdev.c:4699) > ==717566== by 0x47FD13: dpif_netdev_run (dpif-netdev.c:5957) > ==717566== by 0x4331D2: type_run (ofproto-dpif.c:370) > ==717566== by 0x41DFD8: ofproto_type_run (ofproto.c:1768) > ==717566== by 0x40A7FB: bridge_run__ (bridge.c:3245) > ==717566== by 0x411269: bridge_run (bridge.c:3310) > ==717566== by 0x406E6C: main (ovs-vswitchd.c:127) > > The fix is to delete the original packet when it isn't able to be > reinserted into the packet batch. Subsequent valgrind runs show that > the packets are not leaked from the batch any longer. > > Fixes: 640d4db788ed ("ipf: Fix a use-after-free error, and remove the > 'do_not_steal' flag.")
Not sure I would flag this one, since as you explained, the leak was present from the commit below. > Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.") > Reported-by: Wan Junjie <wanjun...@bytedance.com> > Reported-at: https://github.com/openvswitch/ovs-issues/issues/226 > Signed-off-by: Aaron Conole <acon...@redhat.com> But in any case, this patch lgtm. Reviewed-by: David Marchand <david.march...@redhat.com> -- David Marchand _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev