On 04/26/2018 11:06 PM, Jon Maloy wrote:
> In commit be47e41d77fb ("tipc: fix use-after-free in tipc_nametbl_stop")
> we fixed a problem caused by premature release of service range items.
>
> That fix is correct, but doesn't address the root of the problem, which
> is that we don't lookup the tipc_service -> service_range -> publication
> items in the correct hierarchical order.
>
> In this commit we try to make this right, and as a side effect obtain
> some code simplification.
>
> ---
> v2: Moved topology notifications on step up, to tipc_nametebl_remove_publ()
> This simplifies code further and has the effect that we don't try to
> make useless report_overlap() calls to an already stopped topology
> server in tipc_nametbl_stop()->tipc_service_delete(). I believe this
> has been the root of some of our problems with the toplogy server
> earlier.
Good catch!
Acked-by: Ying Xue <[email protected]>
>
> Signed-off-by: Jon Maloy <[email protected]>
> ---
> net/tipc/name_table.c | 103
> ++++++++++++++++++++++++++------------------------
> 1 file changed, 53 insertions(+), 50 deletions(-)
>
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index dd1c4fa..bebe88c 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -136,12 +136,12 @@ static struct tipc_service *tipc_service_create(u32
> type, struct hlist_head *hd)
> }
>
> /**
> - * tipc_service_find_range - find service range matching a service instance
> + * tipc_service_first_range - find first service range in tree matching
> instance
> *
> * Very time-critical, so binary search through range rb tree
> */
> -static struct service_range *tipc_service_find_range(struct tipc_service *sc,
> - u32 instance)
> +static struct service_range *tipc_service_first_range(struct tipc_service
> *sc,
> + u32 instance)
> {
> struct rb_node *n = sc->ranges.rb_node;
> struct service_range *sr;
> @@ -158,6 +158,30 @@ static struct service_range
> *tipc_service_find_range(struct tipc_service *sc,
> return NULL;
> }
>
> +/* tipc_service_find_range - find service range matching publication
> parameters
> + */
> +static struct service_range *tipc_service_find_range(struct tipc_service *sc,
> + u32 lower, u32 upper)
> +{
> + struct rb_node *n = sc->ranges.rb_node;
> + struct service_range *sr;
> +
> + sr = tipc_service_first_range(sc, lower);
> + if (!sr)
> + return NULL;
> +
> + /* Look for exact match */
> + for (n = &sr->tree_node; n; n = rb_next(n)) {
> + sr = container_of(n, struct service_range, tree_node);
> + if (sr->upper == upper)
> + break;
> + }
> + if (!n || sr->lower != lower || sr->upper != upper)
> + return NULL;
> +
> + return sr;
> +}
> +
> static struct service_range *tipc_service_create_range(struct tipc_service
> *sc,
> u32 lower, u32 upper)
> {
> @@ -238,54 +262,19 @@ static struct publication
> *tipc_service_insert_publ(struct net *net,
> /**
> * tipc_service_remove_publ - remove a publication from a service
> */
> -static struct publication *tipc_service_remove_publ(struct net *net,
> - struct tipc_service *sc,
> - u32 lower, u32 upper,
> - u32 node, u32 key,
> - struct service_range **rng)
> +static struct publication *tipc_service_remove_publ(struct service_range *sr,
> + u32 node, u32 key)
> {
> - struct tipc_subscription *sub, *tmp;
> - struct service_range *sr;
> struct publication *p;
> - bool found = false;
> - bool last = false;
> - struct rb_node *n;
> -
> - sr = tipc_service_find_range(sc, lower);
> - if (!sr)
> - return NULL;
>
> - /* Find exact matching service range */
> - for (n = &sr->tree_node; n; n = rb_next(n)) {
> - sr = container_of(n, struct service_range, tree_node);
> - if (sr->upper == upper)
> - break;
> - }
> - if (!n || sr->lower != lower || sr->upper != upper)
> - return NULL;
> -
> - /* Find publication, if it exists */
> list_for_each_entry(p, &sr->all_publ, all_publ) {
> if (p->key != key || (node && node != p->node))
> continue;
> - found = true;
> - break;
> + list_del(&p->all_publ);
> + list_del(&p->local_publ);
> + return p;
> }
> - if (!found)
> - return NULL;
> -
> - list_del(&p->all_publ);
> - list_del(&p->local_publ);
> - if (list_empty(&sr->all_publ))
> - last = true;
> -
> - /* Notify any waiting subscriptions */
> - list_for_each_entry_safe(sub, tmp, &sc->subscriptions, service_list) {
> - tipc_sub_report_overlap(sub, p->lower, p->upper, TIPC_WITHDRAWN,
> - p->port, p->node, p->scope, last);
> - }
> - *rng = sr;
> - return p;
> + return NULL;
> }
>
> /**
> @@ -376,17 +365,31 @@ struct publication *tipc_nametbl_remove_publ(struct net
> *net, u32 type,
> u32 node, u32 key)
> {
> struct tipc_service *sc = tipc_service_find(net, type);
> + struct tipc_subscription *sub, *tmp;
> struct service_range *sr = NULL;
> struct publication *p = NULL;
> + bool last;
>
> if (!sc)
> return NULL;
>
> spin_lock_bh(&sc->lock);
> - p = tipc_service_remove_publ(net, sc, lower, upper, node, key, &sr);
> + sr = tipc_service_find_range(sc, lower, upper);
> + if (!sr)
> + goto exit;
> + p = tipc_service_remove_publ(sr, node, key);
> + if (!p)
> + goto exit;
> +
> + /* Notify any waiting subscriptions */
> + last = list_empty(&sr->all_publ);
> + list_for_each_entry_safe(sub, tmp, &sc->subscriptions, service_list) {
> + tipc_sub_report_overlap(sub, lower, upper, TIPC_WITHDRAWN,
> + p->port, node, p->scope, last);
> + }
>
> /* Remove service range item if this was its last publication */
> - if (sr && list_empty(&sr->all_publ)) {
> + if (list_empty(&sr->all_publ)) {
> rb_erase(&sr->tree_node, &sc->ranges);
> kfree(sr);
> }
> @@ -396,6 +399,7 @@ struct publication *tipc_nametbl_remove_publ(struct net
> *net, u32 type,
> hlist_del_init_rcu(&sc->service_list);
> kfree_rcu(sc, rcu);
> }
> +exit:
> spin_unlock_bh(&sc->lock);
> return p;
> }
> @@ -437,7 +441,7 @@ u32 tipc_nametbl_translate(struct net *net, u32 type, u32
> instance, u32 *dnode)
> goto not_found;
>
> spin_lock_bh(&sc->lock);
> - sr = tipc_service_find_range(sc, instance);
> + sr = tipc_service_first_range(sc, instance);
> if (unlikely(!sr))
> goto no_match;
>
> @@ -484,7 +488,7 @@ bool tipc_nametbl_lookup(struct net *net, u32 type, u32
> instance, u32 scope,
>
> spin_lock_bh(&sc->lock);
>
> - sr = tipc_service_find_range(sc, instance);
> + sr = tipc_service_first_range(sc, instance);
> if (!sr)
> goto no_match;
>
> @@ -756,8 +760,7 @@ static void tipc_service_delete(struct net *net, struct
> tipc_service *sc)
> spin_lock_bh(&sc->lock);
> rbtree_postorder_for_each_entry_safe(sr, tmpr, &sc->ranges, tree_node) {
> list_for_each_entry_safe(p, tmp, &sr->all_publ, all_publ) {
> - tipc_service_remove_publ(net, sc, p->lower, p->upper,
> - p->node, p->key, &sr);
> + tipc_service_remove_publ(sr, p->node, p->key);
> kfree_rcu(p, rcu);
> }
> rb_erase(&sr->tree_node, &sc->ranges);
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
tipc-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
