iface-helper: fix non-reentrant call to platform for failed IPv6 DAD

Platform invokes change events while reading netlink events. However,
platform code is not re-entrant and calling into platform again is not
allowed (aside operations that do not process the netlink socket, like
lookup of the platform cache).

That basically means, we have to always process events in an idle
handler. That is not a too strong limitation, because we anyway don't
know the call context in which the platform event is emitted and we
should avoid unguarded recursive calls into platform.

Otherwise, we get hit an assertion/crash in nm-iface-helper:

     1  raise()
     2  abort()
     3  g_assertion_message()
     4  g_assertion_message_expr()
     5  do_delete_object()
     6  ip6_address_delete()
 >>> 7  nm_platform_ip6_address_delete()
     8  nm_platform_ip6_address_sync()
     9  nm_ip6_config_commit()
     10 ndisc_config_changed()
     11 ffi_call_unix64()
     12 ffi_call()
     13 g_cclosure_marshal_generic_va()
     14 _g_closure_invoke_va()
     15 g_signal_emit_valist()
     16 g_signal_emit()
 >>> 17 nm_ndisc_dad_failed()
     18 ffi_call_unix64()
     19 ffi_call()
     20 g_cclosure_marshal_generic()
     21 g_closure_invoke()
     22 signal_emit_unlocked_R()
     23 g_signal_emit_valist()
     24 g_signal_emit()
 >>> 25 nm_platform_cache_update_emit_signal()
     26 event_handler_recvmsgs()
     27 event_handler_read_netlink()
     28 delayed_action_handle_one()
     29 delayed_action_handle_all()
     30 do_delete_object()
     31 ip6_address_delete()
     32 nm_platform_ip6_address_delete()
     33 nm_platform_ip6_address_sync()
 >>> 34 nm_ip6_config_commit()
     35 ndisc_config_changed()
     36 ffi_call_unix64()
     37 ffi_call()
     38 g_cclosure_marshal_generic_va()
     39 _g_closure_invoke_va()
     40 g_signal_emit_valist()
     41 g_signal_emit()
     42 check_timestamps()
     43 receive_ra()
     44 ndp_call_eventfd_handler()
     45 ndp_callall_eventfd_handler()
     46 event_ready()
     47 g_main_context_dispatch()
     48 g_main_context_iterate.isra.22()
     49 g_main_loop_run()
 >>> 50 main()

NMPlatform already has a check to assert against recursive calls
in delayed_action_handle_all():

    g_return_val_if_fail (priv->delayed_action.is_handling == 0, FALSE);

    priv->delayed_action.is_handling++;
    ...
    priv->delayed_action.is_handling--;

Fixes: f85728ecff824b1fece43aba51d8171db2766ea2

https://bugzilla.redhat.com/show_bug.cgi?id=1546656

