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 it probably makes sense to have execute_recirc() take ownership of the skb, for the sake of clarity. On May 12, 2014, at 3:48 PM, Jesse Gross <je...@nicira.com> wrote: > On Sat, May 10, 2014 at 7:46 PM, Simon Horman <ho...@verge.net.au> wrote: >> It is my understanding that on error execute_recirc() does not free the >> skb passed to it. >> >> Assuming this is true then on error skb should always be freed >> if an error occurs in execute_recirc(). >> >> And if recirc_skb differs from skb, because it is a clone of skb, >> then it should also be freed. >> >> Signed-off-by: Simon Horman <ho...@verge.net.au> > > I think this can introduce a double free in the normal case where > execute_recirc() and is the last action. In this case, we free the skb > in execute_recirc() and then continue down to the bottom of the > do_execute_actions() since we are no longer returning and free the > (same) skb again. > > It may make sense to just have execute_recirc() always take ownership > of the packet and free the skb on error itself. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=Eg%2FfHxEC3iuh2WnriHo1AveJd7tB9j1JgPoHg%2FGJnl4%3D%0A&s=122f5695e4138850c31c68726cb32337f48b293563a03af33dcc2d759c58c7ef _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
