On Mon, May 12, 2014 at 10:46 PM, Simon Horman <ho...@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 <je...@nicira.com> wrote: >> >> > On Mon, May 12, 2014 at 4:06 PM, Daniele Di Proietto >> > <ddiproie...@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 <ho...@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 <ho...@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(). _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
