[ovs-dev,v3] odp-execute: Skip processing actions when batch is emptied
diff mbox series

Message ID AM5PR0701MB2961C3A4D9E0F6931549649BA4380@AM5PR0701MB2961.eurprd07.prod.outlook.com
State Accepted
Headers show
Series
  • [ovs-dev,v3] odp-execute: Skip processing actions when batch is emptied
Related show

Commit Message

Vishal Deep Ajmera Nov. 30, 2017, 5:37 a.m. UTC
Today in OVS, when errors are encountered during the execution
of an action the entire batch of packets may be deleted (for e.g.
in processing push_tnl_action, if the port is not found in the
port_cache of PMD). The remaining actions continue to be executed
even though there are no packets to be processed.

It is assumed that the code dealing with each action checks that
the batch is not empty before executing. Crashes may occur if the
assumption is not met.

The patch makes OVS skip processing of further actions from the
action-set once a batch is emptied. Doing so centralizes the check
in one place and avoids the possibility of crashes.

This change DOES NOT fix any existing bug in the code, only a
precautionary measure to avoid crashes if new actions does not
take care of empty batches.

Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
---
lib/odp-execute.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

--
1.9.1

Comments

Ben Pfaff Nov. 30, 2017, 7:01 p.m. UTC | #1
On Thu, Nov 30, 2017 at 05:37:57AM +0000, Vishal Deep Ajmera wrote:
> Today in OVS, when errors are encountered during the execution
> of an action the entire batch of packets may be deleted (for e.g.
> in processing push_tnl_action, if the port is not found in the
> port_cache of PMD). The remaining actions continue to be executed
> even though there are no packets to be processed.
> 
> It is assumed that the code dealing with each action checks that
> the batch is not empty before executing. Crashes may occur if the
> assumption is not met.
> 
> The patch makes OVS skip processing of further actions from the
> action-set once a batch is emptied. Doing so centralizes the check
> in one place and avoids the possibility of crashes.
> 
> This change DOES NOT fix any existing bug in the code, only a
> precautionary measure to avoid crashes if new actions does not
> take care of empty batches.
> 
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>

Thanks.

I applied this to master.

Patch
diff mbox series

diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 2d20cd5..1090fa8 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -686,9 +686,12 @@  odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,

                 dp_execute_action(dp, batch, a, may_steal);

-                if (last_action) {
-                    /* We do not need to free the packets. dp_execute_actions()
-                     * has stolen them */
+                if (last_action || batch->count == 0) {
+                    /* We do not need to free the packets.
+                     * Either dp_execute_actions() has stolen them
+                     * or the batch is freed due to errors. In either
+                     * case we do not need to execute further actions.
+                     */
                     return;
                 }
             }