3 files changed, 100 insertions, 26 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 4bb610bc64..ecdf7c2008 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -11638,7 +11638,6 @@ queued_ip6_config_change (gpointer user_data)
 {
 	NMDevice *self = user_data;
 	NMDevicePrivate *priv;
-	GSList *iter;
 	gboolean need_ipv6ll = FALSE;
 	NMPlatform *platform;
 
@@ -11671,20 +11670,17 @@ queued_ip6_config_change (gpointer user_data)
 	    && (platform = nm_device_get_platform (self))
 	    && nm_platform_link_get (platform, priv->ifindex)) {
 		/* Handle DAD failures */
-		for (iter = priv->dad6_failed_addrs; iter; iter = iter->next) {
-			const NMPObject *obj = iter->data;
-			const NMPlatformIP6Address *addr = NMP_OBJECT_CAST_IP6_ADDRESS (obj);
-			const NMPlatformIP6Address *addr2;
-
-			addr2 = NMP_OBJECT_CAST_IP6_ADDRESS (nm_platform_lookup_obj (platform,
-			                                                             NMP_CACHE_ID_TYPE_OBJECT_TYPE,
-			                                                             obj));
-			if (   addr2
-			    && (   NM_FLAGS_HAS (addr2->n_ifa_flags, IFA_F_TEMPORARY)
-			        || !NM_FLAGS_HAS (addr2->n_ifa_flags, IFA_F_DADFAILED))) {
-				/* the address still/again exists and is not in DADFAILED state. Skip it. */
+		while (priv->dad6_failed_addrs) {
+			nm_auto_nmpobj const NMPObject *obj = NULL;
+			const NMPlatformIP6Address *addr;
+
+			obj = priv->dad6_failed_addrs->data;
+			priv->dad6_failed_addrs = g_slist_delete_link (priv->dad6_failed_addrs, priv->dad6_failed_addrs);
+
+			if (!nm_ndisc_dad_addr_is_fail_candidate (platform, obj))
 				continue;
-			}
+
+			addr = NMP_OBJECT_CAST_IP6_ADDRESS (obj);
 
 			_LOGI (LOGD_IP6, "ipv6: duplicate address check failed for the %s address",
 			       nm_platform_ip6_address_to_string (addr, NULL, 0));
@@ -11705,11 +11701,11 @@ queued_ip6_config_change (gpointer user_data)
 
 		if (need_ipv6ll)
 			check_and_add_ipv6ll_addr (self);
+	} else {
+		g_slist_free_full (priv->dad6_failed_addrs, (GDestroyNotify) nmp_object_unref);
+		priv->dad6_failed_addrs = NULL;
 	}
 
-	g_slist_free_full (priv->dad6_failed_addrs, (GDestroyNotify) nmp_object_unref);
-	priv->dad6_failed_addrs = NULL;
-
 	/* Check if DAD is still pending */
 	if (   priv->ip6_state == IP_CONF
 	    && priv->dad6_ip6_config
@@ -11763,11 +11759,9 @@ device_ipx_changed (NMPlatform *platform,
 	case NMP_OBJECT_TYPE_IP6_ADDRESS:
 		addr = platform_object;
 
-		if (   !NM_FLAGS_HAS (addr->n_ifa_flags, IFA_F_TEMPORARY)
-		    && priv->state > NM_DEVICE_STATE_DISCONNECTED
+		if (   priv->state > NM_DEVICE_STATE_DISCONNECTED
 		    && priv->state < NM_DEVICE_STATE_DEACTIVATING
-		    && (   (change_type == NM_PLATFORM_SIGNAL_CHANGED && addr->n_ifa_flags & IFA_F_DADFAILED)
-		        || (change_type == NM_PLATFORM_SIGNAL_REMOVED && addr->n_ifa_flags & IFA_F_TENTATIVE))) {
+		    && nm_ndisc_dad_addr_is_fail_candidate_event (change_type, addr)) {
 			priv->dad6_failed_addrs = g_slist_prepend (priv->dad6_failed_addrs,
 			                                           (gpointer) nmp_object_ref (NMP_OBJECT_UP_CAST (addr)));
 		}
diff --git a/src/ndisc/nm-ndisc.h b/src/ndisc/nm-ndisc.h
index 3cbb8f5000..9a8a27d7af 100644
--- a/src/ndisc/nm-ndisc.h
+++ b/src/ndisc/nm-ndisc.h
@@ -27,6 +27,9 @@
 #include "nm-setting-ip6-config.h"
 #include "NetworkManagerUtils.h"
 
+#include "platform/nm-platform.h"
+#include "platform/nmp-object.h"
+
 #define NM_TYPE_NDISC            (nm_ndisc_get_type ())
 #define NM_NDISC(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), NM_TYPE_NDISC, NMNDisc))
 #define NM_NDISC_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), NM_TYPE_NDISC, NMNDiscClass))
@@ -184,4 +187,32 @@ NMPlatform *nm_ndisc_get_platform (NMNDisc *self);
 NMPNetns *nm_ndisc_netns_get (NMNDisc *self);
 gboolean nm_ndisc_netns_push (NMNDisc *self, NMPNetns **netns);
 
+static inline gboolean
+nm_ndisc_dad_addr_is_fail_candidate_event (NMPlatformSignalChangeType change_type,
+                                           const NMPlatformIP6Address *addr)
+{
+	return    !NM_FLAGS_HAS (addr->n_ifa_flags, IFA_F_TEMPORARY)
+	       && (   (change_type == NM_PLATFORM_SIGNAL_CHANGED && addr->n_ifa_flags & IFA_F_DADFAILED)
+	           || (change_type == NM_PLATFORM_SIGNAL_REMOVED && addr->n_ifa_flags & IFA_F_TENTATIVE));
+}
+
+static inline gboolean
+nm_ndisc_dad_addr_is_fail_candidate (NMPlatform *platform,
+                                     const NMPObject *obj)
+{
+	const NMPlatformIP6Address *addr;
+
+	addr = NMP_OBJECT_CAST_IP6_ADDRESS (nm_platform_lookup_obj (platform,
+	                                                            NMP_CACHE_ID_TYPE_OBJECT_TYPE,
+	                                                            obj));
+	if (   addr
+	    && (   NM_FLAGS_HAS (addr->n_ifa_flags, IFA_F_TEMPORARY)
+	        || !NM_FLAGS_HAS (addr->n_ifa_flags, IFA_F_DADFAILED))) {
+		/* the address still/again exists and is not in DADFAILED state. Skip it. */
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
 #endif /* __NETWORKMANAGER_NDISC_H__ */
diff --git a/src/nm-iface-helper.c b/src/nm-iface-helper.c
index c270333894..68bc1beada 100644
--- a/src/nm-iface-helper.c
+++ b/src/nm-iface-helper.c
@@ -33,6 +33,8 @@
 #include <signal.h>
 #include <linux/rtnetlink.h>
 
+#include "nm-utils/nm-c-list.h"
+
 #include "main-utils.h"
 #include "NetworkManagerUtils.h"
 #include "platform/nm-linux-platform.h"
@@ -55,6 +57,9 @@
 static struct {
 	GMainLoop *main_loop;
 	int ifindex;
+
+	guint dad_failed_id;
+	CList dad_failed_lst_head;
 } gl/*obal*/ = {
 	.ifindex = -1,
 };
@@ -316,19 +321,58 @@ do_early_setup (int *argc, char **argv[])
 	return TRUE;
 }
 
+typedef struct {
+	NMPlatform *platform;
+	NMNDisc *ndisc;
+} DadFailedHandleData;
+
+static gboolean
+dad_failed_handle_idle (gpointer user_data)
+{
+	DadFailedHandleData *data = user_data;
+	NMCListElem *elem;
+
+	while ((elem = c_list_first_entry (&gl.dad_failed_lst_head, NMCListElem, lst))) {
+		nm_auto_nmpobj const NMPObject *obj = elem->data;
+
+		nm_c_list_elem_free (elem);
+
+		if (nm_ndisc_dad_addr_is_fail_candidate (data->platform, obj)) {
+			nm_ndisc_dad_failed (data->ndisc,
+			                     &NMP_OBJECT_CAST_IP6_ADDRESS (obj)->address);
+		}
+	}
+
+	gl.dad_failed_id = 0;
+	return G_SOURCE_REMOVE;
+}
+
 static void
 ip6_address_changed (NMPlatform *platform,
                      int obj_type_i,
                      int iface,
-                     NMPlatformIP6Address *addr,
+                     const NMPlatformIP6Address *addr,
                      int change_type_i,
                      NMNDisc *ndisc)
 {
 	const NMPlatformSignalChangeType change_type = change_type_i;
-
-	if (   (change_type == NM_PLATFORM_SIGNAL_CHANGED && addr->n_ifa_flags & IFA_F_DADFAILED)
-	    || (change_type == NM_PLATFORM_SIGNAL_REMOVED && addr->n_ifa_flags & IFA_F_TENTATIVE))
-		nm_ndisc_dad_failed (ndisc, &addr->address);
+	DadFailedHandleData *data;
+
+	if (!nm_ndisc_dad_addr_is_fail_candidate_event (change_type, addr))
+		return;
+
+	c_list_link_tail (&gl.dad_failed_lst_head,
+	                  &nm_c_list_elem_new_stale ((gpointer) nmp_object_ref (NMP_OBJECT_UP_CAST (addr)))->lst);
+	if (gl.dad_failed_id)
+		return;
+
+	data = g_slice_new (DadFailedHandleData);
+	data->platform = platform;
+	data->ndisc = ndisc;
+	gl.dad_failed_id = g_idle_add_full (G_PRIORITY_DEFAULT_IDLE,
+	                                    dad_failed_handle_idle,
+	                                    data,
+	                                    nm_g_slice_free_fcn (DadFailedHandleData));
 }
 
 int
@@ -346,6 +390,8 @@ main (int argc, char *argv[])
 	guint sd_id;
 	char sysctl_path_buf[NM_UTILS_SYSCTL_IP_CONF_PATH_BUFSIZE];
 
+	c_list_init (&gl.dad_failed_lst_head);
+
 	setpgid (getpid (), getpid ());
 
 	if (!do_early_setup (&argc, &argv))
@@ -526,6 +572,9 @@ main (int argc, char *argv[])
 
 	g_main_loop_run (gl.main_loop);
 
+	nm_clear_g_source (&gl.dad_failed_id);
+	nm_c_list_elem_free_all (&gl.dad_failed_lst_head, (GDestroyNotify) nmp_object_unref);
+
 	if (pidfile && wrote_pidfile)
 		unlink (pidfile);