On Tue, Aug 26, 2014 at 11:45 AM, Andy Zhou <[email protected]> wrote:
> On Mon, Aug 25, 2014 at 6:32 PM, Pravin Shelar <[email protected]> wrote:
>> On Mon, Aug 25, 2014 at 3:24 PM, Andy Zhou <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Andy Zhou <[email protected]>
>>> ---
>>> 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;
> }
> }
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
