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

Pravin Shelar pshelar at nicira.com
Tue Aug 26 19:39:22 UTC 2014


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.

> 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.

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



More information about the dev mailing list