From aca9b9b57003f7009affdeaf73f4b78ffd3b9f97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20=C5=A0imerda?= Date: Fri, 26 Jul 2013 17:03:39 +0200 Subject: [PATCH] platform: fix udev/kernel interface race conditions A network interface is only exposed if it's recognized by both netlink cache and udev. --- src/platform/nm-linux-platform.c | 70 +++++++++++++++++++++++-------- src/platform/tests/test-address.c | 6 ++- src/platform/tests/test-cleanup.c | 7 +++- src/platform/tests/test-common.c | 47 +++++++++++++++++++++ src/platform/tests/test-common.h | 2 + src/platform/tests/test-link.c | 49 +--------------------- src/platform/tests/test-route.c | 5 +++ 7 files changed, 117 insertions(+), 69 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 6a0436c3e8..3a005871ed 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -754,13 +754,6 @@ announce_object (NMPlatform *platform, const struct nl_object *object, ObjectSta ObjectType object_type = object_type_from_nl_object (object); const char *sig = signal_by_type_and_status[object_type][status]; - if (object_type == LINK && status == ADDED) { - /* We have to wait until udev has registered the device; we'll - * emit NM_PLATFORM_LINK_ADDED from udev_device_added(). - */ - return; - } - switch (object_type) { case LINK: { @@ -768,6 +761,22 @@ announce_object (NMPlatform *platform, const struct nl_object *object, ObjectSta link_init (platform, &device, (struct rtnl_link *) object); + /* Skip devices not yet discovered by udev. They will be announced + * by udev_device_added(). This doesn't apply to removed devices, as + * those come either from udev_device_removed(), event_notification() + * or link_delete() which block the announcment themselves when + * appropriate. + */ + switch (status) { + case ADDED: + case CHANGED: + if (!device.driver) + return; + break; + default: + break; + } + /* In some cases, the link action is followed by address and/or * route action. Kernel silently removes routes when interface * goes !IFF_UP and we also need to handle addresses and routes @@ -785,7 +794,7 @@ announce_object (NMPlatform *platform, const struct nl_object *object, ObjectSta default: break; } - + g_signal_emit_by_name (platform, sig, device.ifindex, &device); } return; @@ -922,7 +931,6 @@ delete_object (NMPlatform *platform, struct nl_object *obj) } nl_cache_remove (cached_object); - announce_object (platform, cached_object, REMOVED); return TRUE; @@ -984,6 +992,16 @@ event_notification (struct nl_msg *msg, gpointer user_data) return NL_OK; nl_cache_remove (cached_object); + /* Don't announced removed interfaces that are not recognized by + * udev. They were either not yet discovered or they have been + * already removed and announced. + */ + if (event == RTM_DELLINK) { + int ifindex = rtnl_link_get_ifindex ((struct rtnl_link *) object); + + if (!g_hash_table_lookup (priv->udev_devices, GINT_TO_POINTER (ifindex))) + return NL_OK; + } announce_object (platform, cached_object, REMOVED); return NL_OK; @@ -1164,8 +1182,10 @@ link_get (NMPlatform *platform, int ifindex) NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform); struct rtnl_link *rtnllink = rtnl_link_get (priv->link_cache, ifindex); - if (!rtnllink) + if (!rtnllink || !g_hash_table_lookup (priv->udev_devices, GINT_TO_POINTER (ifindex))) { platform->error = NM_PLATFORM_ERROR_NOT_FOUND; + return NULL; + } return rtnllink; } @@ -1198,6 +1218,13 @@ link_change (NMPlatform *platform, int ifindex, struct rtnl_link *change) static gboolean link_delete (NMPlatform *platform, int ifindex) { + NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform); + + if (!g_hash_table_lookup (priv->udev_devices, GINT_TO_POINTER (ifindex))) { + platform->error = NM_PLATFORM_ERROR_NOT_FOUND; + return FALSE; + } + return delete_object (platform, build_rtnl_link (ifindex, NULL, NM_LINK_TYPE_NONE)); } @@ -2252,7 +2279,6 @@ udev_device_added (NMPlatform *platform, NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform); auto_nl_object struct rtnl_link *rtnllink = NULL; const char *ifname, *devtype; - NMPlatformLink link; int ifindex; ifname = g_udev_device_get_name (udev_device); @@ -2286,17 +2312,17 @@ udev_device_added (NMPlatform *platform, return; } - rtnllink = link_get (platform, ifindex); + g_hash_table_insert (priv->udev_devices, GINT_TO_POINTER (ifindex), + g_object_ref (udev_device)); + + /* Don't announce devices that have not yet been discovered via Netlink. */ + rtnllink = rtnl_link_get (priv->link_cache, ifindex); if (!rtnllink) { debug ("%s: not found in link cache, ignoring...", ifname); return; } - g_hash_table_insert (priv->udev_devices, GINT_TO_POINTER (ifindex), - g_object_ref (udev_device)); - - link_init (platform, &link, rtnllink); - g_signal_emit_by_name (platform, NM_PLATFORM_LINK_ADDED, ifindex, &link); + announce_object (platform, (struct nl_object *) rtnllink, ADDED); } static void @@ -2304,7 +2330,7 @@ udev_device_removed (NMPlatform *platform, GUdevDevice *udev_device) { NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE (platform); - int ifindex; + int ifindex = 0; if (g_udev_device_get_sysfs_attr (udev_device, "ifindex")) { ifindex = g_udev_device_get_sysfs_attr_as_int (udev_device, "ifindex"); @@ -2324,6 +2350,14 @@ udev_device_removed (NMPlatform *platform, } } } + + /* Announce device removal if it's still in the Netlink cache. */ + if (ifindex) { + struct rtnl_link *device = rtnl_link_get (priv->link_cache, ifindex); + + if (device) + announce_object (platform, (struct nl_object *) device, REMOVED); + } } static void diff --git a/src/platform/tests/test-address.c b/src/platform/tests/test-address.c index 3483676ce0..e896c077ca 100644 --- a/src/platform/tests/test-address.c +++ b/src/platform/tests/test-address.c @@ -221,9 +221,13 @@ test_ip6_address_external (void) void setup_tests (void) { + SignalData *link_added = add_signal_ifname (NM_PLATFORM_LINK_ADDED, link_callback, DEVICE_NAME); + nm_platform_link_delete (nm_platform_link_get_ifindex (DEVICE_NAME)); g_assert (!nm_platform_link_exists (DEVICE_NAME)); - nm_platform_dummy_add (DEVICE_NAME); + g_assert (nm_platform_dummy_add (DEVICE_NAME)); + wait_signal (link_added); + free_signal (link_added); g_test_add_func ("/address/internal/ip4", test_ip4_address); g_test_add_func ("/address/internal/ip6", test_ip6_address); diff --git a/src/platform/tests/test-cleanup.c b/src/platform/tests/test-cleanup.c index 98ce667b22..34f214eb45 100644 --- a/src/platform/tests/test-cleanup.c +++ b/src/platform/tests/test-cleanup.c @@ -5,6 +5,7 @@ static void test_cleanup_internal () { + SignalData *link_added = add_signal_ifname (NM_PLATFORM_LINK_ADDED, link_callback, DEVICE_NAME); int ifindex; GArray *addresses4; GArray *addresses6; @@ -28,11 +29,13 @@ test_cleanup_internal () inet_pton (AF_INET6, "2001:db8:c:d:0:0:0:0", &network6); inet_pton (AF_INET6, "2001:db8:e:f:1:2:3:4", &gateway6); - /* Create device */ + /* Create and set up device */ g_assert (nm_platform_dummy_add (DEVICE_NAME)); + wait_signal (link_added); + free_signal (link_added); + g_assert (nm_platform_link_set_up (nm_platform_link_get_ifindex (DEVICE_NAME))); ifindex = nm_platform_link_get_ifindex (DEVICE_NAME); g_assert (ifindex > 0); - g_assert (nm_platform_link_set_up (ifindex)); /* Add routes and addresses */ g_assert (nm_platform_ip4_address_add (ifindex, addr4, plen4)); diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index 063c303e76..648f11c1d2 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -46,6 +46,53 @@ free_signal (SignalData *data) g_free (data); } +void +link_callback (NMPlatform *platform, int ifindex, NMPlatformLink *received, SignalData *data) +{ + + GArray *links; + NMPlatformLink *cached; + int i; + + g_assert (received); + g_assert_cmpint (received->ifindex, ==, ifindex); + + if (data->ifindex && data->ifindex != received->ifindex) + return; + if (data->ifname && g_strcmp0 (data->ifname, nm_platform_link_get_name (ifindex)) != 0) + return; + + if (data->loop) { + debug ("Quitting main loop."); + g_main_loop_quit (data->loop); + } + + if (data->received) + g_error ("Received signal '%s' a second time.", data->name); + + debug ("Recieved signal '%s' ifindex %d ifname '%s'.", data->name, ifindex, received->name); + data->received = TRUE; + + /* Check the data */ + g_assert (received->ifindex > 0); + links = nm_platform_link_get_all (); + for (i = 0; i < links->len; i++) { + cached = &g_array_index (links, NMPlatformLink, i); + if (cached->ifindex == received->ifindex) { + g_assert (!memcmp (cached, received, sizeof (*cached))); + if (!g_strcmp0 (data->name, NM_PLATFORM_LINK_REMOVED)) { + g_error ("Deleted link still found in the local cache."); + } + g_array_unref (links); + return; + } + } + g_array_unref (links); + + if (g_strcmp0 (data->name, NM_PLATFORM_LINK_REMOVED)) + g_error ("Added/changed link not found in the local cache."); +} + void run_command (const char *format, ...) { diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h index ed8906e996..a10601af0d 100644 --- a/src/platform/tests/test-common.h +++ b/src/platform/tests/test-common.h @@ -31,6 +31,8 @@ void accept_signal (SignalData *data); void wait_signal (SignalData *data); void free_signal (SignalData *data); +void link_callback (NMPlatform *platform, int ifindex, NMPlatformLink *received, SignalData *data); + void run_command (const char *format, ...); void setup_tests (void); diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index 7728200503..801e8dc797 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -14,53 +14,6 @@ #define VLAN_FLAGS 0 #define MTU 1357 -static void -link_callback (NMPlatform *platform, int ifindex, NMPlatformLink *received, SignalData *data) -{ - - GArray *links; - NMPlatformLink *cached; - int i; - - g_assert (received); - g_assert_cmpint (received->ifindex, ==, ifindex); - - if (data->ifindex && data->ifindex != received->ifindex) - return; - if (data->ifname && g_strcmp0 (data->ifname, nm_platform_link_get_name (ifindex)) != 0) - return; - - if (data->loop) { - debug ("Quitting main loop."); - g_main_loop_quit (data->loop); - } - - if (data->received) - g_error ("Received signal '%s' a second time.", data->name); - - debug ("Recieved signal '%s' ifindex %d ifname '%s'.", data->name, ifindex, received->name); - data->received = TRUE; - - /* Check the data */ - g_assert (received->ifindex > 0); - links = nm_platform_link_get_all (); - for (i = 0; i < links->len; i++) { - cached = &g_array_index (links, NMPlatformLink, i); - if (cached->ifindex == received->ifindex) { - g_assert (!memcmp (cached, received, sizeof (*cached))); - if (!g_strcmp0 (data->name, NM_PLATFORM_LINK_REMOVED)) { - g_error ("Deleted link still found in the local cache."); - } - g_array_unref (links); - return; - } - } - g_array_unref (links); - - if (g_strcmp0 (data->name, NM_PLATFORM_LINK_REMOVED)) - g_error ("Added/changed link not found in the local cache."); -} - static void test_bogus(void) { @@ -294,6 +247,7 @@ test_software (NMLinkType link_type, const char *link_typename) link_added = add_signal_ifname (NM_PLATFORM_LINK_ADDED, link_callback, DEVICE_NAME); g_assert (software_add (link_type, DEVICE_NAME)); no_error (); + wait_signal (link_added); g_assert (nm_platform_link_exists (DEVICE_NAME)); ifindex = nm_platform_link_get_ifindex (DEVICE_NAME); g_assert (ifindex >= 0); @@ -307,7 +261,6 @@ test_software (NMLinkType link_type, const char *link_typename) g_assert_cmpint (vlan_id, ==, VLAN_ID); no_error (); } - wait_signal (link_added); /* Add again */ g_assert (!software_add (link_type, DEVICE_NAME)); diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c index 4306d0ee20..d74728e4ab 100644 --- a/src/platform/tests/test-route.c +++ b/src/platform/tests/test-route.c @@ -199,9 +199,14 @@ test_ip6_route () void setup_tests (void) { + SignalData *link_added = add_signal_ifname (NM_PLATFORM_LINK_ADDED, link_callback, DEVICE_NAME); + nm_platform_link_delete (nm_platform_link_get_ifindex (DEVICE_NAME)); g_assert (!nm_platform_link_exists (DEVICE_NAME)); g_assert (nm_platform_dummy_add (DEVICE_NAME)); + wait_signal (link_added); + free_signal (link_added); + g_assert (nm_platform_link_set_up (nm_platform_link_get_ifindex (DEVICE_NAME))); g_test_add_func ("/route/ip4", test_ip4_route);