[ovs-dev] [PATCH v2 1/2] datapath: Free skb(s) on recirculation error

[Simon Horman]

On Mon, May 12, 2014 at 04:32:29PM -0700, Daniele Di Proietto wrote:
> You’re right, of course.
> 
> Sorry I didn’t see it the first time.
> 
> On May 12, 2014, at 4:28 PM, Jesse Gross <jesse at nicira.com> wrote:
> 
> > On Mon, May 12, 2014 at 4:06 PM, Daniele Di Proietto
> > <ddiproietto at vmware.com> wrote:
> >> If I understand correctly, the new code frees the skb only if execute_recirc() _returns_ an error and this can happen only if ovs_flow_extract() returns an error, in which case recirc_skb only gets freed once, but maybe I’m wrong.
> > 
> > I agree that there is a problem currently with the error case and that
> > this fixes it. However, the current code also returns from the
> > function on success if this is the last action, which this removes.
> > This cause us to both free the packet during the course of the
> > (successful) recirculation and later down in the original caller. In
> > the last_action case, there is only a single skb so this causes a
> > double free

Thanks.

I had made the entirely incorrect assumption that execute_recirc() never
consumes the skb. Here is a fresh attempt at resolving this problem.

I will defer reposting the second patch of this series until
we have ironed out this one.

From: Simon Horman <horms at verge.net.au>

[PATCH v2.1] datapath: Free skb(s) on recirculation error

This patch attempts to ensure that skb(s) are always freed (once)
if if an error occurs in execute_recirc(). It does so in two ways:

1. Ensure that execute_recirc() always consimes skb passed to it.
   Specifically, free the skb if the call to ovs_flow_extract() fails.

2. Return from the recirc case in execute_recirc() whenever
   the skb has not been cloned immediately above.

   This is only the case if the action is both the last action and the
   keep_skb parameter of execute_recirc is not true.  As it is the last
   action and the skb is consumed one way or another by execute_recirc() it
   is correct to return here.  In particular this avoids the possibility of
   the skb, which has been consumed by execute_recirc() from being freed.

   Conversely if this is not the case then the skb has been cloned
   and the clone has been consumed by execute_recirc().
   This leads to three sub-cases:
   * If execute_recirc() returned an error code then the original skb
     will be freed by the error handling code below the case statement in
     do_execute_actions().
   * If this is not the last action then action processing will continue,
     using the original skb.
   * If this is the last action then it must also be the case that keep_skb
     is true (otherwise the skb would not have been cloned). Thus
     do_execute_actions() will return without freeing the original skb.

Signed-off-by: Simon Horman <horms at verge.net.au>

---
v2.1
* As pointed out buy Jesse Gross
  - Rework to avoid double free if there is no error but the action is the
    last action

v2
* First post
---
 datapath/actions.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index 7fe2f54..8383a84 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -530,8 +530,10 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
 	int err;
 
 	err = ovs_flow_extract(skb, p->port_no, &recirc_key);
-	if (err)
+	if (err) {
+		consume_skb(skb);
 		return err;
+	}
 
 	recirc_key.ovs_flow_hash = hash;
 	recirc_key.recirc_id = nla_get_u32(a);
@@ -596,7 +598,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 
 			err = execute_recirc(dp, recirc_skb, a);
 
-			if (last_action || err)
+			if (recirc_skb == skb)
 				return err;
 
 			break;
-- 
1.8.5.2