On Mon, Aug 25, 2014 at 6:32 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Mon, Aug 25, 2014 at 3:24 PM, Andy Zhou <az...@nicira.com> wrote:
>> If recirc action is the last action of a action list, the SKB triggers
>> the recirc will be freed twice. This patch fixes this bug.
>>
>> Reported-by: Justin Pettit <jpet...@nicira.com>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>> ---
>> datapath/actions.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index d70348e..2bfc527 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -897,6 +897,12 @@ static int do_execute_actions(struct datapath *dp,
>> struct sk_buff *skb,
>>
>> case OVS_ACTION_ATTR_RECIRC:
>> err = execute_recirc(dp, skb, a, rem);
>> + if (last_action(a, rem)) {
>> + /* If this is the last action, the skb has
>> + * been consumed or freed.
>> + * Return immediately. */
>> + return err;
>> + }
>
> I think better way is not return error from execute_recirc() action.
> Currently execute_recirc() has odd behavior, it return 0 if skb_clone
> fails, but return error if ovs_flow_key_update() fails. by not
> returning error from execute_recirc(), action execute can return on
> just last action.
>
An error indicates whether we should continue to process the current
actions list. Failed skb_clone should
not, so return 0 should be fine. On the other hand
ovs_flow_key_update() fail would indicate a serious problem
that we should abort. What do you think?
There is an extra kfree in execute_recirc(), so the following
additional patch is needed.
diff --git a/datapath/actions.c b/datapath/actions.c
index 2bfc527..c22e818 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -825,7 +825,6 @@ static int execute_recirc(struct datapath *dp,
struct sk_buff *skb,
if (!is_skb_flow_key_valid(skb)) {
err = ovs_flow_key_update(skb, OVS_CB(skb)->pkt_key);
if (err) {
- kfree_skb(skb);
return err;
}
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
