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

[Andy Zhou]

Pushed with the fix suggested. Thanks.

On Tue, Aug 26, 2014 at 5:15 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Tue, Aug 26, 2014 at 3:36 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 | 26 ++++++++++++++++----------
>>  1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index ad22467..3b39d6f 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -809,7 +809,16 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>                           const struct nlattr *a, int rem)
>>  {
>>         struct sw_flow_key recirc_key;
>> -       int err;
>> +
>> +       if (!is_skb_flow_key_valid(skb)) {
>> +               int err;
>> +
>> +               err = ovs_flow_key_update(skb, OVS_CB(skb)->pkt_key);
>> +               if (err)
>> +                       return err;
>> +
>> +       }
>> +       BUG_ON(!is_skb_flow_key_valid(skb));
>>
>>         if (!last_action(a, rem)) {
>>                 /* Recirc action is the not the last action
>> @@ -822,15 +831,6 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>                         return 0;
>>         }
>>
>> -       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;
>> -               }
>> -       }
>> -       BUG_ON(!is_skb_flow_key_valid(skb));
>> -
>>         if (!last_action(a, rem))
>>                 flow_key_clone(skb, &recirc_key);
>
> Now these are two identical cases, we can just merge them.
>
> Otherwise looks good.
>
> Acked-by: Pravin B Shelar <pshelar at nicira.com>
>
>
>>
>> @@ -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;
>> +                       }
>>                         break;
>>
>>                 case OVS_ACTION_ATTR_SET:
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev