Re: [PATCH 2/2] Avoid network duplicate unref for eth_dev_remote
Hi Eduardo, Network unreference is already being done by free_network, called by g_hash_table_remove. This patche prevents from an invalid read during nework removal. I would be curious to see your valgrind output. The reference ethernet.c is removing is the one which is set when the network is created. device.c remove it's own reference (added in connman_device_add_network). If there is a reference bug, it does not seem to be where you found it. Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 2/2] Avoid network duplicate unref for eth_dev_remote
Hi Tomasz, Below you find my valgrind output. As you mentioned, the network reference device is freeing is the the one it creates, in this case, it is created in eth_newlink. I could not see any network referenced by eth which is not via device, so that's the reason I patched by removing network_unref and letting device manage it by its own. Cheers. = ==23771== Invalid read of size 4 ==23771==at 0x445A0F: connman_network_unref_debug (network.c:1037) ==23771==by 0x41F5A5: remove_network (ethernet.c:131) ==23771==by 0x41F86E: eth_dev_remove (ethernet.c:197) ==23771==by 0x4420C6: remove_device (device.c:295) ==23771==by 0x442149: remove_driver (device.c:310) ==23771==by 0x44229C: connman_device_driver_unregister (device.c:363) ==23771==by 0x41FCBD: ethernet_exit (ethernet.c:365) ==23771==by 0x440C45: __connman_plugin_cleanup (plugin.c:200) ==23771==by 0x43F5FB: main (main.c:697) ==23771== Address 0x7917d20 is 0 bytes inside a block of size 232 free'd ==23771==at 0x4C2BCD7: free (vg_replace_malloc.c:469) ==23771==by 0x445863: network_destruct (network.c:968) ==23771==by 0x445A98: connman_network_unref_debug (network.c:1045) ==23771==by 0x44230B: free_network (device.c:374) ==23771==by 0x4E6D8F9: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3800.1) ==23771==by 0x44310F: connman_device_remove_network (device.c:901) ==23771==by 0x41F586: remove_network (ethernet.c:130) ==23771==by 0x41F86E: eth_dev_remove (ethernet.c:197) ==23771==by 0x4420C6: remove_device (device.c:295) ==23771==by 0x442149: remove_driver (device.c:310) ==23771==by 0x44229C: connman_device_driver_unregister (device.c:363) ==23771==by 0x41FCBD: ethernet_exit (ethernet.c:365) ==23771== ==23771== Invalid read of size 8 ==23771==at 0x445A18: connman_network_unref_debug (network.c:1037) ==23771==by 0x41F5A5: remove_network (ethernet.c:131) ==23771==by 0x41F86E: eth_dev_remove (ethernet.c:197) ==23771==by 0x4420C6: remove_device (device.c:295) ==23771==by 0x442149: remove_driver (device.c:310) ==23771==by 0x44229C: connman_device_driver_unregister (device.c:363) ==23771==by 0x41FCBD: ethernet_exit (ethernet.c:365) ==23771==by 0x440C45: __connman_plugin_cleanup (plugin.c:200) ==23771==by 0x43F5FB: main (main.c:697) ==23771== Address 0x7917d38 is 24 bytes inside a block of size 232 free'd ==23771==at 0x4C2BCD7: free (vg_replace_malloc.c:469) ==23771==by 0x445863: network_destruct (network.c:968) ==23771==by 0x445A98: connman_network_unref_debug (network.c:1045) ==23771==by 0x44230B: free_network (device.c:374) ==23771==by 0x4E6D8F9: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.3800.1) ==23771==by 0x44310F: connman_device_remove_network (device.c:901) ==23771==by 0x41F586: remove_network (ethernet.c:130) ==23771==by 0x41F86E: eth_dev_remove (ethernet.c:197) ==23771==by 0x4420C6: remove_device (device.c:295) ==23771==by 0x442149: remove_driver (device.c:310) ==23771==by 0x44229C: connman_device_driver_unregister (device.c:363) ==23771==by 0x41FCBD: ethernet_exit (ethernet.c:365) On Thu, Apr 17, 2014 at 3:37 AM, Tomasz Bursztyka tomasz.burszt...@linux.intel.com wrote: Hi Eduardo, Network unreference is already being done by free_network, called by g_hash_table_remove. This patche prevents from an invalid read during nework removal. I would be curious to see your valgrind output. The reference ethernet.c is removing is the one which is set when the network is created. device.c remove it's own reference (added in connman_device_add_network). If there is a reference bug, it does not seem to be where you found it. Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 2/2] Avoid network duplicate unref for eth_dev_remote
Hi Eduardo, As you mentioned, the network reference device is freeing is the the one it creates, in this case, it is created in eth_newlink. I could not see any network referenced by eth which is not via device, so that's the reason I patched by removing network_unref and letting device manage it by its own. connman_network_create() sets a reference to 1. That's the first reference. The one who create - then get that first reference - should unreference it. This is what ethernet plugin is doing, logically in add_network/remove_network. According to your valgrind output, you have found a reference issue, but your fix is not proper. There is most probably a place which is unreferencing one time too much the network, thus breaking the logic in plugins/ethernet.c Hopefully, it's possible to follow who calls ref/unref functions. Could you send your connman debug logs? It will help to clarify the valgrind output. Cheers, Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 2/2] Avoid network duplicate unref for eth_dev_remote
Hi Tomasz, Connman logs: http://pastebin.com/qFGaw29x Cheers. On Thu, Apr 17, 2014 at 9:35 AM, Tomasz Bursztyka tomasz.burszt...@linux.intel.com wrote: Hi Eduardo, As you mentioned, the network reference device is freeing is the the one it creates, in this case, it is created in eth_newlink. I could not see any network referenced by eth which is not via device, so that's the reason I patched by removing network_unref and letting device manage it by its own. connman_network_create() sets a reference to 1. That's the first reference. The one who create - then get that first reference - should unreference it. This is what ethernet plugin is doing, logically in add_network/remove_network. According to your valgrind output, you have found a reference issue, but your fix is not proper. There is most probably a place which is unreferencing one time too much the network, thus breaking the logic in plugins/ethernet.c Hopefully, it's possible to follow who calls ref/unref functions. Could you send your connman debug logs? It will help to clarify the valgrind output. Cheers, Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 2/2] Avoid network duplicate unref for eth_dev_remote
Thanks, Now it's possible to pin point which part is bogus: - connman_network_create() network 0x749d890 - connman_network_ref_debug() 0x749d890 name Wired ref 2 by src/device.c:858:connman_device_add_network() - connman_network_ref_debug() 0x749d890 name Wired ref 3 by src/service.c:6670:update_from_network() - connman_network_ref_debug() 0x749d890 name Wired ref 4 by src/network.c:570:autoconf_ipv6_set() - connman_network_ref_debug() 0x749d890 name Wired ref 5 by src/dhcp.c:608:__connman_dhcp_start() - connman_network_unref_debug() 0x749d890 name Wired ref 4 by src/network.c:455:check_dhcpv6() - connman_network_unref_debug() 0x749d890 name Wired ref 3 by src/dhcp.c:145:dhcp_invalidate() - connman_network_unref_debug() 0x749d890 name Wired ref 2 by src/dhcp.c:145:dhcp_invalidate() - connman_network_unref_debug() 0x749d890 name Wired ref 1 by src/service.c:4455:service_free() - connman_network_unref_debug() 0x749d890 name Wired ref 0 by src/device.c:374:free_network() - connman_network_unref_debug() 0x749d890 name Wired ref -1 by plugins/ethernet.c:131:remove_network() Looks like the dhcp process is the culprit: it's unreferencing the network twice though it referenced it only once. 2 times dhcp_invalidate(). Can you check what's the proper fix there and resend a patch? Thanks, Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 2/2] Avoid network duplicate unref for eth_dev_remote
Surely. anyway, please check my first patch, applied in dhcp_invalidate. I going to investigate further this issue you just mentioned. BR. On Thu, Apr 17, 2014 at 10:08 AM, Tomasz Bursztyka tomasz.burszt...@linux.intel.com wrote: Thanks, Now it's possible to pin point which part is bogus: - connman_network_create() network 0x749d890 - connman_network_ref_debug() 0x749d890 name Wired ref 2 by src/device.c:858:connman_device_add_network() - connman_network_ref_debug() 0x749d890 name Wired ref 3 by src/service.c:6670:update_from_network() - connman_network_ref_debug() 0x749d890 name Wired ref 4 by src/network.c:570:autoconf_ipv6_set() - connman_network_ref_debug() 0x749d890 name Wired ref 5 by src/dhcp.c:608:__connman_dhcp_start() - connman_network_unref_debug() 0x749d890 name Wired ref 4 by src/network.c:455:check_dhcpv6() - connman_network_unref_debug() 0x749d890 name Wired ref 3 by src/dhcp.c:145:dhcp_invalidate() - connman_network_unref_debug() 0x749d890 name Wired ref 2 by src/dhcp.c:145:dhcp_invalidate() - connman_network_unref_debug() 0x749d890 name Wired ref 1 by src/service.c:4455:service_free() - connman_network_unref_debug() 0x749d890 name Wired ref 0 by src/device.c:374:free_network() - connman_network_unref_debug() 0x749d890 name Wired ref -1 by plugins/ethernet.c:131:remove_network() Looks like the dhcp process is the culprit: it's unreferencing the network twice though it referenced it only once. 2 times dhcp_invalidate(). Can you check what's the proper fix there and resend a patch? Thanks, Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/2] Avoid network duplicate unref for eth_dev_remote
Network unreference is already being done by free_network, called by g_hash_table_remove. This patche prevents from an invalid read during nework removal. --- plugins/ethernet.c | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/ethernet.c b/plugins/ethernet.c index b8e52ce..aa1153b 100644 --- a/plugins/ethernet.c +++ b/plugins/ethernet.c @@ -128,7 +128,6 @@ static void remove_network(struct connman_device *device, return; connman_device_remove_network(device, ethernet-network); - connman_network_unref(ethernet-network); ethernet-network = NULL; } -- 1.8.3.2 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
