From 472dd9aa672de85b49ade5150cbb81497cb8d14d Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Mon, 2 Jun 2014 12:21:57 +0200

Subject: [PATCH 1/5] firewall: refactor reentrancy for dispose() in

 NMFirewallManager

Signed-off-by: Thomas Haller <thaller@redhat.com>

(cherry picked from commit dde731f0ade88c690b7cf6465c016b9d2343094b)

---

 src/firewall-manager/nm-firewall-manager.c | 17 +++++------------

 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/src/firewall-manager/nm-firewall-manager.c b/src/firewall-manager/nm-firewall-manager.c

index 3bf2f24..6d87391 100644

--- a/src/firewall-manager/nm-firewall-manager.c

+++ b/src/firewall-manager/nm-firewall-manager.c

@@ -44,7 +44,6 @@ typedef struct {

 	guint           name_owner_id;

 	DBusGProxy *    proxy;

 	gboolean        running;

-	gboolean        disposed;

 } NMFirewallManagerPrivate;

 enum {

@@ -247,7 +246,7 @@ nm_firewall_manager_init (NMFirewallManager * self)

 	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (self);

 	DBusGConnection *bus;

-	priv->dbus_mgr = nm_dbus_manager_get ();

+	priv->dbus_mgr = g_object_ref (nm_dbus_manager_get ());

 	priv->name_owner_id = g_signal_connect (priv->dbus_mgr,

 	                                        NM_DBUS_MANAGER_NAME_OWNER_CHANGED,

 	                                        G_CALLBACK (name_owner_changed),

@@ -286,20 +285,14 @@ dispose (GObject *object)

 {

 	NMFirewallManagerPrivate *priv = NM_FIREWALL_MANAGER_GET_PRIVATE (object);

-	if (priv->disposed)

-		goto out;

-	priv->disposed = TRUE;

-

 	if (priv->dbus_mgr) {

-		if (priv->name_owner_id)

-			g_signal_handler_disconnect (priv->dbus_mgr, priv->name_owner_id);

-		priv->dbus_mgr = NULL;

+		g_signal_handler_disconnect (priv->dbus_mgr, priv->name_owner_id);

+		priv->name_owner_id = 0;

+		g_clear_object (&priv->dbus_mgr);

 	}

-	if (priv->proxy)

-		g_object_unref (priv->proxy);

+	g_clear_object (&priv->proxy);

-out:

 	/* Chain up to the parent class */

 	G_OBJECT_CLASS (nm_firewall_manager_parent_class)->dispose (object);

 }

-- 

1.9.3

From 3e4ab3a8860d916286ea772e85dfa0432893a6d5 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Mon, 2 Jun 2014 15:56:24 +0200

Subject: [PATCH 2/5] firewall: refactor allocation of CBInfo data in

 NMFirewallManager

Signed-off-by: Thomas Haller <thaller@redhat.com>

(cherry picked from commit e3605ab924dd9865ecba9a06de6f5011a9bae3e3)

---

 src/firewall-manager/nm-firewall-manager.c | 21 +++++++++++++++------

 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/firewall-manager/nm-firewall-manager.c b/src/firewall-manager/nm-firewall-manager.c

index 6d87391..52a5444 100644

--- a/src/firewall-manager/nm-firewall-manager.c

+++ b/src/firewall-manager/nm-firewall-manager.c

@@ -70,6 +70,19 @@ cb_info_free (CBInfo *info)

 	g_free (info);

 }

+static CBInfo *

+_cb_info_create (const char *iface, FwAddToZoneFunc callback, gpointer user_data)

+{

+	CBInfo *info;

+

+	info = g_malloc0 (sizeof (CBInfo));

+	info->iface = g_strdup (iface);

+	info->callback = callback;

+	info->user_data = user_data;

+

+	return info;

+}

+

 static void

 add_or_change_cb (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_data)

 {

@@ -113,10 +126,7 @@ nm_firewall_manager_add_or_change_zone (NMFirewallManager *self,

 		return NULL;

 	}

-	info = g_malloc0 (sizeof (*info));

-	info->iface = g_strdup (iface);

-	info->callback = callback;

-	info->user_data = user_data;

+	info = _cb_info_create (iface, callback, user_data);

 	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s -> %s%s%s", iface, add ? "add" : "change",

 	                           zone?"\"":"", zone ? zone : "default", zone?"\"":"");

@@ -166,8 +176,7 @@ nm_firewall_manager_remove_from_zone (NMFirewallManager *self,

 		return NULL;

 	}

-	info = g_malloc0 (sizeof (*info));

-	info->iface = g_strdup (iface);

+	info = _cb_info_create (iface, NULL, NULL);

 	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone remove -> %s", iface, zone );

 	return dbus_g_proxy_begin_call_with_timeout (priv->proxy,

-- 

1.9.3

From 28732c8a3589395975ec6113f1fe2bb925acf72c Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Mon, 2 Jun 2014 17:52:33 +0200

Subject: [PATCH 3/5] firewall: extend logging to show id for async dbus calls

 in NMFirewallManager

Signed-off-by: Thomas Haller <thaller@redhat.com>

(cherry picked from commit 3bc38ad531b9976577e543229125d7d8274efe82)

---

 src/firewall-manager/nm-firewall-manager.c | 43 +++++++++++++++++++++++-------

 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/firewall-manager/nm-firewall-manager.c b/src/firewall-manager/nm-firewall-manager.c

index 52a5444..c99372a 100644

--- a/src/firewall-manager/nm-firewall-manager.c

+++ b/src/firewall-manager/nm-firewall-manager.c

@@ -60,12 +60,18 @@ typedef struct {

 	char *iface;

 	FwAddToZoneFunc callback;

 	gpointer user_data;

+	guint id;

+	gboolean completed;

 } CBInfo;

 static void

 cb_info_free (CBInfo *info)

 {

 	g_return_if_fail (info != NULL);

+

+	if (!info->completed)

+		nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone call cancelled [%u]", info->iface, info->id);

+

 	g_free (info->iface);

 	g_free (info);

 }

@@ -73,10 +79,15 @@ cb_info_free (CBInfo *info)

 static CBInfo *

 _cb_info_create (const char *iface, FwAddToZoneFunc callback, gpointer user_data)

 {

+	static guint id;

 	CBInfo *info;

-	info = g_malloc0 (sizeof (CBInfo));

+	info = g_malloc (sizeof (CBInfo));

+	if (++id == 0)

+		++id;

+	info->id = id;

 	info->iface = g_strdup (iface);

+	info->completed = FALSE;

 	info->callback = callback;

 	info->user_data = user_data;

@@ -95,16 +106,20 @@ add_or_change_cb (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_data

 	                            G_TYPE_INVALID)) {

 		g_assert (error);

 		if (g_strcmp0 (error->message, "ZONE_ALREADY_SET") != 0) {

-			nm_log_warn (LOGD_FIREWALL, "(%s) firewall zone add/change failed: (%d) %s",

-			             info->iface, error->code, error->message);

+			nm_log_warn (LOGD_FIREWALL, "(%s) firewall zone add/change failed [%u]: (%d) %s",

+			             info->iface, info->id, error->code, error->message);

 		} else {

-			nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone add/change failed: (%d) %s",

-			            info->iface, error->code, error->message);

+			nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone add/change failed [%u]: (%d) %s",

+			            info->iface, info->id, error->code, error->message);

 		}

+	} else {

+		nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone add/change succeeded [%u]",

+		            info->iface, info->id);

 	}

 	info->callback (error, info->user_data);

