network: fix ref/unref logic for Link object

- bridge or bonding master takes a reference of slave links. - drop link from bridge or bonding master's slave list when slave link is removed. - change type of Link::slaves to Set*, Fixes #12315.
systemd · Apr 25, 2019 · 5f707e1 · 5f707e1
1 parent bd08ce5
commit 5f707e1
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 57 deletions.
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
@@ -409,7 +409,7 @@ void link_update_operstate(Link *link, bool also_update_master) {
         if (operstate >= LINK_OPERSTATE_CARRIER) {
                 Link *slave;
 
-                HASHMAP_FOREACH(slave, link->slaves, i) {
+                SET_FOREACH(slave, link->slaves, i) {
                         link_update_operstate(slave, false);
 
                         if (slave->operstate < LINK_OPERSTATE_CARRIER)
@@ -605,17 +605,8 @@ static int link_new(Manager *manager, sd_netlink_message *message, Link **ret) {
         return 0;
 }
 
-static void link_detach_from_manager(Link *link) {
-        if (!link || !link->manager)
-                return;
-
-        hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex));
-        set_remove(link->manager->links_requesting_uuid, link);
-        link_clean(link);
-}
-
 static Link *link_free(Link *link) {
-        Link *carrier, *master;
+        Link *carrier;
         Address *address;
         Route *route;
         Iterator i;
@@ -663,10 +654,7 @@ static Link *link_free(Link *link) {
         sd_ndisc_unref(link->ndisc);
         sd_radv_unref(link->radv);
 
-        link_detach_from_manager(link);
-
         free(link->ifname);
-
         free(link->kind);
 
         (void) unlink(link->state_file);
@@ -682,17 +670,7 @@ static Link *link_free(Link *link) {
                 hashmap_remove(link->bound_by_links, INT_TO_PTR(carrier->ifindex));
         hashmap_free(link->bound_by_links);
 
-        hashmap_free(link->slaves);
-
-        if (link->network) {
-                if (link->network->bond &&
-                    link_get(link->manager, link->network->bond->ifindex, &master) >= 0)
-                        (void) hashmap_remove(master->slaves, INT_TO_PTR(link->ifindex));
-
-                if (link->network->bridge &&
-                    link_get(link->manager, link->network->bridge->ifindex, &master) >= 0)
-                        (void) hashmap_remove(master->slaves, INT_TO_PTR(link->ifindex));
-        }
+        set_free_with_destructor(link->slaves, link_unref);
 
         return mfree(link);
 }
@@ -1751,28 +1729,6 @@ static int link_set_bond(Link *link) {
         return r;
 }
 
-static int link_append_to_master(Link *link, NetDev *netdev) {
-        Link *master;
-        int r;
-
-        assert(link);
-        assert(netdev);
-
-        r = link_get(link->manager, netdev->ifindex, &master);
-        if (r < 0)
-                return r;
-
-        r = hashmap_ensure_allocated(&master->slaves, NULL);
-        if (r < 0)
-                return r;
-
-        r = hashmap_put(master->slaves, INT_TO_PTR(link->ifindex), link);
-        if (r < 0)
-                return r;
-
-        return 0;
-}
-
 static int link_lldp_save(Link *link) {
         _cleanup_free_ char *temp_path = NULL;
         _cleanup_fclose_ FILE *f = NULL;
@@ -2535,6 +2491,55 @@ static void link_free_carrier_maps(Link *link) {
         return;
 }
 
+static int link_append_to_master(Link *link, NetDev *netdev) {
+        Link *master;
+        int r;
+
+        assert(link);
+        assert(netdev);
+
+        r = link_get(link->manager, netdev->ifindex, &master);
+        if (r < 0)
+                return r;
+
+        r = set_ensure_allocated(&master->slaves, NULL);
+        if (r < 0)
+                return r;
+
+        r = set_put(master->slaves, link);
+        if (r < 0)
+                return r;
+
+        link_ref(link);
+        return 0;
+}
+
+static void link_drop_from_master(Link *link, NetDev *netdev) {
+        Link *master;
+
+        assert(link);
+
+        if (!link->manager || !netdev)
+                return;
+
+        if (link_get(link->manager, netdev->ifindex, &master) < 0)
+                return;
+
+        link_unref(set_remove(master->slaves, link));
+}
+
+static void link_detach_from_manager(Link *link) {
+        if (!link || !link->manager)
+                return;
+
+        link_unref(set_remove(link->manager->links_requesting_uuid, link));
+        link_clean(link);
+
+        /* The following must be called at last. */
+        assert_se(hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex)) == link);
+        link_unref(link);
+}
+
 void link_drop(Link *link) {
         if (!link || link->state == LINK_STATE_LINGER)
                 return;
@@ -2543,15 +2548,15 @@ void link_drop(Link *link) {
 
         link_free_carrier_maps(link);
 
+        if (link->network) {
+                link_drop_from_master(link, link->network->bridge);
+                link_drop_from_master(link, link->network->bond);
+        }
+
         log_link_debug(link, "Link removed");
 
         (void) unlink(link->state_file);
-
         link_detach_from_manager(link);
-
-        link_unref(link);
-
-        return;
 }
 
 static int link_joined(Link *link) {

diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h
@@ -122,7 +122,7 @@ typedef struct Link {
 
         Hashmap *bound_by_links;
         Hashmap *bound_to_links;
-        Hashmap *slaves;
+        Set *slaves;
 } Link;
 
 typedef int (*link_netlink_message_handler_t)(sd_netlink*, sd_netlink_message*, Link*);

diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c
@@ -1450,13 +1450,12 @@ void manager_free(Manager *m) {
         }
 
         m->dirty_links = set_free_with_destructor(m->dirty_links, link_unref);
-        m->links = hashmap_free(m->links);
-        m->links_requesting_uuid = set_free(m->links_requesting_uuid);
-        set_free(m->duids_requesting_uuid);
+        m->links_requesting_uuid = set_free_with_destructor(m->links_requesting_uuid, link_unref);
+        m->links = hashmap_free_with_destructor(m->links, link_unref);
 
+        m->duids_requesting_uuid = set_free(m->duids_requesting_uuid);
         while ((network = m->networks))
                 network_free(network);
-
         hashmap_free(m->networks_by_name);
 
         m->netdevs = hashmap_free_with_destructor(m->netdevs, netdev_unref);