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

Pravin Shelar pshelar at nicira.com
Wed Aug 27 00:15:34 UTC 2014


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



More information about the dev mailing list