Re: [PATCH] device: increase object ref count before invoking g_idle_add

[Thomas Haller]

On Wed, 2016-05-04 at 17:27 +0800, Shih-Yuan Lee (FourDollars) wrote:
> ---
>  src/devices/nm-device.c | 25 +
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
> index ad6f835..77f8874 100644
> --- a/src/devices/nm-device.c
> +++ b/src/devices/nm-device.c
> @@ -1990,8 +1990,8 @@ realize_start_setup (NMDevice *self, const
> NMPlatformLink *plink)
>   }
>  
>   /* trigger initial ip config change to initialize ip-config
> */
> - priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, self);
> - priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, self);
> + priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, g_object_ref (self));
> + priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, g_object_ref (self));
>  
>   nm_device_update_hw_address (self);
>   nm_device_update_initial_hw_address (self);
> @@ -6982,6 +6982,7 @@ queued_ip4_config_change_clear (NMDevice *self)
>   _LOGD (LOGD_DEVICE, "clearing queued IP4 config
> change");
>   g_source_remove (priv->queued_ip4_config_id);
>   priv->queued_ip4_config_id = 0;
> + g_object_unref (self);
>   }
>  }
>  
> @@ -6994,6 +6995,7 @@ queued_ip6_config_change_clear (NMDevice *self)
>   _LOGD (LOGD_DEVICE, "clearing queued IP6 config
> change");
>   g_source_remove (priv->queued_ip6_config_id);
>   priv->queued_ip6_config_id = 0;
> + g_object_unref (self);
>   }
>  }
>  
> @@ -8942,7 +8944,11 @@ update_ip4_config (NMDevice *self, gboolean
> initial)
>   if (activation_source_is_scheduled (self,
>   activate_stage5_ip4_conf
> ig_commit,
>   AF_INET)) {
> - priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, self);
> + if (priv->queued_ip4_config_id) {
> + g_source_remove (priv-
> >queued_ip4_config_id);
> + g_object_unref (self);
> + }
> + priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, g_object_ref (self));
>   _LOGT (LOGD_DEVICE, "IP4 update was postponed");
>   return;
>   }
> @@ -9031,7 +9037,11 @@ update_ip6_config (NMDevice *self, gboolean
> initial)
>   if (activation_source_is_scheduled (self,
>   activate_stage5_ip6_conf
> ig_commit,
>   AF_INET6)) {
> - priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, self);
> + if (priv->queued_ip6_config_id) {
> + g_source_remove (priv-
> >queued_ip6_config_id);
> + g_object_unref (self);
> + }
> + priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, g_object_ref (self));
>   _LOGT (LOGD_DEVICE, "IP6 update was postponed");
>   return;
>   }
> @@ -9109,8 +9119,8 @@ queued_ip4_config_change (gpointer user_data)
>   return TRUE;
>  
>   priv->queued_ip4_config_id = 0;
> - g_object_ref (self);
>   update_ip4_config (self, FALSE);
> +
>   g_object_unref (self);
>  
>   set_unmanaged_external_down (self, TRUE);
> @@ -9131,7 +9141,6 @@ queued_ip6_config_change (gpointer user_data)
>   return TRUE;
>  
>   priv->queued_ip6_config_id = 0;
> - g_object_ref (self);
>   update_ip6_config (self, FALSE);
>  
>   if (   nm_platform_link_get (NM_PLATFORM_GET, priv->ifindex)
> @@ -9194,7 +9203,7 @@ device_ipx_changed (NMPlatform *platform,
>   case NMP_OBJECT_TYPE_IP4_ADDRESS:
>   case NMP_OBJECT_TYPE_IP4_ROUTE:
>   if (!priv->queued_ip4_config_id) {
> - priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, self);
> + priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, g_object_ref (self));
>   _LOGD (LOGD_DEVICE, "queued IP4 config
> change");
>   }
>   break;
> @@ -9211,7 +9220,7 @@ device_ipx_changed (NMPlatform *platform,
>   /* fallthrough */
>   case NMP_OBJECT_TYPE_IP6_ROUTE:
>   if (!priv->queued_ip6_config_id) {
> - priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, self);
> + priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, g_object_ref (self));
>   _LOGD (LOGD_DEVICE, "queued IP6 config
> change");
>   }
>   break;


Hi,


For any pending queued_ip4_config_change(), we track the gsource ID
in queued_ip4_config_id.
Thus, the right fix is not to take an additional reference for @self,
but ensuring that the queued_ip4_config_id is cleare

[PATCH] device: increase object ref count before invoking g_idle_add

[Shih-Yuan Lee (FourDollars)]

---
 src/devices/nm-device.c | 25 +
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index ad6f835..77f8874 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -1990,8 +1990,8 @@ realize_start_setup (NMDevice *self, const NMPlatformLink 
*plink)
}
 
