[ovs-dev] ofproto: Fix crash on PACKET_OUT due to recursive locking after upcall.
diff mbox series

Message ID 20191101212439.5110-1-i.maximets@ovn.org
State Accepted
Delegated to: Ilya Maximets
Headers show
Series
  • [ovs-dev] ofproto: Fix crash on PACKET_OUT due to recursive locking after upcall.
Related show

Commit Message

Ilya Maximets Nov. 1, 2019, 9:24 p.m. UTC
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>
---

Previous discussion about implementation:
https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362082.html

I'm not able to reproduce the original issue, so I can not test if
this patch fixes it.

 ofproto/ofproto-dpif.c     | 40 ++++++++++++++++++++++++++++----------
 ofproto/ofproto-provider.h | 12 +++++++++---
 ofproto/ofproto.c          | 29 +++++++++++++++++++++++----
 3 files changed, 64 insertions(+), 17 deletions(-)

Comments

Ilya Maximets Nov. 21, 2019, 6:31 p.m. UTC | #1
On 01.11.2019 22:24, Ilya Maximets wrote:
> 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>
> ---
> 
> Previous discussion about implementation:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362082.html
> 
> I'm not able to reproduce the original issue, so I can not test if
> this patch fixes it.
> 
>  ofproto/ofproto-dpif.c     | 40 ++++++++++++++++++++++++++++----------
>  ofproto/ofproto-provider.h | 12 +++++++++---
>  ofproto/ofproto.c          | 29 +++++++++++++++++++++++----
>  3 files changed, 64 insertions(+), 17 deletions(-)


Hi Ben,

Could you, please, take a look at this patch?

Best regards, Ilya Maximets.
Sathya Perla via dev Nov. 22, 2019, 2:09 a.m. UTC | #2
Hi Ben,

I was not able to reproduce this issue in ovs sandbox, but able to test this patch on local testbed which had this issue and the patch works.

Thanks & Regards,
Anil Kumar.

-----Original Message-----
From: Ilya Maximets <i.maximets@ovn.org> 
Sent: Friday, 22 November, 2019 12:02 AM
To: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Ben Pfaff <blp@ovn.org>
Cc: Anil Kumar Koli <anilkumar.k@altencalsoftlabs.com>
Subject: Re: [PATCH] ofproto: Fix crash on PACKET_OUT due to recursive locking after upcall.

On 01.11.2019 22:24, Ilya Maximets wrote:
> 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.h
> tml
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
> 
> Previous discussion about implementation:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362082.html
> 
> I'm not able to reproduce the original issue, so I can not test if 
> this patch fixes it.
> 
>  ofproto/ofproto-dpif.c     | 40 ++++++++++++++++++++++++++++----------
>  ofproto/ofproto-provider.h | 12 +++++++++---
>  ofproto/ofproto.c          | 29 +++++++++++++++++++++++----
>  3 files changed, 64 insertions(+), 17 deletions(-)


Hi Ben,

Could you, please, take a look at this patch?

Best regards, Ilya Maximets.
Ben Pfaff Nov. 25, 2019, 9:45 p.m. UTC | #3
On Fri, Nov 01, 2019 at 10:24:39PM +0100, Ilya Maximets wrote:
> 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.

Thank you very much for the fix.  It is simpler and cleaner than I
expected.  It looks correct to me and it passes all of the tests for me
locally.

Acked-by: Ben Pfaff <blp@ovn.org>
Ilya Maximets Nov. 26, 2019, 2:42 p.m. UTC | #4
On 25.11.2019 22:45, Ben Pfaff wrote:
> On Fri, Nov 01, 2019 at 10:24:39PM +0100, Ilya Maximets wrote:
>> 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.
> 
> Thank you very much for the fix.  It is simpler and cleaner than I
> expected.  It looks correct to me and it passes all of the tests for me
> locally.
> 
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks!  Applied to master and backported down to 2.7.

There was a big refactoring between branches 2.6 and 2.7, so this
patch could not be applied to 2.6.  However, IIUC, handle_packet_out()
on this branch doesn't take 'ofproto_mutex' at all.  Looks a bit dangerous.

Best regards, Ilya Maximets.

Patch
diff mbox series

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c35ec3e61..0466952e2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4827,12 +4827,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);
@@ -4841,22 +4842,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;
 }
 
