| Message ID | 1597963790-12362-13-git-send-email-gvrose8192@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Linux kernels up to 5.9.x | expand |
On Fri, Aug 21, 2020 at 6:50 AM Greg Rose <gvrose8192@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > Upstream commit: > commit 50b0e61b32ee890a75b4377d5fbe770a86d6a4c1 > Author: Tonghao Zhang <xiangxia.m.yue@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@gmail.com> > Tested-by: Greg Rose <gvrose8192@gmail.com> > Acked-by: Pravin B Shelar <pshelar@ovn.org> > Signed-off-by: David S. Miller <davem@davemloft.net> > > Added additional compat layer fixup for WRITE_ONCE() > > Cc: Tonghao Zhang <xiangxia.m.yue@gmail.com> > Signed-off-by: Greg Rose <gvrose8192@gmail.com> Reviewed-by: Tonghao Zhang <xiangxia.m.yue@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 >
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