/* trigger initial ip config change to initialize ip-config */
-   priv->queued_ip4_config_id = g_idle_add (queued_ip4_config_change, 
self);
-   priv->queued_ip6_config_id = g_idle_add (queued_ip6_config_change, 
self);
+   priv->queued_ip4_config_id = g_idle_add (queued_ip4_config_change, 
g_object_ref (self));
+   priv->queued_ip6_config_id = g_idle_add (queued_ip6_config_change, 
g_object_ref (self));
 
nm_device_update_hw_address (self);
nm_device_update_initial_hw_address (self);
@@ -6982,6 +6982,7 @@ queued_ip4_config_change_clear (NMDevice *self)
_LOGD (LOGD_DEVICE, "clearing queued IP4 config change");
g_source_remove (priv->queued_ip4_config_id);
priv->queued_ip4_config_id = 0;
+   g_object_unref (self);
}
 }
 
@@ -6994,6 +6995,7 @@ queued_ip6_config_change_clear (NMDevice *self)
_LOGD (LOGD_DEVICE, "clearing queued IP6 config change");
g_source_remove (priv->queued_ip6_config_id);
priv->queued_ip6_config_id = 0;
+   g_object_unref (self);
}
 }
 
@@ -8942,7 +8944,11 @@ update_ip4_config (NMDevice *self, gboolean initial)
if (activation_source_is_scheduled (self,
activate_stage5_ip4_config_commit,
AF_INET)) {
-   priv->queued_ip4_config_id = g_idle_add 
(queued_ip4_config_change, self);
+   if (priv->queued_ip4_config_id) {
+   g_source_remove (priv->queued_ip4_config_id);
+   g_object_unref (self);
+   }
+   priv->queued_ip4_config_id = g_idle_add 
(queued_ip4_config_change, g_object_ref (self));
_LOGT (LOGD_DEVICE, "IP4 update was postponed");
return;
}
@@ -9031,7 +9037,11 @@ update_ip6_config (NMDevice *self, gboolean initial)
if (activation_source_is_scheduled (self,
activate_stage5_ip6_config_commit,
AF_INET6)) {
-   priv->queued_ip6_config_id = g_idle_add 
(queued_ip6_config_change, self);
+   if (priv->queued_ip6_config_id) {
+   g_source_remove (priv->queued_ip6_config_id);
+   g_object_unref (self);
+   }
+   priv->queued_ip6_config_id = g_idle_add 
(queued_ip6_config_change, g_object_ref (self));
_LOGT (LOGD_DEVICE, "IP6 update was postponed");
return;
}
@@ -9109,8 +9119,8 @@ queued_ip4_config_change (gpointer user_data)
return TRUE;
 
priv->queued_ip4_config_id = 0;
-   g_object_ref (self);
update_ip4_config (self, FALSE);
+
g_object_unref (self);
 
set_unmanaged_external_down (self, TRUE);
@@ -9131,7 +9141,6 @@ queued_ip6_config_change (gpointer user_data)
return TRUE;
 
priv->queued_ip6_config_id = 0;
-   g_object_ref (self);
update_ip6_config (self, FALSE);
 
