On 04/26/2018 03:25 AM, 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.
>
> Signed-off-by: Jon Maloy <[email protected]>
Acked-by: Ying Xue <[email protected]>
> ---
> net/tipc/name_table.c | 69
> +++++++++++++++++++++++++++++----------------------
> 1 file changed, 39 insertions(+), 30 deletions(-)
>
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index dd1c4fa..ad15c50d 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,31 +262,14 @@ 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 tipc_service *sc,
> + 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) {
> @@ -284,7 +291,6 @@ static struct publication
> *tipc_service_remove_publ(struct net *net,
> tipc_sub_report_overlap(sub, p->lower, p->upper, TIPC_WITHDRAWN,
> p->port, p->node, p->scope, last);
> }
> - *rng = sr;
> return p;
> }
>
> @@ -383,10 +389,13 @@ struct publication *tipc_nametbl_remove_publ(struct net
> *net, u32 type,
> 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(sc, sr, node, key);
>
> /* 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 +405,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 +447,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 +494,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 +766,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(sc, 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
