[ovs-dev] [PATCH] ipf: release unhandled packets from the batch

[David Marchand]

On Tue, Oct 5, 2021 at 8:19 PM Aaron Conole <aconole at 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 <wanjunjie at bytedance.com>
> Reported-at: https://github.com/openvswitch/ovs-issues/issues/226
> Signed-off-by: Aaron Conole <aconole at redhat.com>

But in any case, this patch lgtm.

Reviewed-by: David Marchand <david.marchand at redhat.com>


-- 
David Marchand