+	info->completed = TRUE;

 	g_free (zone);

 	g_clear_error (&error);

 }

@@ -128,8 +143,8 @@ nm_firewall_manager_add_or_change_zone (NMFirewallManager *self,

 	info = _cb_info_create (iface, callback, user_data);

-	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s -> %s%s%s", iface, add ? "add" : "change",

-	                           zone?"\"":"", zone ? zone : "default", zone?"\"":"");

+	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone %s -> %s%s%s [%u]", iface, add ? "add" : "change",

+	                           zone?"\"":"", zone ? zone : "default", zone?"\"":"", info->id);

 	return dbus_g_proxy_begin_call_with_timeout (priv->proxy,

 	                                             add ? "addInterface" : "changeZone",

 	                                             add_or_change_cb,

@@ -154,11 +169,18 @@ remove_cb (DBusGProxy *proxy, DBusGProxyCall *call_id, gpointer user_data)

 		g_assert (error);

 		/* ignore UNKNOWN_INTERFACE errors */

 		if (error->message && !strstr (error->message, "UNKNOWN_INTERFACE")) {

-			nm_log_warn (LOGD_FIREWALL, "(%s) firewall zone remove failed: (%d) %s",

-			             info->iface, error->code, error->message);

+			nm_log_warn (LOGD_FIREWALL, "(%s) firewall zone remove failed [%u]: (%d) %s",

+			             info->iface, info->id, error->code, error->message);

+		} else {

+			nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone remove failed [%u]: (%d) %s",

+			            info->iface, info->id, error->code, error->message);

 		}

+	} else {

+		nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone remove succeeded [%u]",

+		            info->iface, info->id);

 	}

+	info->completed = TRUE;

 	g_free (zone);

 	g_clear_error (&error);

 }