if (   nm_platform_link_get (NM_PLATFORM_GET, priv->ifindex)
@@ -9194,7 +9203,7 @@ device_ipx_changed (NMPlatform *platform,
case NMP_OBJECT_TYPE_IP4_ADDRESS:
case NMP_OBJECT_TYPE_IP4_ROUTE:
if (!priv->queued_ip4_config_id) {
-   priv->queued_ip4_config_id = g_idle_add 
(queued_ip4_config_change, self);
+   priv->queued_ip4_config_id = g_idle_add 
(queued_ip4_config_change, g_object_ref (self));
_LOGD (LOGD_DEVICE, "queued IP4 config change");
}
break;
@@ -9211,7 +9220,7 @@ device_ipx_changed (NMPlatform *platform,
/* fallthrough */
case NMP_OBJECT_TYPE_IP6_ROUTE:
if (!priv->queued_ip6_config_id) {
-   priv->queued_ip6_config_id = g_idle_add 
(queued_ip6_config_change, self);
+   priv->queued_ip6_config_id = g_idle_add 
(queued_ip6_config_change, g_object_ref (self));
_LOGD (LOGD_DEVICE, "queued IP6 config change");
}
break;
-- 
2.7.4

___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

[PATCH] device: increase object ref count before invoking g_idle_add

[Shih-Yuan Lee (FourDollars)]

There are three different crash cases during the suspend&resume stress test.

Inreasing the object reference count before invoking g_idle_add() can avoid
this kind of race condition.

=== crash #1 ===
GNU gdb (Ubuntu 7.11-0ubuntu1) 7.11
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word".
Reading symbols from /usr/sbin/NetworkManager...Reading symbols from 
/usr/lib/debug/.build-id/15/8d5eb163d8235942d8d39bf004fe114445ea7a.debug...done.
done.
[New LWP 878]
[New LWP 987]
[New LWP 989]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/usr/sbin/NetworkManager --no-daemon'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  queued_ip6_config_change (user_data=0x24f7be0) at devices/nm-device.c:8913
8913if (priv->queued_state.id)
[Current thread is 1 (Thread 0x7f819a689940 (LWP 878))]
(gdb) p priv
$1 = (NMDevicePrivate *) 0x440
(gdb) bt
#0  queued_ip6_config_change (user_data=0x24f7be0) at devices/nm-device.c:8913
#1  0x7f819894bfda in g_main_context_dispatch () from 
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x7f819894c380 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#3  0x7f819894c6a2 in g_main_loop_run () from 
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#4  0x00444d0a in main (argc=1, argv=0x7ffd7a962dc8) at main.c:477
(gdb) l
8908NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
8909GSList *iter;
8910gboolean need_ipv6ll = FALSE;
8911
8912/* Wait for any queued state changes */
8913if (priv->queued_state.id)
8914return TRUE;
8915
8916priv->queued_ip6_config_id = 0;
8917g_object_ref (self);

=== crash #2 ===
GNU gdb (Ubuntu 7.11-0ubuntu1) 7.11
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word".
Reading symbols from /usr/sbin/NetworkManager...Reading symbols from 
/usr/lib/debug/.build-id/15/8d5eb163d8235942d8d39bf004fe114445ea7a.debug...done.
done.
[New LWP 776]
[New LWP 945]
[New LWP 943]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/usr/sbin/NetworkManager --no-daemon'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x7f2c583f78f0 in g_type_check_instance_cast () from 
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
[Current thread is 1 (Thread 0x7f2c59e39940 (LWP 776))]
(gdb) bt
#0  0x7f2c583f78f0 in g_type_check_instance_cast () from 
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#1  0x004775d1 in queued_ip6_config_change (user_data=0x1fbe220) at 
devices/nm-device.c:8907
#2  0x7f2c580fbfda in g_main_context_dispatch () from 
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#3  0x7f2c580fc380 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#4  0x7f2c580fc6a2 in g_main_loop_run () from 
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#5  0x00444d0a in main (argc=1, argv=0x7fffe2cd4418) at main.c:477
(gdb) up 1
#1  0x004775d1 in queued_ip6_config_change (user_data=0x1fbe220) at 
devices/nm-device.c:8907
8907NMDevice *self = NM_DEVICE (user_data);
(gdb) p self
$1 = 
(gdb) l
8902}
8903
8904static gboolean
8905queued_ip6_config_change (gpointer user_data)
8906{
8907NMDevice *self = NM_DEVICE (user_data);
8908NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
8909GSList *iter;
8910gboolean need_ipv6ll = FALSE;
8911
(gdb) info registers
rax0x1ee122032379424
rbx0x1fbe22033284640
rcx0x0  0
rdx0x1fbe22033284640