On Wed, Oct 6, 2021 at 4:26 PM David Marchand <david.march...@redhat.com> wrote:
>
> 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
Tested-by: Wan Junjie <wanjun...@bytedance.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
