From a3c73e783b472745850110755f58923ff4cdc130 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 3 Oct 2018 15:37:36 +0200 Subject: [PATCH 1/5] ndisc: first reschedule timeout before invoking change event in check_timestamps() It's just ugly to invoke external code in the middel of an operation. You never know, whether the handler won' unref the ndisc instance. (cherry picked from commit 1f856b7cb3dc1f229753d3a14acb26a8316ba13d) --- src/ndisc/nm-ndisc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index 879eec67fe..1e72c165f6 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -1132,9 +1132,6 @@ check_timestamps (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap changed) clean_dns_servers (ndisc, now, &changed, &nextevent); clean_dns_domains (ndisc, now, &changed, &nextevent); - if (changed) - nm_ndisc_emit_config_change (ndisc, changed); - if (nextevent != G_MAXINT32) { if (nextevent <= now) g_return_if_reached (); @@ -1142,6 +1139,9 @@ check_timestamps (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap changed) (int) (nextevent - now)); priv->timeout_id = g_timeout_add_seconds (nextevent - now, timeout_cb, ndisc); } + + if (changed) + nm_ndisc_emit_config_change (ndisc, changed); } static gboolean From efb9e2bc6b354d189da34fab07e79db1bf162cb1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 3 Oct 2018 15:41:29 +0200 Subject: [PATCH 2/5] ndisc: keep NMNDisc instance alive while processing IO in event_ready() event_ready() calls ndp_callall_eventfd_handler(), which invokes our own callback, which may invoke change notification. At that point, it's not guaranteed that the signal handler won't destroy the ndisc instance, which means, the "struct ndp" gets destroyed while invoking callbacks. That's bad, because libndp is not robust against that. Ensure the object stays alive long enough. (cherry picked from commit 9aa628cedb707e9c4f0e0dba437ec22375a0032e) --- src/ndisc/nm-lndp-ndisc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index c0a0cd4096..8d237e86c7 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -491,6 +491,7 @@ receive_rs (struct ndp *ndp, struct ndp_msg *msg, gpointer user_data) static gboolean event_ready (GIOChannel *source, GIOCondition condition, NMNDisc *ndisc) { + gs_unref_object NMNDisc *ndisc_keep_alive = g_object_ref (ndisc); nm_auto_pop_netns NMPNetns *netns = NULL; NMLndpNDiscPrivate *priv = NM_LNDP_NDISC_GET_PRIVATE ((NMLndpNDisc *) ndisc); From 6631debaa32d42c8e45f24a696c2e033d525c061 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 3 Oct 2018 15:44:36 +0200 Subject: [PATCH 3/5] ndisc: abort handling IO in event_ready() if we are unable to switch namespace It should never happen that we are unable to switch the namespace. However, in case it does, we cannot just return G_SOURCE_CONTINUE, because we will just endlessly trying to process IO without actually reading from the socket. This shouldn't happen, but the instance is hosed and something is very wrong. No longer handle the socket to avoid an endless loop. (cherry picked from commit d444fcde34f06711208494a65cd0ea9b9ba2c592) --- src/ndisc/nm-lndp-ndisc.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index 8d237e86c7..8bb1812764 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -497,8 +497,11 @@ event_ready (GIOChannel *source, GIOCondition condition, NMNDisc *ndisc) _LOGD ("processing libndp events"); - if (!nm_ndisc_netns_push (ndisc, &netns)) - return G_SOURCE_CONTINUE; + if (!nm_ndisc_netns_push (ndisc, &netns)) { + /* something is very wrong. Stop handling events. */ + priv->event_id = 0; + return G_SOURCE_REMOVE; + } ndp_callall_eventfd_handler (priv->ndp); return G_SOURCE_CONTINUE; From bd41719f95d20a5cf719a7f1ddd21c45b12dfda1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 3 Oct 2018 16:02:20 +0200 Subject: [PATCH 4/5] ndisc/trivial: move code (cherry picked from commit 4f78d82fcd135e42bc0a5dabb4522ab318c47808) --- src/ndisc/nm-ndisc.c | 104 ++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 51 deletions(-) diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index 1e72c165f6..1a1138668e 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -124,6 +124,59 @@ _preference_to_priority (NMIcmpv6RouterPref pref) /*****************************************************************************/ +static gint32 +get_expiry_time (guint32 timestamp, guint32 lifetime) +{ + gint64 t; + + /* timestamp is supposed to come from nm_utils_get_monotonic_timestamp_s(). + * It is expected to be within a certain range. */ + nm_assert (timestamp > 0); + nm_assert (timestamp <= G_MAXINT32); + + if (lifetime == NM_NDISC_INFINITY) + return G_MAXINT32; + + t = (gint64) timestamp + (gint64) lifetime; + return CLAMP (t, 0, G_MAXINT32 - 1); +} + +#define get_expiry(item) \ + ({ \ + typeof (item) _item = (item); \ + nm_assert (_item); \ + get_expiry_time ((_item->timestamp), (_item->lifetime)); \ + }) + +#define get_expiry_half(item) \ + ({ \ + typeof (item) _item = (item); \ + nm_assert (_item); \ + get_expiry_time ((_item->timestamp),\ + (_item->lifetime) == NM_NDISC_INFINITY \ + ? NM_NDISC_INFINITY \ + : (_item->lifetime) / 2); \ + }) + +static const char * +_get_exp (char *buf, gsize buf_size, gint64 now_ns, gint32 expiry_time) +{ + int l; + + if (expiry_time == G_MAXINT32) + return "permanent"; + l = g_snprintf (buf, buf_size, + "%.4f", + ((double) ((expiry_time * NM_UTILS_NS_PER_SECOND) - now_ns)) / ((double) NM_UTILS_NS_PER_SECOND)); + nm_assert (l < buf_size); + return buf; +} + +#define get_exp(buf, now_ns, item) \ + _get_exp ((buf), G_N_ELEMENTS (buf), (now_ns), (get_expiry (item))) + +/*****************************************************************************/ + NMPNetns * nm_ndisc_netns_get (NMNDisc *self) { @@ -860,57 +913,6 @@ dhcp_level_to_string (NMNDiscDHCPLevel dhcp_level) } } -static gint32 -get_expiry_time (guint32 timestamp, guint32 lifetime) -{ - gint64 t; - - /* timestamp is supposed to come from nm_utils_get_monotonic_timestamp_s(). - * It is expected to be within a certain range. */ - nm_assert (timestamp > 0); - nm_assert (timestamp <= G_MAXINT32); - - if (lifetime == NM_NDISC_INFINITY) - return G_MAXINT32; - - t = (gint64) timestamp + (gint64) lifetime; - return CLAMP (t, 0, G_MAXINT32 - 1); -} - -#define get_expiry(item) \ - ({ \ - typeof (item) _item = (item); \ - nm_assert (_item); \ - get_expiry_time ((_item->timestamp), (_item->lifetime)); \ - }) - -#define get_expiry_half(item) \ - ({ \ - typeof (item) _item = (item); \ - nm_assert (_item); \ - get_expiry_time ((_item->timestamp),\ - (_item->lifetime) == NM_NDISC_INFINITY \ - ? NM_NDISC_INFINITY \ - : (_item->lifetime) / 2); \ - }) - -static const char * -_get_exp (char *buf, gsize buf_size, gint64 now_ns, gint32 expiry_time) -{ - int l; - - if (expiry_time == G_MAXINT32) - return "permanent"; - l = g_snprintf (buf, buf_size, - "%.4f", - ((double) ((expiry_time * NM_UTILS_NS_PER_SECOND) - now_ns)) / ((double) NM_UTILS_NS_PER_SECOND)); - nm_assert (l < buf_size); - return buf; -} - -#define get_exp(buf, now_ns, item) \ - _get_exp ((buf), G_N_ELEMENTS (buf), (now_ns), (get_expiry (item))) - static void _config_changed_log (NMNDisc *ndisc, NMNDiscConfigMap changed) { From 2e12660dd417d75e30d4a1ab689259a9c10344e4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 3 Oct 2018 16:21:47 +0200 Subject: [PATCH 5/5] ndisc: always emit changed signal if an ndisc parameter changes Note how the nm_ndisc_add_*() return a boolean to indicate whether anything changes. That is taken to decide whether to emit a changed signal. Previously, we would not consider all fields which are exposed as public API. Note that nm-ip6-config.c would care about the lifetime of NMNDiscAddress. For that, nm_ndisc_add_address() would correctly consider a change of the lifetime as relevant. So, this was for the most part not broken. However, for example nm_ndisc_add_route() would ignore changes to the gateway. Always signal changes if anything changes at all. It's more correct and robust. (cherry picked from commit 98ec56c6704aa45b438e7c70ad8860a3c0dc5b70) --- src/ndisc/nm-ndisc.c | 60 +++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index 1a1138668e..a6b4453891 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -158,6 +158,13 @@ get_expiry_time (guint32 timestamp, guint32 lifetime) : (_item->lifetime) / 2); \ }) +#define get_expiry_preferred(item) \ + ({ \ + typeof (item) _item = (item); \ + nm_assert (_item); \ + get_expiry_time ((_item->timestamp), (_item->preferred)); \ + }) + static const char * _get_exp (char *buf, gsize buf_size, gint64 now_ns, gint32 expiry_time) { @@ -317,9 +324,12 @@ nm_ndisc_add_gateway (NMNDisc *ndisc, const NMNDiscGateway *new) continue; } + if (get_expiry (item) == get_expiry (new)) + return FALSE; + *item = *new; _ASSERT_data_gateways (rdata); - return FALSE; + return TRUE; } /* Put before less preferable gateways. */ @@ -411,17 +421,18 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new) NMNDiscAddress *item = &g_array_index (rdata->addresses, NMNDiscAddress, i); if (IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) { - gboolean changed; - if (new->lifetime == 0) { g_array_remove_index (rdata->addresses, i); return TRUE; } - changed = item->timestamp + item->lifetime != new->timestamp + new->lifetime || - item->timestamp + item->preferred != new->timestamp + new->preferred; + if ( item->dad_counter == new->dad_counter + && get_expiry (item) == get_expiry (new) + && get_expiry_preferred (item) == get_expiry_preferred (new)) + return FALSE; + *item = *new; - return changed; + return TRUE; } } @@ -472,7 +483,8 @@ nm_ndisc_add_route (NMNDisc *ndisc, const NMNDiscRoute *new) for (i = 0; i < rdata->routes->len; ) { NMNDiscRoute *item = &g_array_index (rdata->routes, NMNDiscRoute, i); - if (IN6_ARE_ADDR_EQUAL (&item->network, &new->network) && item->plen == new->plen) { + if ( IN6_ARE_ADDR_EQUAL (&item->network, &new->network) + && item->plen == new->plen) { if (new->lifetime == 0) { g_array_remove_index (rdata->routes, i); return TRUE; @@ -483,8 +495,12 @@ nm_ndisc_add_route (NMNDisc *ndisc, const NMNDiscRoute *new) continue; } - memcpy (item, new, sizeof (*new)); - return FALSE; + if ( get_expiry (item) == get_expiry (new) + && IN6_ARE_ADDR_EQUAL (&item->gateway, &new->gateway)) + return FALSE; + + *item = *new; + return TRUE; } /* Put before less preferable routes. */ @@ -523,11 +539,12 @@ nm_ndisc_add_dns_server (NMNDisc *ndisc, const NMNDiscDNSServer *new) g_array_remove_index (rdata->dns_servers, i); return TRUE; } - if (item->timestamp != new->timestamp || item->lifetime != new->lifetime) { - *item = *new; - return TRUE; - } - return FALSE; + + if (get_expiry (item) == get_expiry (new)) + return FALSE; + + *item = *new; + return TRUE; } } @@ -552,20 +569,17 @@ nm_ndisc_add_dns_domain (NMNDisc *ndisc, const NMNDiscDNSDomain *new) item = &g_array_index (rdata->dns_domains, NMNDiscDNSDomain, i); if (!g_strcmp0 (item->domain, new->domain)) { - gboolean changed; - if (new->lifetime == 0) { g_array_remove_index (rdata->dns_domains, i); return TRUE; } - changed = (item->timestamp != new->timestamp || - item->lifetime != new->lifetime); - if (changed) { - item->timestamp = new->timestamp; - item->lifetime = new->lifetime; - } - return changed; + if (get_expiry (item) == get_expiry (new)) + return FALSE; + + item->timestamp = new->timestamp; + item->lifetime = new->lifetime; + return TRUE; } }