diff options
author | Ilya Maximets <i.maximets@ovn.org> | 2019-11-01 22:24:39 +0100 |
---|---|---|
committer | Ilya Maximets <i.maximets@ovn.org> | 2019-11-26 09:48:36 +0100 |
commit | 47f8743e8434c3918a0e23cf9815016eb23c3daf (patch) | |
tree | 430814927eead20e89e9a950dcaf42dead6a5ae7 /ofproto/ofproto-dpif.c | |
parent | 953ef918883f80dc131f10e0882a977abcd2c561 (diff) | |
download | openvswitch-47f8743e8434c3918a0e23cf9815016eb23c3daf.tar.gz |
ofproto: Fix crash on PACKET_OUT due to recursive locking after upcall.
Handling of OpenFlow PACKET_OUT implies pushing the packet through
the datapath and packet processing inside the datapath could trigger
an upcall. In case of system datapath, 'dpif_execute()' sends packet
to the kernel module and returns. If any, upcall will be triggered
inside the kernel and handled by a separate thread in userspace.
But in case of userspace datapath full processing of the packet happens
inside the 'dpif_execute()' in the same thread that handled PACKET_OUT.
This causes an issue if upcall will lead to modification of flow rules.
For example, it could happen while processing of 'learn' actions.
Since whole handling of PACKET_OUT is protected by 'ofproto_mutex',
OVS will assert on attempt to take it recursively while processing
'learn' actions:
0 __GI_raise (sig=sig@entry=6)
1 __GI_abort ()
2 ovs_abort_valist ()
3 ovs_abort ()
4 ovs_mutex_lock_at (where=where@entry=0xad4199 "ofproto/ofproto.c:5391")
<Trying to acquire ofproto_mutex again>
5 ofproto_flow_mod_learn () at ofproto/ofproto.c:5391
<Trying to modify flows according to 'learn' action>
6 xlate_learn_action () at ofproto/ofproto-dpif-xlate.c:5378
<'learn' action found>
7 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:6893
8 xlate_recursively () at ofproto/ofproto-dpif-xlate.c:4233
9 xlate_table_action () at ofproto/ofproto-dpif-xlate.c:4361
10 in xlate_ofpact_resubmit () at ofproto/ofproto-dpif-xlate.c:4672
11 do_xlate_actions () at ofproto/ofproto-dpif-xlate.c:6773
12 xlate_actions () at ofproto/ofproto-dpif-xlate.c:7570
<Translating actions>
13 upcall_xlate () at ofproto/ofproto-dpif-upcall.c:1197
14 process_upcall () at ofproto/ofproto-dpif-upcall.c:1413
15 upcall_cb () at ofproto/ofproto-dpif-upcall.c:1315
16 dp_netdev_upcall (DPIF_UC_MISS) at lib/dpif-netdev.c:6236
<Flow cache miss. Making upcall>
17 handle_packet_upcall () at lib/dpif-netdev.c:6591
18 fast_path_processing () at lib/dpif-netdev.c:6709
19 dp_netdev_input__ () at lib/dpif-netdev.c:6797
20 dp_netdev_recirculate () at lib/dpif-netdev.c:6842
21 dp_execute_cb () at lib/dpif-netdev.c:7158
22 odp_execute_actions () at lib/odp-execute.c:794
23 dp_netdev_execute_actions () at lib/dpif-netdev.c:7332
24 dpif_netdev_execute () at lib/dpif-netdev.c:3725
25 dpif_netdev_operate () at lib/dpif-netdev.c:3756
<Packet pushed to userspace datapath for processing>
26 dpif_operate () at lib/dpif.c:1367
27 dpif_execute () at lib/dpif.c:1321
28 packet_execute () at ofproto/ofproto-dpif.c:4760
29 ofproto_packet_out_finish () at ofproto/ofproto.c:3594
<Taking ofproto_mutex>
30 handle_packet_out () at ofproto/ofproto.c:3635
31 handle_single_part_openflow (OFPTYPE_PACKET_OUT) at ofproto/ofproto.c:8400
32 handle_openflow () at ofproto/ofproto.c:8587
33 ofconn_run ()
34 connmgr_run ()
35 ofproto_run ()
36 bridge_run__ ()
37 bridge_run ()
38 main ()
Fix that by splitting the 'ofproto_packet_out_finish()' in two parts.
First one that translates side-effects and requires holding 'ofproto_mutex'
and the second that only pushes packet to datapath. The second part moved
out of 'ofproto_mutex' because 'ofproto_mutex' is not required and actually
should not be taken in order to avoid recursive locking.
Reported-by: Anil Kumar Koli <anilkumar.k@altencalsoftlabs.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2019-April/048494.html
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
Diffstat (limited to 'ofproto/ofproto-dpif.c')
-rw-r--r-- | ofproto/ofproto-dpif.c | 40 |
1 files changed, 30 insertions, 10 deletions
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 2cd786a16..63c3d8b71 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4884,12 +4884,13 @@ ofproto_dpif_xcache_execute(struct ofproto_dpif *ofproto, } static void -packet_execute(struct ofproto *ofproto_, struct ofproto_packet_out *opo) +packet_execute_prepare(struct ofproto *ofproto_, + struct ofproto_packet_out *opo) OVS_REQUIRES(ofproto_mutex) { struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); struct dpif_flow_stats stats; - struct dpif_execute execute; + struct dpif_execute *execute; struct ofproto_dpif_packet_out *aux = opo->aux; ovs_assert(aux); @@ -4898,22 +4899,40 @@ packet_execute(struct ofproto *ofproto_, struct ofproto_packet_out *opo) dpif_flow_stats_extract(opo->flow, opo->packet, time_msec(), &stats); ofproto_dpif_xcache_execute(ofproto, &aux->xcache, &stats); - execute.actions = aux->odp_actions.data; - execute.actions_len = aux->odp_actions.size; + execute = xzalloc(sizeof *execute); + execute->actions = xmemdup(aux->odp_actions.data, aux->odp_actions.size); + execute->actions_len = aux->odp_actions.size; pkt_metadata_from_flow(&opo->packet->md, opo->flow); - execute.packet = opo->packet; - execute.flow = opo->flow; - execute.needs_help = aux->needs_help; - execute.probe = false; - execute.mtu = 0; + execute->packet = opo->packet; + execute->flow = opo->flow; + execute->needs_help = aux->needs_help; + execute->probe = false; + execute->mtu = 0; /* Fix up in_port. */ ofproto_dpif_set_packet_odp_port(ofproto, opo->flow->in_port.ofp_port, opo->packet); - dpif_execute(ofproto->backer->dpif, &execute); ofproto_dpif_packet_out_delete(aux); + opo->aux = execute; +} + +static void +packet_execute(struct ofproto *ofproto_, struct ofproto_packet_out *opo) + OVS_EXCLUDED(ofproto_mutex) +{ + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); + struct dpif_execute *execute = opo->aux; + + if (!execute) { + return; + } + + dpif_execute(ofproto->backer->dpif, execute); + + free(CONST_CAST(struct nlattr *, execute->actions)); + free(execute); opo->aux = NULL; } @@ -6637,6 +6656,7 @@ const struct ofproto_class ofproto_dpif_class = { rule_get_stats, packet_xlate, packet_xlate_revert, + packet_execute_prepare, packet_execute, set_frag_handling, nxt_resume, |