[ovs-dev] [PATCH] datapath: Fix recirc bug where skb is double freed.

[Andy Zhou]

On Tue, Aug 26, 2014 at 12:39 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Tue, Aug 26, 2014 at 11:45 AM, Andy Zhou <azhou at nicira.com> wrote:
>> On Mon, Aug 25, 2014 at 6:32 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>>> On Mon, Aug 25, 2014 at 3:24 PM, Andy Zhou <azhou at 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 <jpettit at nicira.com>
>>>> Signed-off-by: Andy Zhou <azhou at 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?
>>
> Right, But it also returns ENOMEM which is same as skb_clone() error,
> so we can return 0 if ovs_flow_key_update() returns ENOMEM.

I am not sure why it some times return ENOMEM, should it always be EINVAL?
If you agree it should be fixed, I will send a separate patch.

Assume ovs_flow_key_update() returns EINVAL for error, we should
reflect this error
code to its caller, right?

>
>> There is an extra kfree in execute_recirc(), so the following
>> additional patch is needed.
>>
>
> Current code is fine.
> If it is last action do_execute_actions() returns without freeing skb
> and if it not the last action execute_recirc() is freeing clone skb.

Hmmm, The intention is to ask the caller to free the skb passed in.
It is probably better to do ovs_flow_key_update() before skb_clone, so
the error will be caught earlier.  I will send out the next version if
this sounds
right.

>
>> 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;
>>                 }
>>         }