Bug 1277171

Backtrace from the testing machine:

NetworkManager[16397]: <info>  dhclient started with pid 16522
NetworkManager[16397]: <info>  (eth1): DHCPv4 client pid 16522 killed by signal 15
NetworkManager[16397]: <info>  (eth1): DHCPv4 state changed unknown -> fail
NetworkManager[16397]: <info>  (eth1): canceled DHCP transaction
NetworkManager[16397]: <info>  (eth1): DHCPv4 state changed fail -> done
NetworkManager[16397]: linklocal6_complete: assertion 'FALSE' failed

Program received signal SIGTRAP, Trace/breakpoint trap.
g_logv (log_domain=0x5555556ae4c0 "NetworkManager", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7fffffffddb0) at gmessages.c:1046
1046		  g_private_set (&g_log_depth, GUINT_TO_POINTER (depth));
(gdb) bt
#0  g_logv (log_domain=0x5555556ae4c0 "NetworkManager", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7fffffffddb0) at gmessages.c:1046
#1  0x00007ffff4a4ea3f in g_log (log_domain=<optimized out>, log_level=<optimized out>, format=<optimized out>) at gmessages.c:1079
#2  0x00005555555b4f11 in queued_ip6_config_change (user_data=<optimized out>) at devices/nm-device.c:7476
#3  0x00007ffff4a4779a in g_main_dispatch (context=0x55555593e400) at gmain.c:3109
#4  g_main_context_dispatch (context=context@entry=0x55555593e400) at gmain.c:3708
#5  0x00007ffff4a47ae8 in g_main_context_iterate (context=0x55555593e400, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3779
#6  0x00007ffff4a47dba in g_main_loop_run (loop=0x55555593e4c0) at gmain.c:3973
#7  0x00005555555970a3 in main (argc=1, argv=0x7fffffffe198) at main.c:512
(gdb) 

It seems to be a bit misleading.
Actually
  g_return_if_fail (FALSE); in linklocal6_complete() was hit because method was "shared".

Created attachment 1088627 [details]
[PATCH] don't use g_return_if_fail() for non-programming error case

Well, NetworkManager terminated because of setting G_DEBUG to fatal-warnings on the machine. But g_return_if_fail() should not be used in this case, IMO.

Created attachment 1088628 [details]
[PATCH] don't use g_return_if_fail() for non-programming error case

Correction of the patch

I think that the original code was right, if the IPv6 method is
'shared', link-local configuration should not have been started
and if we reach that line there's a bug somewhere.

I suspect that the root cause is that we are reading the ipv6.method
from the settings_connection (which can change on the fly). Instead we
should read it from the applied_connection?

(In reply to Beniamino Galvani from comment #7)
> I think that the original code was right, if the IPv6 method is
> 'shared', link-local configuration should not have been started
> and if we reach that line there's a bug somewhere.

I tend to agree with Beniamino.



> I suspect that the root cause is that we are reading the ipv6.method
> from the settings_connection (which can change on the fly). Instead we
> should read it from the applied_connection?

Vladimir testes RHEL-7.2/nm-1-0. Applied-connection split is only on master. But yes, let's investigate master too.




The commit message reads:

    The g_return family of macros (g_return_if_fail()) should only be used for
    programming errors. And this is not the case here. Turning on GLib debugging
    would terminate NetworkManager.

it is well understood that g_return_if_fail() is not supposed to affect bug-free control-flow. I added it there, because I wanted to assert that this condition doesn't happen. The commit message better should explain, why the assertion is wrong at this place.

Dropping the assertion entirely is wrong. If you really expect shared there, then let's do:

     } else if (strcmp (method, NM_SETTING_IP6_CONFIG_METHOD_LINK_LOCAL) == 0)
          nm_device_activate_schedule_ip6_config_result (self);
+    } else if (strcmp (method, NM_SETTING_IP6_CONFIG_METHOD_SHARED) == 0) {
+         /* handle it somehow?? */
+    } else
+         g_return_if_reached ();



(note that g_return_if_reached() is better then g_return_if_fail(FALSE). I didn't get that originally).

(In reply to Beniamino Galvani from comment #7)
> I think that the original code was right, if the IPv6 method is
> 'shared', link-local configuration should not have been started
> and if we reach that line there's a bug somewhere.

I think in this case, the original IPv6 method was 'auto' or 'link-local' and so LL was started, but then before LL finished it got changed to 'shared'.  So I can see why the linklocal6_complete() code would be executed.

I agree with the g_return_if_reached() though.

In the end, this is an NM bug that a user can trigger and not really their fault, so we should handle it by not crashing at least.  Maybe if the method isn't recognized from linklocal6_complete() we should fail the activation?

Pushed the updated patch to jk/ipv6-shared-SIGTRAP-rh1277171

jk/ipv6-shared-SIGTRAP-rh1277171 LGTM

jk/ipv6-shared-SIGTRAP-rh1277171 has the patch; just need to merge to master and backport?

This is a crash due to a known race when modifying the settings connection during activation.

Fixed in master by splitting settings and applied connections.

Created attachment 1112146 [details]
patch from jk/ipv6-shared-SIGTRAP-rh1277171

I attach the patch from jk/ipv6-shared-SIGTRAP-rh1277171 here for reference.

I think it should be rejected because:

- the crash happened due to a "g_return_if_fail (FALSE)". This kind of assertion is already forgiving to the user which does not cause a real crash (unless user configured fatal-criticals).
- the patch doesn't fix the real issue, it just silences a non-fatal, correct assertion.