@@ -178,7 +200,8 @@ nm_firewall_manager_remove_from_zone (NMFirewallManager *self,

 	info = _cb_info_create (iface, NULL, NULL);

-	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone remove -> %s", iface, zone );

+	nm_log_dbg (LOGD_FIREWALL, "(%s) firewall zone remove -> %s%s%s [%u]", iface,

+	                           zone?"\"":"", zone ? zone : "*", zone?"\"":"", info->id);

 	return dbus_g_proxy_begin_call_with_timeout (priv->proxy,

 	                                             "removeInterface",

 	                                             remove_cb,

-- 

1.9.3

From 342f9f90dbbea28d02dd7a5058c1caea7817142c Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Mon, 2 Jun 2014 18:11:54 +0200

Subject: [PATCH 4/5] firewall: fix ZONE_CONFLICT when removing interface from

 zone

The firewalld removeInterface call fails with ZONE_CONFLICT when

removing an interface from a wrong zone. This can happen, when the

connection gets modified, while being active (which is related to

bgo#724041).

By not specifying any zone, we remove the interface from the zone

where it currently is added. This behavior was introduced in upstream

firewalld with commit cc3101ab70a3997228be7bc9f45a069c7fccfa36, March 2012,

r0_2_3-1.

This is the behavior we actually want and we don't have to keep proper track

of the current zone.

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

Signed-off-by: Thomas Haller <thaller@redhat.com>

(cherry picked from commit c598336de8d5a257765bf415b87e2bb7a1140b7d)

---

 src/devices/nm-device.c | 4 +---

 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c

index 8399cf6..dd0ea5a 100644

--- a/src/devices/nm-device.c

+++ b/src/devices/nm-device.c

@@ -4912,7 +4912,6 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason)

 	NMDevicePrivate *priv;

 	NMDeviceStateReason ignored = NM_DEVICE_STATE_REASON_NONE;

 	NMConnection *connection = NULL;

-	NMSettingConnection *s_con = NULL;

 	int ifindex;

 	g_return_if_fail (NM_IS_DEVICE (self));

@@ -4937,10 +4936,9 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason)

 	if (priv->act_request)

 		connection = nm_act_request_get_connection (priv->act_request);

 	if (connection) {

-		s_con = nm_connection_get_setting_connection (connection);

 		nm_firewall_manager_remove_from_zone (priv->fw_manager,

 		                                      nm_device_get_ip_iface (self),

-		                                      nm_setting_connection_get_zone (s_con));

+		                                      NULL);

 	}

 	ip_check_gw_ping_cleanup (self);

-- 

1.9.3

From cda973fe9c83ea415d3178973ff7826904b2997f Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Tue, 3 Jun 2014 08:58:20 +0200

Subject: [PATCH 5/5] firewall: fix ZONE_CONFLICT when adding firewall

 interface to zone

Firewalld call addInterface() fails with ZONE_CONFLICT if the interface

is already part of another zone. This complicates the code in NM,

because we would have to keep better track of the zone in which the

interface currently is. Which might be quite difficult because

the zone might be changed from an external program (so we would have

to monitor the firewall configuration and work around potential races).

A better and simpler fix is to simply always use the changeZone() call.

This will do the right thing, regardless if the interface is already part

of a zone or not.

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

Signed-off-by: Thomas Haller <thaller@redhat.com>

(cherry picked from commit c29388bf028d404066e46ea55abc4058abce4078)

---

 src/devices/nm-device.c | 6 +++---

 src/nm-policy.c         | 2 +-

 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c

index dd0ea5a..264d4ab 100644

--- a/src/devices/nm-device.c

+++ b/src/devices/nm-device.c

@@ -4023,7 +4023,7 @@ out:

 static void

-fw_add_to_zone_cb (GError *error, gpointer user_data)

+fw_change_zone_cb (GError *error, gpointer user_data)

 {

 	NMDevice *self = NM_DEVICE (user_data);

 	NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);

@@ -4072,8 +4072,8 @@ nm_device_activate_schedule_stage3_ip_config_start (NMDevice *self)

 	priv->fw_call = nm_firewall_manager_add_or_change_zone (priv->fw_manager,

 	                                                        nm_device_get_ip_iface (self),

 	                                                        zone,

-	                                                        TRUE,

-	                                                        fw_add_to_zone_cb,

+	                                                        FALSE,

+	                                                        fw_change_zone_cb,

 	                                                        self);

 }

diff --git a/src/nm-policy.c b/src/nm-policy.c

index b412427..f064fa0 100644

--- a/src/nm-policy.c

+++ b/src/nm-policy.c

@@ -1886,7 +1886,7 @@ firewall_started (NMFirewallManager *manager,

 			nm_firewall_manager_add_or_change_zone (priv->fw_manager,

 			                                        nm_device_get_ip_iface (dev),

 			                                        nm_setting_connection_get_zone (s_con),

-			                                        TRUE, /* add zone */

+			                                        FALSE, /* still change zone */

 			                                        add_or_change_zone_cb,

 			                                        g_object_ref (dev));

 		}

-- 

1.9.3