[ovs-dev] [PATCH net-next v4 08/10] net: openvswitch: fix possible memleak on destroy flow-table

Tonghao Zhang xiangxia.m.yue at gmail.com
Mon Oct 28 06:49:00 UTC 2019


On Thu, Oct 24, 2019 at 3:14 PM Pravin Shelar <pshelar at ovn.org> wrote:
>
> On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m.yue at gmail.com> wrote:
> >
> > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar at ovn.org> wrote:
> > >
> ...
>
> > > > >
> > > Sure, I can review it, Can you send the patch inlined in mail?
> > >
> > > Thanks.
> > diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> > index 5df5182..5b20793 100644
> > --- a/net/openvswitch/flow_table.c
> > +++ b/net/openvswitch/flow_table.c
> > @@ -257,10 +257,75 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu)
> >         __table_instance_destroy(ti);
> >  }
> >
> > -static void table_instance_destroy(struct table_instance *ti,
> > -                                  struct table_instance *ufid_ti,
> > +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);
> > +       }
> > +}
> > +
> > +static void table_instance_remove(struct flow_table *table, struct
> > sw_flow *flow)
> > +{
> > +       struct table_instance *ti = ovsl_dereference(table->ti);
> > +       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);
> > +}
> > +
> > +static void table_instance_destroy(struct flow_table *table,
> >                                    bool deferred)
> >  {
> > +       struct table_instance *ti = ovsl_dereference(table->ti);
> > +       struct table_instance *ufid_ti = ovsl_dereference(table->ufid_ti);
> >         int i;
> >
> >         if (!ti)
> > @@ -274,13 +339,9 @@ 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_remove(table, flow);
> >                         ovs_flow_free(flow, deferred);
> >                 }
> >         }
> > @@ -300,12 +361,9 @@ static void table_instance_destroy(struct
> > table_instance *ti,
> >   */
> >  void ovs_flow_tbl_destroy(struct flow_table *table)
> >  {
> > -       struct table_instance *ti = rcu_dereference_raw(table->ti);
> > -       struct table_instance *ufid_ti = rcu_dereference_raw(table->ufid_ti);
> > -
> >         free_percpu(table->mask_cache);
> >         kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
> > -       table_instance_destroy(ti, ufid_ti, false);
> > +       table_instance_destroy(table, false);
> >  }
> >
> >  struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
> > @@ -400,10 +458,9 @@ static struct table_instance
> > *table_instance_rehash(struct table_instance *ti,
> >         return new_ti;
> >  }
> >
> > -int ovs_flow_tbl_flush(struct flow_table *flow_table)
> > +int ovs_flow_tbl_flush(struct flow_table *table)
> >  {
> > -       struct table_instance *old_ti, *new_ti;
> > -       struct table_instance *old_ufid_ti, *new_ufid_ti;
> > +       struct table_instance *new_ti, *new_ufid_ti;
> >
> >         new_ti = table_instance_alloc(TBL_MIN_BUCKETS);
> >         if (!new_ti)
> > @@ -412,16 +469,12 @@ int ovs_flow_tbl_flush(struct flow_table *flow_table)
> >         if (!new_ufid_ti)
> >                 goto err_free_ti;
> >
> > -       old_ti = ovsl_dereference(flow_table->ti);
> > -       old_ufid_ti = ovsl_dereference(flow_table->ufid_ti);
> > +       table_instance_destroy(table, true);
> >
> This would destroy running table causing unnecessary flow miss. Lets
> keep current scheme of setting up new table before destroying current
> one.
>
> > -       rcu_assign_pointer(flow_table->ti, new_ti);
> > -       rcu_assign_pointer(flow_table->ufid_ti, new_ufid_ti);
> > -       flow_table->last_rehash = jiffies;
> > -       flow_table->count = 0;
> > -       flow_table->ufid_count = 0;
> > +       rcu_assign_pointer(table->ti, new_ti);
> > +       rcu_assign_pointer(table->ufid_ti, new_ufid_ti);
> > +       table->last_rehash = jiffies;
> >
> > -       table_instance_destroy(old_ti, old_ufid_ti, true);
> >         return 0;
> >
> >  err_free_ti:
> > @@ -700,69 +753,10 @@ 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)
> >  {
> > -       struct table_instance *ti = ovsl_dereference(table->ti);
> > -       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_remove(table, flow);
> Can you just rename table_instance_remove() to ovs_flow_tbl_remove()?

diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 5df5182..4871ab8 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -210,6 +210,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;
@@ -257,7 +325,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_remove(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)
 {
@@ -274,13 +363,11 @@ 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_remove(table, ti, ufid_ti, flow, false);
                        ovs_flow_free(flow, deferred);
                }
        }
@@ -305,7 +392,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)

        free_percpu(table->mask_cache);
        kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
-       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,
@@ -421,7 +508,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:
@@ -700,51 +787,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)
 {
@@ -752,17 +794,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_remove(table, ti, ufid_ti, flow, true);
 }

 static struct sw_flow_mask *mask_alloc(void)
@@ -805,29 +837,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)


More information about the dev mailing list