[ovs-discuss] one patch was omitted to be pushed to branch-2.3---datapath: Fix recirc bug where skb is double freed

[Alex Wang]

Fwd to Andy,~

On Sun, May 17, 2015 at 4:29 AM, zhangzhiming <zhangzhiming at yunshan.net.cn>
wrote:

>   Hi,
>
> I found one patch was omitted to be pushed to branch-2.3, which leads to
> double freed skb.
> Could someone to confirm the patch and submit it to branch-2.3?
> Thanks!
>
> Here is the patch information:
>
>
> commit 867e37ba00091b3e319c4c47c1598f1ae84dd32e
> Author: Andy Zhou <azhou at nicira.com>
> Date:   Mon Aug 25 15:18:19 2014 -0700
>
>     datapath: Fix recirc bug where skb is double freed.
>
>     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>
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index ad22467..7f25553 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
>
> @@ -820,19 +829,9 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>                  * continue on with the rest of the action list. */
>                 if (!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);
> +       }
>
>         flow_key_set_recirc_id(skb, nla_get_u32(a));
>         ovs_dp_process_packet(skb, true);
>
> @@ -897,6 +896,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:
>
> ------------------------------
> Jeremy Zhang
>
> _______________________________________________
> discuss mailing list
> discuss at openvswitch.org
> http://openvswitch.org/mailman/listinfo/discuss
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/ovs-discuss/attachments/20150517/31ba997c/attachment-0002.html>