On Thu, Jan 27, 2022 at 4:08 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> On 1/27/22 07:42, Sriharsha Basavapatna via dev wrote:
> > In dp_netdev_pmd_remove_flow() we schedule the deletion of an
> > offloaded flow, if a mark has been assigned to the flow. But if
> > this occurs in the window in which the offload thread completes
> > offloading the flow and assigns a mark to the flow, then we miss
> > deleting the flow. This problem has been observed while adding
> > and deleting flows in a loop. To fix this, always enqueue flow
> > deletion regardless of the flow->mark being set.
> >
> > Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
> > Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapa...@broadcom.com>
> > ---
> > lib/dpif-netdev.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index e28e0b554..22c5f19a8 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread
> > *pmd,
> > dp_netdev_simple_match_remove(pmd, flow);
> > cmap_remove(&pmd->flow_table, node, dp_netdev_flow_hash(&flow->ufid));
> > ccmap_dec(&pmd->n_flows, odp_to_u32(in_port));
> > - if (flow->mark != INVALID_FLOW_MARK) {
> > - queue_netdev_flow_del(pmd, flow);
> > - }
> > + queue_netdev_flow_del(pmd, flow);
> > flow->dead = true;
> >
> > dp_netdev_flow_unref(flow);
> >
>
> Thanks for the patch, but it looks like that it's not that simple...
> A lot of tests are crashing in GHA and ASAN reports use-after-free
> on flow disassociation if the dpif already destroyed:
> https://github.com/ovsrobot/ovs/actions/runs/1754866321
>
> I think that the problem is that offload thread holds the ref count
> for PMD thread, but PMD thread structure doesn't hold the ref count
> for the dp, because it doesn't expect that dp_netdev_pmd structure
> will be used while PMD thread is destroyed. I guess, we should fix
> that. OTOH, I'm not sure if offload thread actually needs a reference
> to dp_netdev_pmd structure. If I didn't miss something, it only
> uses pmd pointer to access pmd->dp. So, maybe, dp_offload_thread_item
> should hold and ref the dp pointer instead?
>
> What do you think? Gaetan?
>
> Another point is that queue_netdev_flow_del() should check for
> netdev_is_flow_api_enabled() to avoid creation of offload threads
> if offloading is disabled. But that's good that we didn't have it,
> as the refcount issue got exposed. Otherwise it would be hard
> to reproduce.
>
> Best regards, Ilya Maximets.
>
> Asan report below, for convenience:
>
> =================================================================
> ==17076==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x616000003080 at pc 0x0000005e0e38 bp 0x7fe0594309f0 sp 0x7fe0594309e8
> READ of size 8 at 0x616000003080 thread T5 (hw_offload4)
> #0 0x5e0e37 in mark_to_flow_disassociate
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2556:62
> #1 0x5dfaf1 in dp_offload_flow
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2821:15
> #2 0x5df8bd in dp_netdev_flow_offload_main
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2884:17
> #3 0x73a2fc in ovsthread_wrapper
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/ovs-thread.c:422:12
> #4 0x7fe0619506da in start_thread
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
> #5 0x7fe060ecf71e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12171e)
>
> 0x616000003080 is located 0 bytes inside of 576-byte region
> [0x616000003080,0x6160000032c0)
> freed by thread T0 here:
> #0 0x49640d in free
> (/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/vswitchd/ovs-vswitchd+0x49640d)
> #1 0x5d6652 in dp_netdev_free
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1980:5
> #2 0x5f0722 in dp_netdev_unref
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1991:13
> #3 0x5cf5e5 in dpif_netdev_close
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2002:5
> #4 0x5fc393 in dpif_uninit
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:1725:9
> #5 0x5fc0c0 in dpif_close
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:457:9
> #6 0x52a972 in close_dpif_backer
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:715:5
> #7 0x517f08 in destruct
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:1860:5
> #8 0x4f09e0 in ofproto_destroy
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto.c:1728:5
> #9 0x4c71e4 in bridge_destroy
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:3608:9
> #10 0x4c6f4a in bridge_exit
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:553:9
> #11 0x4e109a in main
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/ovs-vswitchd.c:146:5
> #12 0x7fe060dcfbf6 in __libc_start_main
> (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
>
> previously allocated by thread T0 here:
> #0 0x496802 in calloc
> (/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/vswitchd/ovs-vswitchd+0x496802)
> #1 0x7af6b2 in xcalloc__
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/util.c:121:31
> #2 0x5d5a89 in create_dp_netdev
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1810:10
> #3 0x5cdb5e in dpif_netdev_open
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1896:26
> #4 0x5fbbf1 in do_open
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:348:13
> #5 0x5fbfc8 in dpif_create_and_open
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:417:13
> #6 0x529547 in open_dpif_backer
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:776:13
> #7 0x5174eb in construct
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:1672:13
> #8 0x4ec78d in ofproto_create
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto.c:553:13
> #9 0x4c7fa7 in bridge_reconfigure
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:882:21
> #10 0x4c74d5 in bridge_run
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:3331:9
> #11 0x4e0fb1 in main
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/ovs-vswitchd.c:129:9
> #12 0x7fe060dcfbf6 in __libc_start_main
> (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
>
> Thread T5 (hw_offload4) created by T0 here:
> #0 0x480e1a in pthread_create
> (/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/vswitchd/ovs-vswitchd+0x480e1a)
> #1 0x739f17 in ovs_thread_create
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/ovs-thread.c:486:13
> #2 0x5df689 in dp_netdev_offload_init
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:415:9
> #3 0x5df580 in dp_netdev_append_offload
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2689:5
> #4 0x5deddb in dp_netdev_pmd_remove_flow
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:3032:5
> #5 0x5f5082 in flow_del_on_pmd
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:4297:9
> #6 0x5f3ec5 in dpif_netdev_flow_del
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:4340:17
> #7 0x5d1beb in dpif_netdev_operate
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:4602:25
> #8 0x5fec3e in dpif_operate
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:1372:13
> #9 0x5fe9f4 in dpif_flow_del
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:1062:5
> #10 0x5fdfdb in dpif_probe_feature
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:977:13
> #11 0x52ad76 in check_recirc
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:904:21
> #12 0x52a131 in check_support
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:1630:37
> #13 0x5298f6 in open_dpif_backer
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:820:5
> #14 0x5174eb in construct
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:1672:13
> #15 0x4ec78d in ofproto_create
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto.c:553:13
> #16 0x4c7fa7 in bridge_reconfigure
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:882:21
> #17 0x4c74d5 in bridge_run
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:3331:9
> #18 0x4e0fb1 in main
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/ovs-vswitchd.c:129:9
> #19 0x7fe060dcfbf6 in __libc_start_main
> (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
>
> SUMMARY: AddressSanitizer: heap-use-after-free
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2556:62
> in mark_to_flow_disassociate
> Shadow bytes around the buggy address:
> 0x0c2c7fff85c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2c7fff85d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2c7fff85e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2c7fff85f0: fd fd fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c2c7fff8600: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> =>0x0c2c7fff8610:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2c7fff8620: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2c7fff8630: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2c7fff8640: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2c7fff8650: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
> 0x0c2c7fff8660: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Partially addressable: 01 02 03 04 05 06 07
> Heap left redzone: fa
> Freed heap region: fd
> Stack left redzone: f1
> Stack mid redzone: f2
> Stack right redzone: f3
> Stack after return: f5
> Stack use after scope: f8
> Global redzone: f9
> Global init order: f6
> Poisoned by user: f7
> Container overflow: fc
> Array cookie: ac
> Intra object redzone: bb
> ASan internal: fe
> Left alloca redzone: ca
> Right alloca redzone: cb
> Shadow gap: cc
> ==17076==ABORTING
Thanks Ilya for catching these issues. Are these specific to 2.17 ? If
possible, can you please try this patch with 2.16 ?
Thanks,
-Harsha
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
