Bug 1298007 - [abrt] NetworkManager: nm_supplicant_interface_get_scanning(): NetworkManager killed by SIGSEGV
[abrt] NetworkManager: nm_supplicant_interface_get_scanning(): NetworkManager...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: NetworkManager (Show other bugs)
23
i686 Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Thomas Haller
Fedora Extras Quality Assurance
https://retrace.fedoraproject.org/faf...
abrt_hash:c830d615783ab643271e67bfe48...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-12 19:31 EST by Yonatan
Modified: 2016-04-03 13:51 EDT (History)
5 users (show)

See Also:
Fixed In Version: NetworkManager-1.0.12-1.fc23
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-04-03 13:51:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
File: backtrace (16.57 KB, text/plain)
2016-01-12 19:31 EST, Yonatan
no flags Details
File: cgroup (195 bytes, text/plain)
2016-01-12 19:31 EST, Yonatan
no flags Details
File: core_backtrace (5.35 KB, text/plain)
2016-01-12 19:31 EST, Yonatan
no flags Details
File: dso_list (5.96 KB, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: environ (72 bytes, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: exploitable (110 bytes, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: limits (1.29 KB, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: maps (16.94 KB, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: mountinfo (3.25 KB, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: namespaces (85 bytes, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: open_fds (2.86 KB, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: proc_pid_status (839 bytes, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details
File: var_log_messages (14.52 KB, text/plain)
2016-01-12 19:32 EST, Yonatan
no flags Details

  None (edit)
Description Yonatan 2016-01-12 19:31:37 EST
Version-Release number of selected component:
NetworkManager-1.0.8-1.fc23

Additional info:
reporter:       libreport-2.6.3
backtrace_rating: 4
cmdline:        /usr/sbin/NetworkManager --no-daemon
crash_function: nm_supplicant_interface_get_scanning
executable:     /usr/sbin/NetworkManager
global_pid:     4733
kernel:         4.2.8-300.fc23.i686
runlevel:       N 5
type:           CCpp
uid:            0

Truncated backtrace:
Thread no. 1 (9 frames)
 #0 nm_supplicant_interface_get_scanning at supplicant-manager/nm-supplicant-interface.c:329
 #1 scanning_allowed at nm-device-wifi.c:1366
 #2 ffi_call_SYSV at ../src/x86/sysv.S:65
 #3 ffi_call at ../src/x86/ffi.c:382
 #4 g_cclosure_marshal_generic at gclosure.c:1487
 #8 g_signal_emitv at gsignal.c:3122
 #9 check_scanning_allowed at nm-device-wifi.c:1418
 #10 request_wireless_scan at nm-device-wifi.c:1495
 #11 request_wireless_scan_periodic at nm-device-wifi.c:1550
Comment 1 Yonatan 2016-01-12 19:31:51 EST
Created attachment 1114194 [details]
File: backtrace
Comment 2 Yonatan 2016-01-12 19:31:55 EST
Created attachment 1114195 [details]
File: cgroup
Comment 3 Yonatan 2016-01-12 19:31:59 EST
Created attachment 1114196 [details]
File: core_backtrace
Comment 4 Yonatan 2016-01-12 19:32:03 EST
Created attachment 1114197 [details]
File: dso_list
Comment 5 Yonatan 2016-01-12 19:32:08 EST
Created attachment 1114198 [details]
File: environ
Comment 6 Yonatan 2016-01-12 19:32:13 EST
Created attachment 1114199 [details]
File: exploitable
Comment 7 Yonatan 2016-01-12 19:32:17 EST
Created attachment 1114200 [details]
File: limits
Comment 8 Yonatan 2016-01-12 19:32:22 EST
Created attachment 1114201 [details]
File: maps
Comment 9 Yonatan 2016-01-12 19:32:27 EST
Created attachment 1114202 [details]
File: mountinfo
Comment 10 Yonatan 2016-01-12 19:32:32 EST
Created attachment 1114203 [details]
File: namespaces
Comment 11 Yonatan 2016-01-12 19:32:36 EST
Created attachment 1114204 [details]
File: open_fds
Comment 12 Yonatan 2016-01-12 19:32:39 EST
Created attachment 1114205 [details]
File: proc_pid_status
Comment 13 Yonatan 2016-01-12 19:32:45 EST
Created attachment 1114206 [details]
File: var_log_messages
Comment 14 Thomas Haller 2016-01-19 16:49:48 EST
fix for review at: th/supplicant-manager-fix-ref-count-rh1298007
Comment 15 Dan Williams 2016-01-20 10:59:24 EST
> device/ethernet: cleanup clearing handlers registered to supplicant interface

supplicant_interface_release() no longer removes the priv->supplicant.iface_state_id handler, so the signal never gets disconnected.


> supplicant: don't pass start_now argument to nm_supplicant_interface_new()

Instead of starting the interface if the die_count has not been exceeded, shouldn't it just use is_available()?  That already takes the die_count into account and also checks priv->running.  A bug in the old code, but we should fix that up.  The rest of the patch is fine, but we can just get rid of the "start_now" variable entirely and just do:

if (is_available (self))
	nm_supplicant_interface_set_supplicant_available (iface, TRUE);

We must have the if() there unfortunately, because the supplicant_interface doesn't track the supplicant_available state internally and ignore the call if the state doesn't actually change.  But that's OK.


> wifi: fix crash due to wrong ownership handling in nm_supplicant_manager_iface_release()

nm_supplicant_manager_create_interface() should really check if there's an existing NMSupplicantInterface for the same 'iface' name and bail out early with a big warning.  We really need to know what causes that problem, because it should never ever happen.
Comment 16 Thomas Haller 2016-01-20 11:24:10 EST
(In reply to Dan Williams from comment #15)
> > device/ethernet: cleanup clearing handlers registered to supplicant interface
> 
> supplicant_interface_release() no longer removes the
> priv->supplicant.iface_state_id handler, so the signal never gets
> disconnected.

done in fixup! commit


> > supplicant: don't pass start_now argument to nm_supplicant_interface_new()
> 
> Instead of starting the interface if the die_count has not been exceeded,
> shouldn't it just use is_available()?  That already takes the die_count into
> account and also checks priv->running.  A bug in the old code, but we should
> fix that up.  The rest of the patch is fine, but we can just get rid of the
> "start_now" variable entirely and just do:
> 
> if (is_available (self))
> 	nm_supplicant_interface_set_supplicant_available (iface, TRUE);
> 
> We must have the if() there unfortunately, because the supplicant_interface
> doesn't track the supplicant_available state internally and ignore the call
> if the state doesn't actually change.  But that's OK.

done in fixup.


> > wifi: fix crash due to wrong ownership handling in nm_supplicant_manager_iface_release()
> 
> nm_supplicant_manager_create_interface() should really check if there's an
> existing NMSupplicantInterface for the same 'iface' name and bail out early
> with a big warning.  We really need to know what causes that problem,
> because it should never ever happen.

done in fixup.



Repushed.
Comment 17 Beniamino Galvani 2016-01-20 12:36:31 EST
+       /* manager caches and reuses teh supplicant. In case the supplicant is already
+        * registered, aquire a the supplicant first. */
+       supplicant_interface_release (self);
+       if (!sup_iface) {

Isn't this comment outdated? (The manager doesn't cache the supplicant
interface anymore I think).


 static NMSupplicantConfig *
 build_supplicant_config (NMDeviceWifi *self,
                          NMConnection *connection,
                          guint32 fixed_freq,
                          GError **error)
 {
-       NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
+       NMDeviceWifiPrivate *priv;
[...]
-       g_return_val_if_fail (self != NULL, NULL);
+       g_return_val_if_fail (self, NULL);
+
+       priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
+       g_return_val_if_fail (priv->sup_iface, NULL);

Nitpick: in my opinion we should keep:

  NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self)

and remove

  g_return_val_if_fail (self != NULL, NULL)'

since the former already throws an assertion if the pointer is NULL.


   /* priv->ifaces may be modified if availability changes; can't use GHashTableIter */
-  ifaces = g_hash_table_get_values (priv->ifaces);

The comment is obsolete now.


-       g_hash_table_remove (priv->ifaces, ifname);
+       priv->ifaces = g_slist_remove (priv->ifaces, sup_iface);
+       g_object_remove_toggle_ref ((GObject *) sup_iface, _sup_iface_last_ref, self);

I'm not very familiar with toggle references, does it make any
difference here between g_object_remove_toggle_ref() and
g_object_unref()?
Comment 18 Thomas Haller 2016-01-20 13:11:29 EST
(In reply to Beniamino Galvani from comment #17)
> +       /* manager caches and reuses teh supplicant. In case the supplicant
> is already
> +        * registered, aquire a the supplicant first. */
> +       supplicant_interface_release (self);
> +       if (!sup_iface) {
> 
> Isn't this comment outdated? (The manager doesn't cache the supplicant
> interface anymore I think).

fixup added.


>  static NMSupplicantConfig *
>  build_supplicant_config (NMDeviceWifi *self,
>                           NMConnection *connection,
>                           guint32 fixed_freq,
>                           GError **error)
>  {
> -       NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
> +       NMDeviceWifiPrivate *priv;
> [...]
> -       g_return_val_if_fail (self != NULL, NULL);
> +       g_return_val_if_fail (self, NULL);
> +
> +       priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
> +       g_return_val_if_fail (priv->sup_iface, NULL);
> 
> Nitpick: in my opinion we should keep:
> 
>   NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self)
> 
> and remove
> 
>   g_return_val_if_fail (self != NULL, NULL)'
> 
> since the former already throws an assertion if the pointer is NULL.

fixup!



>    /* priv->ifaces may be modified if availability changes; can't use
> GHashTableIter */
> -  ifaces = g_hash_table_get_values (priv->ifaces);
> 
> The comment is obsolete now.

comment updated.


> -       g_hash_table_remove (priv->ifaces, ifname);
> +       priv->ifaces = g_slist_remove (priv->ifaces, sup_iface);
> +       g_object_remove_toggle_ref ((GObject *) sup_iface,
> _sup_iface_last_ref, self);
> 
> I'm not very familiar with toggle references, does it make any
> difference here between g_object_remove_toggle_ref() and
> g_object_unref()?

remove_toggle_ref() is an object_unref() and unsubscribing the toggle callback.

I think this works nice, because the manager owns a reference to the object, but if everybody else drops it's reference, the manager destroys the object.
Only downside is, that the object is owned by the manager too, which is non-obvious. I added a comment about that.



Repushed.
Comment 19 Thomas Haller 2016-01-21 09:13:00 EST
merged to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=de19bc20d041c3a5267dc05303b24e5871d10632


A simplified fix was added to nm-1-0: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=bd27102277e5d7e52d87bd26711ae6c431e08192






Note that this does only converts the crash of the reporter to a (non-crashing) assertion. As syslog from comment 13 shows, the issue is that the wifi devices got renamed, but seems that the renaming didn't clean up the supplicant interface (leading to creating supplicant interfaces for wlan0 twice).

Leaving BZ open
Comment 20 Thomas Haller 2016-03-02 10:05:08 EST
I backported the nm-1-0 workaround [1] to Fedora 22 and Fedora 23.

It will be in updates
https://bodhi.fedoraproject.org/updates/NetworkManager-1.0.10-3.fc23
https://bodhi.fedoraproject.org/updates/NetworkManager-1.0.10-3.fc22


I don't close this bug, because the patch only avoids the crash, but doesn't properly handle renaming of wifi devices.


[1] http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=bd27102277e5d7e52d87bd26711ae6c431e08192
Comment 21 Fedora Update System 2016-04-02 01:25:46 EDT
NetworkManager-1.0.12-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-8201e3fefa
Comment 22 Fedora Update System 2016-04-03 13:50:58 EDT
NetworkManager-1.0.12-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.