On 1/27/22 13:09, Sriharsha Basavapatna wrote:
> 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)
<snip>
>
> Thanks Ilya for catching these issues. Are these specific to 2.17 ? If
> possible, can you please try this patch with 2.16 ?
This is not specific to 2.17, but it's harder to reproduce on 2.16, because
on 2.16 the main thread waits for the offloading thread to wake up. But the
race is still there and the following change exposes it:
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 8cca57f1f..d7036f6a7 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2667,6 +2667,7 @@ dp_netdev_flow_offload_main(void *data OVS_UNUSED)
&dp_flow_offload.mutex);
ovsrcu_quiesce_end();
}
+ xnanosleep(64000000);
list = ovs_list_pop_front(&dp_flow_offload.list);
offload = CONTAINER_OF(list, struct dp_flow_offload_item, node);
ovs_mutex_unlock(&dp_flow_offload.mutex);
---
With the delay above, unit tests on 2.16 are failing in the same way as
they do on current master. Therefore if the offloading thread will
be re-scheduled or delayed for any other reason, it may use already
freed datapath structure.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
