Discussion:
[Question] When an interface is removed, it may cause IPv6 to be disabled on all other interfaces
(too old to reply)
Abtin Keshavarzian
2015-10-05 21:46:43 UTC
Permalink
Hi,

We have been seeing some strange behavior in our network using Connman with
IPv6 addresses.

We wanted to check if this happens to be a known issue, or if we are
missing
something here.

We are using Connman version 1.29. We have multiple network interfaces some
with IPv6 addressing enabled. We notice that when an interface is brought
down
removed), it can cause IPv6 to be disabled on all other interfaces (thus
removing
all IPv6 addresses and routes).

We tracked the source of this as follows: (all methods/functions are in
ipconfig.c):

- When the interface is brought down, we get a DELLINK message in Connman
- This triggers a call to '__connman_ipconfig_dellink()' in ipconfig.c
- In this function at the very end we remove the ipdevice from
'ipdevice_hash' using
'g_hash_table_remove(ipdevice_hash, GINT_TO_POINTER(index))'
- This indirectly causes a call to `free_ipdevice()' to free the associated
ipdevice
element in
the hash table
- In 'free_ipdevice()' we get the interface name using
'connman_inet_ifname(index)' from the
index which is stored in 'ipdevice->index'
- 'connman_inet_ifname(index)' uses ioctl to get the ifname, but since the
interface is (being)
removed, there is no name and we return NULL as ifname
- Back in 'free_ipdevice()' we call 'set_ipv6_state(ifname,
ipdevice->ipv6_enabled)'
- In 'set_ipv6_state()' if the passed in ifname is null, then the change is
applied to all interfaces
- So we disable ipv6 on all interfaces (instead of the one being removed).

We have the Connman logs showing this chain of events which I'd be happy to
share.
The quick fix we are using now is in 'free_ipdevice()' as follows:

+++ connman/src/ipconfig.c
@@ -422,8 +402,10 @@ static void free_ipdevice(gpointer data)

g_free(ipdevice->address);

- set_ipv6_state(ifname, ipdevice->ipv6_enabled);
- set_ipv6_privacy(ifname, ipdevice->ipv6_privacy);
+ if (ifname) {
+ set_ipv6_state(ifname, ipdevice->ipv6_enabled);
+ set_ipv6_privacy(ifname, ipdevice->ipv6_privacy);
+ }

g_free(ifname);
g_free(ipdevice);

Basically checking the 'ifname' and only invoking 'set_ipv6_state()' with a
non-null 'ifname'

Also about the variable 'bool ipv6_enabled' in 'struct connman_ipdevice':

- This value is set initially in '__connman_ipconfig_newlink()' when a new
ipdevice is created.
- And it is used only when the ipdevice is freed in 'free_ipdevice()'
- This variable is not changed when 'enable_ipv6()' or 'disable_ipv6()' is
called.

Is this the intended use for this variable? i.e., remembering the initial
state and setting it when the
ipdevice is deleted.

Thanks,
Abtin.
Patrik Flykt
2015-10-08 07:32:33 UTC
Permalink
Hi,
Post by Abtin Keshavarzian
We tracked the source of this as follows: (all methods/functions are in
- When the interface is brought down, we get a DELLINK message in Connman
- This triggers a call to '__connman_ipconfig_dellink()' in ipconfig.c
- In this function at the very end we remove the ipdevice from
'ipdevice_hash' using
'g_hash_table_remove(ipdevice_hash, GINT_TO_POINTER(index))'
- This indirectly causes a call to `free_ipdevice()' to free the associated
ipdevice
element in
the hash table
- In 'free_ipdevice()' we get the interface name using
'connman_inet_ifname(index)' from the
index which is stored in 'ipdevice->index'
- 'connman_inet_ifname(index)' uses ioctl to get the ifname, but since the
interface is (being)
removed, there is no name and we return NULL as ifname
- Back in 'free_ipdevice()' we call 'set_ipv6_state(ifname,
ipdevice->ipv6_enabled)'
- In 'set_ipv6_state()' if the passed in ifname is null, then the change is
applied to all interfaces
- So we disable ipv6 on all interfaces (instead of the one being removed).
Nice work, thanks for the detailed report!
Post by Abtin Keshavarzian
We have the Connman logs showing this chain of events which I'd be happy to
share.
+++ connman/src/ipconfig.c
@@ -422,8 +402,10 @@ static void free_ipdevice(gpointer data)
g_free(ipdevice->address);
- set_ipv6_state(ifname, ipdevice->ipv6_enabled);
- set_ipv6_privacy(ifname, ipdevice->ipv6_privacy);
+ if (ifname) {
+ set_ipv6_state(ifname, ipdevice->ipv6_enabled);
+ set_ipv6_privacy(ifname, ipdevice->ipv6_privacy);
+ }
g_free(ifname);
g_free(ipdevice);
Basically checking the 'ifname' and only invoking 'set_ipv6_state()' with a
non-null 'ifname'
Yes, that looks like the quick fix right now. Can you send a patch for
this?

The long term solution is to use the ipdevice data structure instead of
the ioctl in connman_inet_if{name,index} as ipdevice properly follows
the change in interface names anyway. I'll look into that if I have
time...
Post by Abtin Keshavarzian
- This value is set initially in '__connman_ipconfig_newlink()' when a new
ipdevice is created.
- And it is used only when the ipdevice is freed in 'free_ipdevice()'
- This variable is not changed when 'enable_ipv6()' or 'disable_ipv6()' is
called.
Is this the intended use for this variable? i.e., remembering the initial
state and setting it when the
ipdevice is deleted.
The commit where this was introduced is
d4e1dfc943644199849eb3de2463eb0ca0344408 and the change says "Restore
original IPv6 interface status when connman quits". So yes, that's the
inteded behavior.

Thanks,

Patrik

Loading...