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

[Simon Horman]

On Tue, May 13, 2014 at 01:42:47PM -0700, Jesse Gross wrote:
> On Mon, May 12, 2014 at 10:46 PM, Simon Horman <horms at verge.net.au> wrote:
> > 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>
> 
> Thanks, this version looks good to me.
> 
> I made one small adjustment before I applied this. On error paths, I
> think it is more appropriate to use kfree_skb() instead of
> consume_skb() so that drops can be more easily flagged. I made that
> change in execute_recirc().

Thanks. Of course I am perfectly fine with that change.

I will go ahead and send a revised version of patch #2:
sample actions without side effects.