| 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
| ||
|
Message-ID: <CAMDZJNWeUoXD9SOBXfWes7Xk=BLRPs1iti+Kwz7YfC0NSE6oig@mail.gmail.com>
Date: Mon, 28 Oct 2019 14:49:00 +0800
From: Tonghao Zhang <xiangxia.m.yue@...il.com>
To: Pravin Shelar <pshelar@....org>
Cc: Greg Rose <gvrose8192@...il.com>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH net-next v4 08/10] net: openvswitch: fix possible memleak
on destroy flow-table
On Thu, Oct 24, 2019 at 3:14 PM Pravin Shelar <pshelar@....org> wrote:
>
> On Tue, Oct 22, 2019 at 7:35 PM Tonghao Zhang <xiangxia.m.yue@...il.com> wrote:
> >
> > On Tue, Oct 22, 2019 at 2:58 PM Pravin Shelar <pshelar@....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)
Powered by blists - more mailing lists