[ovs-dev] [PATCH V3 11/24] datapath: fix possible memleak on destroy flow-table

Gregory Rose gvrose8192 at gmail.com
Fri Oct 2 16:43:52 UTC 2020



On 9/29/2020 2:43 PM, Yi-Hung Wei wrote:
> On Wed, Sep 16, 2020 at 10:33 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>
>> Reviewed-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
>> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
>> ---
>>   datapath/flow_table.c                         | 186 +++++++++---------
>>   .../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 ca2efe94d..bd05dd394 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 65f3ba6f4..e15c61d2a 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
> 
> Hi Greg,
> 
> Just a minor a nit on the WRITE_ONCE() compatibility code, in the
> upstream kernel WRITE_ONCE is defined as
> 
> #define WRITE_ONCE(x, val)                                              \
> do {                                                                    \
>          compiletime_assert_rwonce_type(x);                              \
>          __WRITE_ONCE(x, val);                                           \
> } while (0)
> 
> I guess there is a reason for you to not backport
> compiletime_assert_rwonce_type()? If yes, should we just define
> WRITE_ONCE() for simplicity?
> #define WRITE_ONCE(x, val)                                            \
> do {                                                                    \
>          *(volatile typeof(x) *)&(x) = (val);                            \
> } while (0)
> 
> Thanks,
> 
> -Yi-Hung
> 

Sure, sounds good to me.  I'll fix this one up for the next patch
series.

- Greg


More information about the dev mailing list