@@ -6529,6 +6548,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,
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index c980e6bff..e27164ac0 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1376,9 +1376,15 @@  struct ofproto_class {
      * packet_xlate_revert() calls have to be made in reverse order. */
     void (*packet_xlate_revert)(struct ofproto *, struct ofproto_packet_out *);
 
-    /* Executes the datapath actions, translation side-effects, and stats as
-     * produced by ->packet_xlate().  The caller retains ownership of 'opo'.
-     */
+    /* Translates side-effects, and stats as produced by ->packet_xlate().
+     * Prepares to execute datapath actions.  The caller retains ownership
+     * of 'opo'. */
+    void (*packet_execute_prepare)(struct ofproto *,
+                                   struct ofproto_packet_out *opo);
+
+    /* Executes the datapath actions.  The caller retains ownership of 'opo'.
+     * Should be called after successful packet_execute_prepare().
+     * No-op if called after packet_xlate_revert(). */
     void (*packet_execute)(struct ofproto *, struct ofproto_packet_out *opo);
 
     /* Changes the OpenFlow IP fragment handling policy to 'frag_handling',
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3aaa45a9b..53761ab71 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3634,10 +3634,21 @@  ofproto_packet_out_revert(struct ofproto *ofproto,
     ofproto->ofproto_class->packet_xlate_revert(ofproto, opo);
 }
 
+static void
+ofproto_packet_out_prepare(struct ofproto *ofproto,
+                           struct ofproto_packet_out *opo)
+    OVS_REQUIRES(ofproto_mutex)
+{
+    ofproto->ofproto_class->packet_execute_prepare(ofproto, opo);
+}
+
+/* Execution of packet_out action in datapath could end up in upcall with
+ * subsequent flow translations and possible rule modifications.
+ * So, the caller should not hold 'ofproto_mutex'. */
 static void
 ofproto_packet_out_finish(struct ofproto *ofproto,
                           struct ofproto_packet_out *opo)
-    OVS_REQUIRES(ofproto_mutex)
+    OVS_EXCLUDED(ofproto_mutex)
 {
     ofproto->ofproto_class->packet_execute(ofproto, opo);
 }
@@ -3680,10 +3691,13 @@  handle_packet_out(struct ofconn *ofconn, const struct ofp_header *oh)
     opo.version = p->tables_version;
     error = ofproto_packet_out_start(p, &opo);
     if (!error) {
-        ofproto_packet_out_finish(p, &opo);
+        ofproto_packet_out_prepare(p, &opo);
     }
     ovs_mutex_unlock(&ofproto_mutex);
 
+    if (!error) {
+        ofproto_packet_out_finish(p, &opo);
+    }
     ofproto_packet_out_uninit(&opo);
     return error;
 }
@@ -8202,7 +8216,7 @@  do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                     } else if (be->type == OFPTYPE_GROUP_MOD) {
                         ofproto_group_mod_finish(ofproto, &be->ogm, &req);
                     } else if (be->type == OFPTYPE_PACKET_OUT) {
-                        ofproto_packet_out_finish(ofproto, &be->opo);
+                        ofproto_packet_out_prepare(ofproto, &be->opo);
                     }
                 }
                 if (error) {
@@ -8213,7 +8227,7 @@  do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
                 /* Send error referring to the original message. */
                 ofconn_send_error(ofconn, be->msg, error);
                 error = OFPERR_OFPBFC_MSG_FAILED;
-                
+
                 /* Revert.  Undo all the changes made above. */
                 LIST_FOR_EACH_REVERSE_CONTINUE (be, node, &bundle->msg_list) {
                     if (be->type == OFPTYPE_FLOW_MOD) {
@@ -8232,6 +8246,13 @@  do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags)
         ovs_mutex_unlock(&ofproto_mutex);
     }
 
+    /* Executing remaining datapath actions. */
+    LIST_FOR_EACH (be, node, &bundle->msg_list) {
+        if (be->type == OFPTYPE_PACKET_OUT) {
+            ofproto_packet_out_finish(ofproto, &be->opo);
+        }
+    }
+
     /* The bundle is discarded regardless the outcome. */
     ofp_bundle_remove__(ofconn, bundle);
     return error;