From: Daniele Di Proietto Date: Tue, 4 Oct 2016 21:53:31 +0000 (-0700) Subject: dpif-netdev: Fix crash in dpif_netdev_execute(). X-Git-Tag: v2.12.3~4458 X-Git-Url: https://git.proxmox.com/?a=commitdiff_plain;h=546e57d44c473aac2915037f6906c9dd04294105;hp=888e071e7d95d8ba2b21874af94213424957feda;p=mirror_ovs.git dpif-netdev: Fix crash in dpif_netdev_execute(). dp_netdev_get_pmd() is allowed to return NULL (even if we call it with NON_PMD_CORE_ID) for different reasons: * Since we use RCU to protect pmd threads, it is possible that ovs_refcount_try_ref_rcu() has failed. * During reconfiguration we destroy every thread. This commit makes sure that we always handle the case when dp_netdev_get_pmd() returns NULL without crashing (the change in dpif_netdev_run() doesn't fix anything, because everything is happening in the main thread, but it's better to honor the interface in case we change our threading model). This actually fixes a pretty serious crash that happens if dpif_netdev_execute() is called from a non pmd thread while reconfiguration is happening. It can be triggered by enabling bfd (because it's handled by the monitor thread, which is a non pmd thread) on an interface and changing something that requires datapath reconfiguration (n_rxq, pmd-cpu-mask, mtu). A testcase that reproduces the race condition is included. This is a possible backtrace of the segfault: #0 0x000000000060c7f1 in dp_execute_cb (aux_=0x7f1dd2d2a320, packets_=0x7f1dd2d2a370, a=0x7f1dd2d2a658, may_steal=false) at ../lib/dpif-netdev.c:4357 #1 0x00000000006448b2 in odp_execute_actions (dp=0x7f1dd2d2a320, batch=0x7f1dd2d2a370, steal=false, actions=0x7f1dd2d2a658, actions_len=8, dp_execute_action=0x60c7a5 ) at ../lib/odp-execute.c:538 #2 0x000000000060d00c in dp_netdev_execute_actions (pmd=0x0, packets=0x7f1dd2d2a370, may_steal=false, flow=0x7f1dd2d2ae70, actions=0x7f1dd2d2a658, actions_len=8, now=44965873) at ../lib/dpif-netdev.c:4577 #3 0x000000000060834a in dpif_netdev_execute (dpif=0x2b67b70, execute=0x7f1dd2d2a578) at ../lib/dpif-netdev.c:2624 #4 0x0000000000608441 in dpif_netdev_operate (dpif=0x2b67b70, ops=0x7f1dd2d2a5c8, n_ops=1) at ../lib/dpif-netdev.c:2654 #5 0x0000000000610a30 in dpif_operate (dpif=0x2b67b70, ops=0x7f1dd2d2a5c8, n_ops=1) at ../lib/dpif.c:1268 #6 0x000000000061098c in dpif_execute (dpif=0x2b67b70, execute=0x7f1dd2d2aa50) at ../lib/dpif.c:1233 #7 0x00000000005b9008 in ofproto_dpif_execute_actions__ (ofproto=0x2b69360, version=18446744073709551614, flow=0x7f1dd2d2ae70, rule=0x0, ofpacts=0x7f1dd2d2b100, ofpacts_len=16, indentation=0, depth=0, resubmits=0, packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif.c:3806 #8 0x00000000005b907a in ofproto_dpif_execute_actions (ofproto=0x2b69360, version=18446744073709551614, flow=0x7f1dd2d2ae70, rule=0x0, ofpacts=0x7f1dd2d2b100, ofpacts_len=16, packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif.c:3823 #9 0x00000000005dea9b in xlate_send_packet (ofport=0x2b98380, oam=false, packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif-xlate.c:5792 #10 0x00000000005bab12 in ofproto_dpif_send_packet (ofport=0x2b98380, oam=false, packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif.c:4628 #11 0x00000000005c3fc8 in monitor_mport_run (mport=0x2b8cd00, packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif-monitor.c:287 #12 0x00000000005c3d9b in monitor_run () at ../ofproto/ofproto-dpif-monitor.c:227 #13 0x00000000005c3cab in monitor_main (args=0x0) at ../ofproto/ofproto-dpif-monitor.c:189 #14 0x00000000006a183a in ovsthread_wrapper (aux_=0x2b8afd0) at ../lib/ovs-thread.c:342 #15 0x00007f1dd75eb444 in start_thread (arg=0x7f1dd2d2c700) at pthread_create.c:333 #16 0x00007f1dd6e1d20d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109 Signed-off-by: Daniele Di Proietto Acked-by: Ben Pfaff --- diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 6e09e44eb..83a309788 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -2602,6 +2602,9 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute) pmd = ovsthread_getspecific(dp->per_pmd_key); if (!pmd) { pmd = dp_netdev_get_pmd(dp, NON_PMD_CORE_ID); + if (!pmd) { + return EBUSY; + } } /* If the current thread is non-pmd thread, acquires @@ -2967,25 +2970,28 @@ dpif_netdev_run(struct dpif *dpif) { struct dp_netdev_port *port; struct dp_netdev *dp = get_dp_netdev(dpif); - struct dp_netdev_pmd_thread *non_pmd = dp_netdev_get_pmd(dp, - NON_PMD_CORE_ID); + struct dp_netdev_pmd_thread *non_pmd; uint64_t new_tnl_seq; ovs_mutex_lock(&dp->port_mutex); - ovs_mutex_lock(&dp->non_pmd_mutex); - HMAP_FOR_EACH (port, node, &dp->ports) { - if (!netdev_is_pmd(port->netdev)) { - int i; + non_pmd = dp_netdev_get_pmd(dp, NON_PMD_CORE_ID); + if (non_pmd) { + ovs_mutex_lock(&dp->non_pmd_mutex); + HMAP_FOR_EACH (port, node, &dp->ports) { + if (!netdev_is_pmd(port->netdev)) { + int i; - for (i = 0; i < port->n_rxq; i++) { - dp_netdev_process_rxq_port(non_pmd, port, port->rxqs[i].rxq); + for (i = 0; i < port->n_rxq; i++) { + dp_netdev_process_rxq_port(non_pmd, port, + port->rxqs[i].rxq); + } } } - } - dpif_netdev_xps_revalidate_pmd(non_pmd, time_msec(), false); - ovs_mutex_unlock(&dp->non_pmd_mutex); + dpif_netdev_xps_revalidate_pmd(non_pmd, time_msec(), false); + ovs_mutex_unlock(&dp->non_pmd_mutex); - dp_netdev_pmd_unref(non_pmd); + dp_netdev_pmd_unref(non_pmd); + } if (dp_netdev_is_reconf_required(dp) || ports_require_restart(dp)) { reconfigure_pmd_threads(dp); @@ -3189,7 +3195,8 @@ dp_netdev_pmd_reload_done(struct dp_netdev_pmd_thread *pmd) } /* Finds and refs the dp_netdev_pmd_thread on core 'core_id'. Returns - * the pointer if succeeds, otherwise, NULL. + * the pointer if succeeds, otherwise, NULL (it can return NULL even if + * 'core_id' is NON_PMD_CORE_ID). * * Caller must unrefs the returned reference. */ static struct dp_netdev_pmd_thread * diff --git a/tests/pmd.at b/tests/pmd.at index 67382113e..7b63b6713 100644 --- a/tests/pmd.at +++ b/tests/pmd.at @@ -516,3 +516,41 @@ p1 3 0 2 OVS_VSWITCHD_STOP(["/dpif_netdev|WARN|There is no PMD thread on core/d"]) AT_CLEANUP + +AT_SETUP([PMD - monitor threads]) +OVS_VSWITCHD_START( + [], [], [], [--dummy-numa 0,0]) +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) + +dnl The two devices are connected together externally using net.sock +AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd ofport_request=1 options:n_rxq=1 options:pstream=punix:$OVS_RUNDIR/net.sock]) +AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=dummy-pmd ofport_request=2 options:n_rxq=1 options:stream=unix:$OVS_RUNDIR/net.sock]) + +AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | parse_pmd_rxq_show], [0], [dnl +p1 0 0 0 +p2 0 0 0 +]) + +dnl Enable bfd with a very aggressive interval. This will make the monitor very +dnl busy, and uncover race conditions with the main thread. +AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=true bfd:min_rx=1 bfd:min_tx=1]) +AT_CHECK([ovs-vsctl set Interface p2 bfd:enable=true bfd:min_rx=1 bfd:min_tx=1]) + +AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p1 bfd_status:forwarding=true \ + -- wait-until Interface p2 bfd_status:forwarding=true]) + +dnl Trigger reconfiguration of the datapath +AT_CHECK([ovs-vsctl set Interface p1 options:n_rxq=2]) +AT_CHECK([ovs-vsctl set Interface p2 options:n_rxq=2]) + +dnl Make sure that reconfiguration succeded +AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | parse_pmd_rxq_show], [0], [dnl +p1 0 0 0 +p1 1 0 0 +p2 0 0 0 +p2 1 0 0 +]) + +dnl During reconfiguration some packets will be dropped. This is expected +OVS_VSWITCHD_STOP(["/dpif(monitor[[0-9]]\+)|WARN|dummy@ovs-dummy: execute [[0-9]]\+ failed/d"]) +AT_CLEANUP