On Tue, Aug 26, 2014 at 12:39 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Tue, Aug 26, 2014 at 11:45 AM, Andy Zhou <az...@nicira.com> wrote:
>> 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?
>>
> 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;
>> }
>> }
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
