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

Andy Zhou azhou at nicira.com
Tue Aug 26 18:45:36 UTC 2014


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?

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



More information about the dev mailing list