[ovs-dev] [PATCH 12/23] datapath: fix possible memleak on destroy flow-table

Tonghao Zhang xiangxia.m.yue at gmail.com
Tue Aug 25 04:00:18 UTC 2020


On Fri, Aug 21, 2020 at 6:50 AM Greg Rose <gvrose8192 at gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue at gmail.com>
>
> Upstream commit:
>     commit 50b0e61b32ee890a75b4377d5fbe770a86d6a4c1
>     Author: Tonghao Zhang <xiangxia.m.yue at gmail.com>
>     Date:   Fri Nov 1 22:23:52 2019 +0800
>
>     net: openvswitch: fix possible memleak on destroy flow-table
>
>     When we destroy the flow tables which may contain the flow_mask,
>     so release the flow mask struct.
>
>     Signed-off-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
>     Tested-by: Greg Rose <gvrose8192 at gmail.com>
>     Acked-by: Pravin B Shelar <pshelar at ovn.org>
>     Signed-off-by: David S. Miller <davem at davemloft.net>
>
> Added additional compat layer fixup for WRITE_ONCE()
>
> Cc: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
Reviewed-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
> ---
>  datapath/flow_table.c                          | 186 +++++++++++++------------
>  datapath/linux/compat/include/linux/compiler.h |  13 ++
>  2 files changed, 111 insertions(+), 88 deletions(-)
>
> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index ca2efe9..bd05dd3 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> @@ -234,6 +234,74 @@ static int tbl_mask_array_realloc(struct flow_table *tbl, int size)
>         return 0;
>  }
>
> +static int tbl_mask_array_add_mask(struct flow_table *tbl,
> +                                  struct sw_flow_mask *new)
> +{
> +       struct mask_array *ma = ovsl_dereference(tbl->mask_array);
> +       int err, ma_count = READ_ONCE(ma->count);
> +
> +       if (ma_count >= ma->max) {
> +               err = tbl_mask_array_realloc(tbl, ma->max +
> +                                             MASK_ARRAY_SIZE_MIN);
> +               if (err)
> +                       return err;
> +
> +               ma = ovsl_dereference(tbl->mask_array);
> +       }
> +
> +       BUG_ON(ovsl_dereference(ma->masks[ma_count]));
> +
> +       rcu_assign_pointer(ma->masks[ma_count], new);
> +       WRITE_ONCE(ma->count, ma_count +1);
> +
> +       return 0;
> +}
> +
> +static void tbl_mask_array_del_mask(struct flow_table *tbl,
> +                                   struct sw_flow_mask *mask)
> +{
> +       struct mask_array *ma = ovsl_dereference(tbl->mask_array);
> +       int i, ma_count = READ_ONCE(ma->count);
> +
> +       /* Remove the deleted mask pointers from the array */
> +       for (i = 0; i < ma_count; i++) {
> +               if (mask == ovsl_dereference(ma->masks[i]))
> +                       goto found;
> +       }
> +
> +       BUG();
> +       return;
> +
> +found:
> +       WRITE_ONCE(ma->count, ma_count -1);
> +
> +       rcu_assign_pointer(ma->masks[i], ma->masks[ma_count -1]);
> +       RCU_INIT_POINTER(ma->masks[ma_count -1], NULL);
> +
> +       kfree_rcu(mask, rcu);
> +
> +       /* Shrink the mask array if necessary. */
> +       if (ma->max >= (MASK_ARRAY_SIZE_MIN * 2) &&
> +           ma_count <= (ma->max / 3))
> +               tbl_mask_array_realloc(tbl, ma->max / 2);
> +}
> +
> +/* Remove 'mask' from the mask list, if it is not needed any more. */
> +static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
> +{
> +       if (mask) {
> +               /* ovs-lock is required to protect mask-refcount and
> +                * mask list.
> +                */
> +               ASSERT_OVSL();
> +               BUG_ON(!mask->ref_count);
> +               mask->ref_count--;
> +
> +               if (!mask->ref_count)
> +                       tbl_mask_array_del_mask(tbl, mask);
> +       }
> +}
> +
>  int ovs_flow_tbl_init(struct flow_table *table)
>  {
>         struct table_instance *ti, *ufid_ti;
> @@ -280,7 +348,28 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu)
>         __table_instance_destroy(ti);
>  }
>
> -static void table_instance_destroy(struct table_instance *ti,
> +static void table_instance_flow_free(struct flow_table *table,
> +                                 struct table_instance *ti,
> +                                 struct table_instance *ufid_ti,
> +                                 struct sw_flow *flow,
> +                                 bool count)
> +{
> +       hlist_del_rcu(&flow->flow_table.node[ti->node_ver]);
> +       if (count)
> +               table->count--;
> +
> +       if (ovs_identifier_is_ufid(&flow->id)) {
> +               hlist_del_rcu(&flow->ufid_table.node[ufid_ti->node_ver]);
> +
> +               if (count)
> +                       table->ufid_count--;
> +       }
> +
> +       flow_mask_remove(table, flow->mask);
> +}
> +
> +static void table_instance_destroy(struct flow_table *table,
> +                                  struct table_instance *ti,
>                                    struct table_instance *ufid_ti,
>                                    bool deferred)
>  {
> @@ -297,13 +386,12 @@ static void table_instance_destroy(struct table_instance *ti,
>                 struct sw_flow *flow;
>                 struct hlist_head *head = &ti->buckets[i];
>                 struct hlist_node *n;
> -               int ver = ti->node_ver;
> -               int ufid_ver = ufid_ti->node_ver;
>
> -               hlist_for_each_entry_safe(flow, n, head, flow_table.node[ver]) {
> -                       hlist_del_rcu(&flow->flow_table.node[ver]);
> -                       if (ovs_identifier_is_ufid(&flow->id))
> -                               hlist_del_rcu(&flow->ufid_table.node[ufid_ver]);
> +               hlist_for_each_entry_safe(flow, n, head,
> +                                         flow_table.node[ti->node_ver]) {
> +
> +                       table_instance_flow_free(table, ti, ufid_ti,
> +                                                flow, false);
>                         ovs_flow_free(flow, deferred);
>                 }
>         }
> @@ -328,7 +416,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)
>
>         free_percpu(table->mask_cache);
>         kfree(rcu_dereference_raw(table->mask_array));
> -       table_instance_destroy(ti, ufid_ti, false);
> +       table_instance_destroy(table, ti, ufid_ti, false);
>  }
>
>  struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
> @@ -444,7 +532,7 @@ int ovs_flow_tbl_flush(struct flow_table *flow_table)
>         flow_table->count = 0;
>         flow_table->ufid_count = 0;
>
> -       table_instance_destroy(old_ti, old_ufid_ti, true);
> +       table_instance_destroy(flow_table, old_ti, old_ufid_ti, true);
>         return 0;
>
>  err_free_ti:
> @@ -722,51 +810,6 @@ static struct table_instance *table_instance_expand(struct table_instance *ti,
>         return table_instance_rehash(ti, ti->n_buckets * 2, ufid);
>  }
>
> -static void tbl_mask_array_del_mask(struct flow_table *tbl,
> -                                   struct sw_flow_mask *mask)
> -{
> -       struct mask_array *ma = ovsl_dereference(tbl->mask_array);
> -       int i, ma_count = READ_ONCE(ma->count);
> -
> -       /* Remove the deleted mask pointers from the array */
> -       for (i = 0; i < ma_count; i++) {
> -               if (mask == ovsl_dereference(ma->masks[i]))
> -                       goto found;
> -       }
> -
> -       BUG();
> -       return;
> -
> -found:
> -       WRITE_ONCE(ma->count, ma_count -1);
> -
> -       rcu_assign_pointer(ma->masks[i], ma->masks[ma_count -1]);
> -       RCU_INIT_POINTER(ma->masks[ma_count -1], NULL);
> -
> -       kfree_rcu(mask, rcu);
> -
> -       /* Shrink the mask array if necessary. */
> -       if (ma->max >= (MASK_ARRAY_SIZE_MIN * 2) &&
> -           ma_count <= (ma->max / 3))
> -               tbl_mask_array_realloc(tbl, ma->max / 2);
> -}
> -
> -/* Remove 'mask' from the mask list, if it is not needed any more. */
> -static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
> -{
> -       if (mask) {
> -               /* ovs-lock is required to protect mask-refcount and
> -                * mask list.
> -                */
> -               ASSERT_OVSL();
> -               BUG_ON(!mask->ref_count);
> -               mask->ref_count--;
> -
> -               if (!mask->ref_count)
> -                       tbl_mask_array_del_mask(tbl, mask);
> -       }
> -}
> -
>  /* Must be called with OVS mutex held. */
>  void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow)
>  {
> @@ -774,17 +817,7 @@ void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow)
>         struct table_instance *ufid_ti = ovsl_dereference(table->ufid_ti);
>
>         BUG_ON(table->count == 0);
> -       hlist_del_rcu(&flow->flow_table.node[ti->node_ver]);
> -       table->count--;
> -       if (ovs_identifier_is_ufid(&flow->id)) {
> -               hlist_del_rcu(&flow->ufid_table.node[ufid_ti->node_ver]);
> -               table->ufid_count--;
> -       }
> -
> -       /* RCU delete the mask. 'flow->mask' is not NULLed, as it should be
> -        * accessible as long as the RCU read lock is held.
> -        */
> -       flow_mask_remove(table, flow->mask);
> +       table_instance_flow_free(table, ti, ufid_ti, flow, true);
>  }
>
>  static struct sw_flow_mask *mask_alloc(void)
> @@ -827,29 +860,6 @@ static struct sw_flow_mask *flow_mask_find(const struct flow_table *tbl,
>         return NULL;
>  }
>
> -static int tbl_mask_array_add_mask(struct flow_table *tbl,
> -                                  struct sw_flow_mask *new)
> -{
> -       struct mask_array *ma = ovsl_dereference(tbl->mask_array);
> -       int err, ma_count = READ_ONCE(ma->count);
> -
> -       if (ma_count >= ma->max) {
> -               err = tbl_mask_array_realloc(tbl, ma->max +
> -                                             MASK_ARRAY_SIZE_MIN);
> -               if (err)
> -                       return err;
> -
> -               ma = ovsl_dereference(tbl->mask_array);
> -       }
> -
> -       BUG_ON(ovsl_dereference(ma->masks[ma_count]));
> -
> -       rcu_assign_pointer(ma->masks[ma_count], new);
> -       WRITE_ONCE(ma->count, ma_count +1);
> -
> -       return 0;
> -}
> -
>  /* Add 'mask' into the mask list, if it is not already there. */
>  static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
>                             const struct sw_flow_mask *new)
> diff --git a/datapath/linux/compat/include/linux/compiler.h b/datapath/linux/compat/include/linux/compiler.h
> index 65f3ba6..e15c61d 100644
> --- a/datapath/linux/compat/include/linux/compiler.h
> +++ b/datapath/linux/compat/include/linux/compiler.h
> @@ -15,4 +15,17 @@
>  #define READ_ONCE(x) (x)
>  #endif
>
> +#ifndef WRITE_ONCE
> +#define __WRITE_ONCE(x, val)                                           \
> +do {                                                                   \
> +       *(volatile typeof(x) *)&(x) = (val);                            \
> +} while (0)
> +
> +#define WRITE_ONCE(x, val)                                             \
> +do {                                                                   \
> +       __WRITE_ONCE(x, val);                                           \
> +} while (0)
> +#endif
> +
> +
>  #endif
> --
> 1.8.3.1
>


-- 
Best regards, Tonghao


More information about the dev mailing list