netdev - Re: [PATCH v3] datapath: Increase maximum allocation size of action list.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Fri, 22 Feb 2013 17:08:50 -0800
From:	Pravin Shelar <pshelar@...ira.com>
To:	netdev@...r.kernel.org
Subject: Re: [PATCH v3] datapath: Increase maximum allocation size of action list.

Please ignore this patch.
This patch is for openvswitch mailing list.

On Fri, Feb 22, 2013 at 5:03 PM, Pravin B Shelar <pshelar@...ira.com> wrote:
> The switch to flow based tunneling increased the size of each output
> action in the flow action list.  In extreme cases, this can result
> in the action list exceeding the maximum buffer size.
>
> This doubles the maximum buffer size to compensate for the increase
> in action size.  In the common case, most allocations will be
> less than a page and those uses kmalloc.  Therefore, for the majority
> of situations, this will have no impact.
>
> Bug #15203
> Signed-off-by: Pravin B Shelar <pshelar@...ira.com>
> ---
> Fixed according to commnets from Jesse.
> v2-v3:
>  - Use size for buf_size.
>  - Fixed size check in ovs_flow_actions_free().
>
> v1-v2:
> Fixes some issues noticed in earlier patch.
> ---
>  datapath/datapath.c |    8 ++++----
>  datapath/flow.c     |   19 +++++++++++++++++--
>  datapath/flow.h     |    5 ++++-
>  3 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index fc5d2de..bd2d57b 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -434,10 +434,10 @@ static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa, int attr_le
>         int next_offset = offsetof(struct sw_flow_actions, actions) +
>                                         (*sfa)->actions_len;
>
> -       if (req_size <= (ksize(*sfa) - next_offset))
> +       if (req_size <= ((*sfa)->buf_size - next_offset))
>                 goto out;
>
> -       new_acts_size = ksize(*sfa) * 2;
> +       new_acts_size = (*sfa)->buf_size * 2;
>
>         if (new_acts_size > MAX_ACTIONS_BUFSIZE) {
>                 if ((MAX_ACTIONS_BUFSIZE - next_offset) < req_size)
> @@ -451,7 +451,7 @@ static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa, int attr_le
>
>         memcpy(acts->actions, (*sfa)->actions, (*sfa)->actions_len);
>         acts->actions_len = (*sfa)->actions_len;
> -       kfree(*sfa);
> +       ovs_flow_actions_free(*sfa);
>         *sfa = acts;
>
>  out:
> @@ -1292,7 +1292,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
>         return 0;
>
>  err_kfree:
> -       kfree(acts);
> +       ovs_flow_actions_free(acts);
>  error:
>         return error;
>  }
> diff --git a/datapath/flow.c b/datapath/flow.c
> index b14229f..b6bb7a7 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -207,14 +207,29 @@ struct sw_flow_actions *ovs_flow_actions_alloc(int size)
>         if (size > MAX_ACTIONS_BUFSIZE)
>                 return ERR_PTR(-EINVAL);
>
> -       sfa = kmalloc(sizeof(*sfa) + size, GFP_KERNEL);
> +       size += sizeof(*sfa);
> +       if (size <= MAX_ACTIONS_BUFSIZE_KMALLOC)
> +               sfa = kmalloc(size, GFP_KERNEL);
> +       else
> +               sfa = vmalloc(size);
> +
>         if (!sfa)
>                 return ERR_PTR(-ENOMEM);
>
>         sfa->actions_len = 0;
> +       sfa->buf_size = size;
> +
>         return sfa;
>  }
>
> +void ovs_flow_actions_free(struct sw_flow_actions *sfa)
> +{
> +       if (sfa->buf_size <= MAX_ACTIONS_BUFSIZE_KMALLOC)
> +               kfree(sfa);
> +       else
> +               vfree(sfa);
> +}
> +
>  struct sw_flow *ovs_flow_alloc(void)
>  {
>         struct sw_flow *flow;
> @@ -437,7 +452,7 @@ static void rcu_free_acts_callback(struct rcu_head *rcu)
>  {
>         struct sw_flow_actions *sf_acts = container_of(rcu,
>                         struct sw_flow_actions, rcu);
> -       kfree(sf_acts);
> +       ovs_flow_actions_free(sf_acts);
>  }
>
>  /* Schedules 'sf_acts' to be freed after the next RCU grace period.
> diff --git a/datapath/flow.h b/datapath/flow.h
> index 6949640..3b08ea6 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -37,6 +37,7 @@ struct sk_buff;
>  struct sw_flow_actions {
>         struct rcu_head rcu;
>         u32 actions_len;
> +       int buf_size;
>         struct nlattr actions[];
>  };
>
> @@ -151,6 +152,7 @@ void ovs_flow_deferred_free(struct sw_flow *);
>  void ovs_flow_free(struct sw_flow *);
>
>  struct sw_flow_actions *ovs_flow_actions_alloc(int actions_len);
> +void ovs_flow_actions_free(struct sw_flow_actions *sfa);
>  void ovs_flow_deferred_free_acts(struct sw_flow_actions *);
>
>  int ovs_flow_extract(struct sk_buff *, u16 in_port, struct sw_flow_key *,
> @@ -194,7 +196,8 @@ int ovs_flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
>  int ovs_flow_metadata_from_nlattrs(struct sw_flow *flow, int key_len,
>                                    const struct nlattr *attr);
>
> -#define MAX_ACTIONS_BUFSIZE    (16 * 1024)
> +#define MAX_ACTIONS_BUFSIZE            (32 * 1024)
> +#define MAX_ACTIONS_BUFSIZE_KMALLOC    PAGE_SIZE
>  #define TBL_MIN_BUCKETS                1024
>
>  struct flow_table {
> --
> 1.7.1
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html