I'll try to write up an rough overview...

To enable IPv6 on a 3G mobile broadband connection, the first step is to request a IPv6 capable PDP context. Two such PDP context types exist:

* "IPV6" - a single-stack IPv6-only bearer
* "IPV4V6" - a dual-stack IPv4+IPv6 bearer.

Note that the modem may implement IPV4V6 by setting up two simultaneous single-stack PDP contexts (IP+IPV6) if the network doesn't support IPV4V6 natively.

To check what PDP types the modem supports, use "AT+CGDCONT=?". I don't know if there's any way to find out what the network supports, other than trying and failing.

The network will supply an interface ID (in PCO) that the MS is supposed to use to construct an IPv6 link-local address. To get a global IPv6 address, and an IPv6 default route, SLAAC is used. The network's GGSN will transmit ICMPv6 Router Advertisements to the MS.

The IPv6 DNS server address will be communicated using PCO. There is nothing preventing the network from additionally communicating it using the RDNSS option in its RAs or stateless DHCPv6 (or both), but this is not commonly done today due to lack of support in popular GGSN implementations.

An IPv6-enabled 3GPP link basically be treated the same as regular Ethernet - i.e., NM should listen for RAs, let the kernel perform SLAAC, and start DHCPv6 if the M- or O-flag in the RA is set.

There are a few 3GPP-specific gotchas/considerations, though:

* How to learn the DNS server address

Since current networks usually doesn't support DHCPv6 or the RNDSS RA option for communicating the DNS server address, the DNS server address must be extracted straight from PCO data. This can often be done using proprietary AT commands (e.g. for Icera: "AT%IPDPADDR", for Ericsson MBM: "AT*E2IPCFG?").

Also worth noting is that some Ericsson MBMs (at least the F5321gw) implements a stateless DHCPv6 server in firmware, and will change the RA coming from the network by setting the OtherConfig flag. The OS can then proceed to perform a DHCPv6 Information-Request, and thus learning the DNS server address in a completely standard way.

* Incorrect link-local address configured when using Ethernet emulation

As mentioned above, the network will provide an interface ID to the MS when the connection is set up, and expects the MS to use it when constructing its link local address. However, when Ethernet emulation is used, the MS' OS will typically have configured another link-local address based on running the EUI-64 algorithm on the invented/fake MAC address (provided by firmware or the driver).

This may cause some subtle problems, e.g., if the MS sends an ICMPv6 Router Solicitation to the network, the GGSN may respond with an RA unicasted to the link-local address the network expects the MS to have. However, since the OS doesn't have this link-local address configured on the interface, the RA is discarded.

In order to be compliant with the 3GPP standards, therefore, the default EUI64-derived link-local address should be replaced with the correct network-provided one as soon as the link is up. This address may be obtained using the same proprietary commands as may be used to learn the DNS server address.

Note that this is not a problem when in PPP mode, as IPV6CP will ensure that the correct network-provided link-local address is used on the PPP interface.

* Ethernet emulation may not support IPv6 (even though the modem does)

The Ericsson F3507g supports the IPV6 PDP type, and connections may be successfully established using PPP/IPV6CP. However, this simply does not work when using Ethernet emulation - PPP must be used for IPv6 to work on this module. This may well apply for other modules as well.

* Dual-stack may require special consideration

If the module doesn't support the IPV4V6 type (true for the F3507g and F5321gw), or if the network doesnt and the modem fallback to IP+IPV6, NM/MM must establish two parallel IP+IPV6 data bearers in order to get dual-stack connectivity. Note that if the module doesn't support multiplexing two bearers into the same emulated Ethernet device (or doesn't provide multiple Ethernet devices), this means that at least one of the protocols must be relegated to using PPP mode. This is the case for the F5321gw at least.

Which protocol should get preferential treatment (by getting the Ethernet interface) may also require some thought or manual configuration. In general, if the network provides DNS64/NAT64 service for IPv6 clients, most of the traffic will likely be IPv6, hence IPv6 should get preferential treatment.

Even without DNS64/NAT64, IPv6 will likely have a large share of the total traffic (due to large content providers like Google and Facebook being available over IPv6), and as this share is expected to increase over time, it may be prudent to always grant the Ethernet interface to IPv6 by default (unless the modem doesn't support it of course, see above point).

See also this presentation by T-Mobile USA: http://conference.apnic.net/__data/assets/pdf_file/0010/58870/tmo-ipv6-feb-2013_1361827441.pdf, slide 10, which mentions 50% as the share of typical end-user traffic expected to go over IPv6 today, if the user is dual-stacked.

* 464XLAT for providing IPv4 support on IPv6-only networks

If the network only supports the IPV6 PDP type, and are also providing DNS64/NAT64 service, 464XLAT may be used in order to create a virtual IPv4 interface on the MS that IPv4-only applications may bind to. While I'm not aware of any implementation of 464XLAT for Linux at the moment, it would be wise to implement IPv6 mobile broadband support in such a way that facilitates for easy addition of support for 464XLAT at a later time.

http://tools.ietf.org/html/rfc6877

* IPv6 internet sharing/hotspot support

The 3GPP network will delegate a whole /64 to the MS, which may be shared. The following draft documents how this can be done:

http://tools.ietf.org/html/draft-ietf-v6ops-64share

Note that networks conforming to 3GPP Rel-10 will also support DHCPv6-PD for obtaining prefixes for downstream use. However it will be a while before this is generally deployed. Also, the 64share method is expected to continue working in Rel-10(+) networks as well.

Tore

On to some practical stuff, where to start and so on...

Currently, my main problem with the MM/NM combo in F18 is that it doesn't seem to accept the fact that there may be IPv6 on a mobile connection, and that there may not be IPv4.

I've tried adding to the connection's config file:

[ipv4]
method=auto
may-fail=true

[ipv6]
method=auto
may-fail=true

...but that doesn't seem to work, it ignores IPv6 and have a hard dependency on IPv4 in order to succeed. In the nm-connection-editor GUI there's no IPv6 tab at all. So a start would be to unlock the standard IPv6 stuff, i.e., treat the 3G connection as a regular interface and run through the standard IPv6 device activation routines (looking for RAs, doing DHCPv6 if instructed to by RA, and so on). This should allow for basic IPv6-only or dual-stack (IPV4V6) connectivity. The specific 3GPP tweaks can be added afterwards.

Another thing worth mentioning is that MM will default to "IP" (i.e., IPv4) setting up the PDP context parameters (i.e., AT+CGDCONT=<cid>,"IP","apnname"). There appears to be no way to override this using NM config that I can see.

Fortunately, the PDP type can be easily overridden by issuing a manual AT+CGDCONT command to the modem - it appears that MM only considers the APN name when looking for a pre-defined PDP context, so by setting it to use IPV6 or IPV4V6 while using the expected APN name, MM will re-use it as-is, yielding an IPv6-enabled data bearer. This works for me, but it should of course become possible to configure through NM eventually.

Tore

Just change the title to avoid clash with the unrelated Mobile IPv6 standard.

NM bugzilla reorganization... sorry for the bug spam.

*** Bug 731483 has been marked as a duplicate of this bug. ***

Fixes posted to dcbw/wwan-ipv6 branch.  Requires the ModemManager dcbw/ipv6-fixes branch but should work without it too.

> rdisc: allow prefix parsing on non-Ethernet interfaces like PPP

if the MAC address is not set, then the host-part is all zero. That seems wrong. 

I think without MAC address (interface identifier) you cannot generate a stable public address (according to rfc4862, except via rfc7217). You could fill it with random, but then it wouldn't be stable...

Or maybe you can extend the commit message explaining why this makes sense?

And if this commit actually fixes that question, then you can remove the comment:
  /* FIXME: what if interface has no lladdr, like PPP? */
in addrconf6_start_with_link_ready().




> core: track a Device's IP interface hardware address

How about combining the length and data of the address in a struct:

+typedef struct {
+    guint8 hw_addr[NM_UTILS_HWADDR_LEN_MAX];
+    guint8 len;
+} NMPlatformHwAddress;

See th/NMPlatformHwAddress for a showcase. I would refactor the use of hw-addresses to pass around this struct instead, except for gobject properties, that should expose the hwaddr as GByteArray*.

(In reply to comment #7)
> > rdisc: allow prefix parsing on non-Ethernet interfaces like PPP
> 
> if the MAC address is not set, then the host-part is all zero. That seems
> wrong. 
> 
> I think without MAC address (interface identifier) you cannot generate a stable
> public address (according to rfc4862, except via rfc7217). You could fill it
> with random, but then it wouldn't be stable...
> 
> Or maybe you can extend the commit message explaining why this makes sense?
> 
> And if this commit actually fixes that question, then you can remove the
> comment:
>   /* FIXME: what if interface has no lladdr, like PPP? */
> in addrconf6_start_with_link_ready().

You're right, I need to fix this up to work with generic Interface Identifiers.  RIght now the rdisc code doesn't work with PPP or IPoIB because they don't have 6-byte MAC addresses, but they do have interface identifiers which should be used instead.

> > core: track a Device's IP interface hardware address
> 
> How about combining the length and data of the address in a struct:
> 
> +typedef struct {
> +    guint8 hw_addr[NM_UTILS_HWADDR_LEN_MAX];
> +    guint8 len;
> +} NMPlatformHwAddress;
> 
> See th/NMPlatformHwAddress for a showcase. I would refactor the use of
> hw-addresses to pass around this struct instead, except for gobject properties,
> that should expose the hwaddr as GByteArray*.

Why not just use GByteArray everywhere, since that's bascially the same thing?

I've reworked and re-pushed the branch.  Notable changes:

1) properly fixed the rdisc stuff to use interface identifiers, and fix up NMDevice, NMDeviceInfiniband, and NMDeviceModem to generate them

2) changed PPP to just push the interface identifiers to NM, and NM creates the actual IPv6LL addresses from those, and also passes the IID on to rdisc

3) new commit to watch the NMDevice::ip-iface property and remove child devices (like ppp0) when necessary; turned out that danw's new fixes for rechecking connection assume took over configuration of the PPP interface unexpectedly, and we have to prevent that.  In the future (1.0?) we'll deal with this properly and hopefully have parent/child device relationships.

(In reply to comment #8)
> (In reply to comment #7)
> > > core: track a Device's IP interface hardware address
> > 
> > How about combining the length and data of the address in a struct:
> > 
> > +typedef struct {
> > +    guint8 hw_addr[NM_UTILS_HWADDR_LEN_MAX];
> > +    guint8 len;
> > +} NMPlatformHwAddress;
> > 
> > See th/NMPlatformHwAddress for a showcase. I would refactor the use of
> > hw-addresses to pass around this struct instead, except for gobject properties,
> > that should expose the hwaddr as GByteArray*.
> 
> Why not just use GByteArray everywhere, since that's bascially the same thing?

That was my first suggestions. I revised it because I think NMPlatformHwAddress has the following pro and cons.:

+:
  A) it's more explicit(!)
  B) if needed, we could easily extend it to add the HwAddress type field.
  C) can be embedded in a private struct
     (no need to dispose(), cannot be NULL << less typing)
-:
  a) not ref-countable (OTOH, also a 'const GByteArray *' would need to be 
     copied most of the time)
  b) gobject properties should probably still be exposed as GByteArray*

>> core: track a Device's IP interface hardware address

+                   g_assert (addrlen <= sizeof (priv->ip_hw_addr));

If you decide to assert against this condition, it should be done earlier as well: in link_get_address().

I pushed a commit for that, it also handles the unexpected case gracefully.
  "platform: assert against the maximum length of link_get_address()"



>> core: use Interface Identifiers for IPv6 SLAAC addresses

+    /* Use the port GUID per http://tools.ietf.org/html/rfc4391#section-8,
+     * making sure to set the 'u' bit to 1.  The GUID is the lower 64 bits
+     * of the IPoIB interface's hardware address.
+     */

Isn't the right RFC this one: http://tools.ietf.org/html/rfc4291#page-20.
Also, you only "invert/flip" the 'u' bit

I think, all these methods to create interface identifiers should be separate functions. E.g. nm_utils_ipv6_iface_id_from_eui64()


+#define NM_RDISC_IID_LEN 8

I would call this, NM_UTILS_IPV6_IFACE_ID_LEN (because it is also relevant for LL and outside of RDISC). Also, I would create a set of related nm_utils_ipv6_iface_id_*() functions.


I would even create a type

typedef struct {
    union {
        guint64 id;
        guint8 id_u8[NM_UTILS_IPV6_IFACE_ID_LEN];
        guint16 id_u16[NM_UTILS_IPV6_IFACE_ID_LEN / 2];
        guint32 id_u32[NM_UTILS_IPV6_IFACE_ID_LEN / 4];
        guint64 id_u64[NM_UTILS_IPV6_IFACE_ID_LEN / 8];
    };
} NMUtilsIPv6IfaceId;

and I would not return the identifier as GByteArray* but as: 

guint64
nm_utils_ipv6_iface_id_from_eui64(const NMPlatformHwAddress *);

or

NMUtilsIPv6IfaceId 
nm_utils_ipv6_iface_id_from_eui64(const NMPlatformHwAddress*);

Were you suggesting separate functions so we can do testcases for them?  Otherwise I can't think of a great reason to split them out...

(In reply to comment #12)
> Were you suggesting separate functions so we can do testcases for them? 
> Otherwise I can't think of a great reason to split them out...

Because these functions have a general use that should be extracted and grouped together.


E.g. (referring to the new functions:) why is ipv4_is_linklocal_169() in nm-device-gre.c? It is a duplication of ip4_is_link_local()/nm-platform-linux.c.

I am not claiming that nm-utils.h is the perfect place, because it is just an heap of utility functions. But for the moment I think it is good enough of a place. I would name these: nm_utils_ipv4_is_linklocal_169() -- by giving them a common prefix (nm_utils_ipv4_is_*) and putting them close to each other they are actually grouped together (inside the utility heap).


Common functionality should be grouped together, and it should be clearly separated from unrelated functionality. This way, nm-device-gre.c (to stick with the example) gets smaller and simpler to understand. And it makes use of simpler building blocks that are themselves simpler to understand an defined in a specific place and provide a specific function that can be reused. If it's inside nm-device-gre.c it cannot be reused (see the code duplication it already creates now).

I would even split the commit to first add the new (independent) functionality, and make use of it in a second place. This way we can cherry-pick the new functionality individually.

The same applies to nm_utils_ipv6_iface_id_from_*() functions, which initialize an IPv6 interface identifier from *, according to RFC-xzy. And having a type NMUtilsIPv6IfaceIdent makes it explicit what this "thing" actually is.

Currently this functionality is spread over get_ip_iface_identifier() and nm_rdisc_utils_ether_to_iid(). Note that IPv6LL also uses the latter algorithm (on ethernet devices). So, why is this inside nm-rdisc.[ch]? Is nm-rdisc.[ch] supposed to contain utility functions around IPv6 in general? Then the name seems wrong because IPv6 means more then mere RD.


It is such an easy win to put simple algorithms and concepts (e.g. the "IPv6 interface identifier") in their own, well-named functions/structures and use them where they are needed. I believe that the naming of things and how they are grouped and structured together is just so important to make a complex program understandable. Creating simple building blocks that can be combined.

Did I ever rant about the 8000+ LOC in nm-device.c? Source code doesn't (necessarily) become hard to understand because of the mere lines of code, but when it just mashes everything together without enough structure.
I believe, that *you* can understand it. And it might be understood with some effort, but it is hard. Harder then it has to be.



An IID is not the same as a HwAddress, although both can be hold in a GByteArray. I suggest creating distinct types for them and use them thoroughly.

Also note that nm_rdisc_utils_ether_to_iid() gets a "guint8 iid[NM_RDISC_IID_LEN]" argument, but the compiler doesn't help you anymore on what you actually pass in there. So, single every time I see it I think how ugly this is. Why not having a properly sized type (NMUtilsIPv6IfaceIdent, or guint64, if you insist)?

(In reply to comment #11)
> >> core: track a Device's IP interface hardware address
> 
> +                   g_assert (addrlen <= sizeof (priv->ip_hw_addr));
> 
> If you decide to assert against this condition, it should be done earlier as
> well: in link_get_address().
> 
> I pushed a commit for that, it also handles the unexpected case gracefully.
>   "platform: assert against the maximum length of link_get_address()"

Looks fine.

> >> core: use Interface Identifiers for IPv6 SLAAC addresses
> 
> +    /* Use the port GUID per http://tools.ietf.org/html/rfc4391#section-8,
> +     * making sure to set the 'u' bit to 1.  The GUID is the lower 64 bits
> +     * of the IPoIB interface's hardware address.
> +     */
> 
> Isn't the right RFC this one: http://tools.ietf.org/html/rfc4291#page-20.
> Also, you only "invert/flip" the 'u' bit

RFC4391 is the InfiniBand specifics of IPv6, which is why it's referenced here. 

> I think, all these methods to create interface identifiers should be separate
> functions. E.g. nm_utils_ipv6_iface_id_from_eui64()

The method of generating an EUI-64 from an interface is pretty type-specific though, which is why I've put them into their respective files.  The way to generate an EUI-64 from a GRE interface is highly dependent on the way that its hardware address is specified.  We could split them out, but then we'd just have to pass in the NMLinkType and have a switch().  I'm not sure that's really worth it?

> +#define NM_RDISC_IID_LEN 8
> 
> I would call this, NM_UTILS_IPV6_IFACE_ID_LEN (because it is also relevant for
> LL and outside of RDISC). Also, I would create a set of related
> nm_utils_ipv6_iface_id_*() functions.
>
> 
> I would even create a type
> 
> typedef struct {
>     union {
>         guint64 id;
>         guint8 id_u8[NM_UTILS_IPV6_IFACE_ID_LEN];
>         guint16 id_u16[NM_UTILS_IPV6_IFACE_ID_LEN / 2];
>         guint32 id_u32[NM_UTILS_IPV6_IFACE_ID_LEN / 4];
>         guint64 id_u64[NM_UTILS_IPV6_IFACE_ID_LEN / 8];
>     };
> } NMUtilsIPv6IfaceId;

So I moved all this into the platform, and replaced the byte array usage with the struct you wrote here.  Unfortunately, we can't get rid of the "get_ip_iface_identifier" NMDevice class method because the only place we get the PPP interface IID is from pppd itself, we can't read it from the kernel at all like we can with Ethernet/InfiniBand/GRE.  See if you like the current state of the branch.

(In reply to comment #13)
> (In reply to comment #12)
> > Were you suggesting separate functions so we can do testcases for them? 
> > Otherwise I can't think of a great reason to split them out...
> 
> Because these functions have a general use that should be extracted and grouped
> together.
> 
> 
> E.g. (referring to the new functions:) why is ipv4_is_linklocal_169() in
> nm-device-gre.c? It is a duplication of
> ip4_is_link_local()/nm-platform-linux.c.
> 
> I am not claiming that nm-utils.h is the perfect place, because it is just an
> heap of utility functions. But for the moment I think it is good enough of a
> place. I would name these: nm_utils_ipv4_is_linklocal_169() -- by giving them a
> common prefix (nm_utils_ipv4_is_*) and putting them close to each other they
> are actually grouped together (inside the utility heap).

This is true; however I reworked the code not to use all these functions, but to use a table instead.  See get_gre_eui64_u_bit() in src/platform/nm-platform.c.  We could generalize that function in the platform, but I think until we actually have another consumer for the global/local thing I'd like to leave it as-is.

> Common functionality should be grouped together, and it should be clearly
> separated from unrelated functionality. This way, nm-device-gre.c (to stick
> with the example) gets smaller and simpler to understand. And it makes use of
> simpler building blocks that are themselves simpler to understand an defined in
> a specific place and provide a specific function that can be reused. If it's
> inside nm-device-gre.c it cannot be reused (see the code duplication it already
> creates now).

I agree, though this code is GRE-specific at this time.  I'm a big proponent of only generalizing things when they are actually used in two places, because I have often found that if I generalize something because it might be useful somewhere else, half the time it never does get used anywhere else.  And I feel that contributes to complexity, because the functionality that is only used in one place is not near that place in the code.  The rule of thumb that I use for splitting stuff like this is (a) is it used from two separate places in the code and (b) can it/should it be unit-tested.  If the answer to either of those is "yes" then it should probably be split out.

Again, I agree with you, but personally I like to see that second use-case first, before splitting the code out.

> The same applies to nm_utils_ipv6_iface_id_from_*() functions, which initialize
> an IPv6 interface identifier from *, according to RFC-xzy. And having a type
> NMUtilsIPv6IfaceIdent makes it explicit what this "thing" actually is.

Agreed.  I created NMPlatformIfaceId per your suggestion and converted the API to use it.  We can certainly go overboard on creating custom types, but I think in this case it's useful.

> Currently this functionality is spread over get_ip_iface_identifier() and
> nm_rdisc_utils_ether_to_iid(). Note that IPv6LL also uses the latter algorithm
> (on ethernet devices). So, why is this inside nm-rdisc.[ch]? Is nm-rdisc.[ch]
> supposed to contain utility functions around IPv6 in general? Then the name
> seems wrong because IPv6 means more then mere RD.

You're correct, I moved it to the platform and kept it more generic.  This does mean that we have to essentially duplicate some logic about the interface type, which previously was encapsulated in NMDevice subclasses, but since the platform already has a lot of that logic, I think it's OK there.

> It is such an easy win to put simple algorithms and concepts (e.g. the "IPv6
> interface identifier") in their own, well-named functions/structures and use
> them where they are needed. I believe that the naming of things and how they
> are grouped and structured together is just so important to make a complex
> program understandable. Creating simple building blocks that can be combined.
> 
> Did I ever rant about the 8000+ LOC in nm-device.c? Source code doesn't

I haven't heard you rant about it, but I do in my head quite often :)  I also dislike the 8000+LOC in nm-device.c, and I think a lot of stuff should get split out of there, like splitting out some of the IP configuration stuff.

(In reply to comment #10)
> That was my first suggestions. I revised it because I think NMPlatformHwAddress
> has the following pro and cons.:
> 
> +:
>   A) it's more explicit(!)
>   B) if needed, we could easily extend it to add the HwAddress type field.
>   C) can be embedded in a private struct
>      (no need to dispose(), cannot be NULL << less typing)
> -:
>   a) not ref-countable (OTOH, also a 'const GByteArray *' would need to be 
>      copied most of the time)
>   b) gobject properties should probably still be exposed as GByteArray*

FYI, in 1.0's libnm, hardware address properties will be strings, not GByteArrays. (It turns out that most of NM would rather have them in string form than in binary form anyway.)

I've been pondering whether the remaining GByteArray-valued properties (NMSetting8021x's cert/key/password blobs, and NMSettingWireless:ssid) should become GBytes instead. (GBytes is like GByteArray, except that it's inherently "const", and thus can always be safely refcounted rather than copied.)

> core: track a Device's IP interface hardware address
>
> The IP interface may have it's own hardware address (like the net

"its"



> core: use Interface Identifiers for IPv6 SLAAC addresses

>+gboolean
>+nm_platform_get_interface_identifier (NMLinkType link_type,
>+                                      const guint8 *hwaddr,
>+                                      guint hwaddr_len,
>+                                      NMPlatformIfaceId *out_iid)

If this is going to be in nm-platform, I think I would expect:

  gboolean
  nm_platform_link_get_interface_identifier (int ifindex,
                                             NMPlatformIfaceId *out_iid);

>+        guint64 id;
>+        guint8 id_u8[8];
>+        guint16 id_u16[4];
>+        guint32 id_u32[2];
>+        guint64 id_u64[1];

might as well align id_u8 with the others

>+		/* Adding the Interface Identifier is left to consumers */

This seems like a bad failure of abstraction... why should nm-device have to know that particular detail of how SLAAC works?



> ppp: add IPv6 support

>+	if (!val || !G_VALUE_HOLDS (val, G_TYPE_UINT64)) {
>+		nm_log_dbg (LOGD_PPP, "pppd plugin property '%s' missing or not a uchar array", prop);

"not a uint64"

>+	/* Construct an IPv6 LL address from the interface identifier */

specify where this logic comes from?

>+	memcpy(&iid, &eui, sizeof (eui));

missing space before "(", and maybe

  G_STATIC_ASSERT (sizeof (iid) == sizeof (eui))



> wwan: read supported IP types from ModemManager

>+	NM_MODEM_IP_TYPE_IPV4 = 0x1,
>+	NM_MODEM_IP_TYPE_IPV6 = 0x2,
>+	NM_MODEM_IP_TYPE_IPV4V6 = 0x4

So... if supported_types is 0x3, that means that it can do IPv4, and it can do IPv6, but it can't do them both at the same time? If so, maybe "TYPE_IPV4_ONLY", "TYPE_IPV6_ONLY" and "TYPE_DUAL_STACK" would be clearer. (And if not, then it's not clear why you need TYPE_IPV4V6, since it would mean the same thing as TYPE_IPV4 | TYPE_IPV6.)



> wwan: add IPv6 support for ModemManager1 (bgo #682623)
> wwan: split IPv4 and IPv6 bearer IP configuration methods

This seems like it's essentially "add half-broken IPv6 support", followed by "fix the previous commit". Shouldn't the "split" commit go first?



> wwan: consoldiate IP method handling into the modem object
> 
> Instead of checking it twice (once in NMDeviceModem and a second
> time in NMDeviceBluetooth) do it once in NMModem.

"consolidate". Also, there's nothing about NMDeviceBluetooth in this commit...



> platform: assert against the maximum length of link_get_address()

The API doesn't use fixed-length buffers, so there's no need for it to enforce a max length; the caller should just do the right thing with the length it returns. NMDevice requires that the length be <= NM_UTILS_HWADDR_MAX_LEN, but that's NMDevice's problem, not NMPlatform's.

>> core: use Interface Identifiers for IPv6 SLAAC addresses

I like this commit now much better.


Sidenote: regarding danw's comment "bad failure of abstraction".

If we would have a type
  typedef struct {
      guint8 addr[NM_UTILS_HWADDR_MAX_LEN];
      guint len;
      NMPlatformType type;
  } NMPlatformHwAddress;
that is a private member of NMDevice, it could be neatly passed on to NMRDisc via nm_rdisc_set_lladdr().

I still fancy a separate type here :)



+              /* Add the Interface Identifier to the lower 64 bits */
+              if (!NM_PLATFORM_IFACE_ID_IS_NULL (iid))
+                   memcpy (address.address.s6_addr + 8, iid.id_u8, 8);

I don't think that it makes any sense to add addresses with the host-part zero.
This "if" has no actual effect, because address.address has already 8 zero bytes at the end.

Or what does that mean? E.g. for IPv4 the "network-address" is not usable. Is it useful for IPv6?



+    if (!success) {
+         nm_log_warn (LOGD_HW, "(%s): failed to generate interface identifier "
+                      "for link type %u hwaddr_len %u",
+                      nm_device_get_ip_iface (self), priv->link_type, hwaddr_len);
+    }

this function gets potentially called whenever we get some RA. Isn't this going to spam our logfile? As NMDevice caches both priv->hw_addr and priv->ip_hw_addr, maybe we should cache the priv->ip_iface_identifier and having a priv->ip_iface_identifier_set flag.
Whenever we change priv->hw_addr, priv->ip_iface, et.al, we clear priv->ip_iface_identifier_set. get_ip_iface_identifier() only calls nm_platform_get_interface_identifier() if priv->ip_iface_identifier is not already set. The purpose is to suppress repeated warnings about the same.



>> ppp: add IPv6 support

+    /* already monitoring */
+    if (priv->monitor_fd >= 0)
+         return;
+
     priv->monitor_fd = socket (AF_INET, SOCK_DGRAM, 0);
-    if (priv->monitor_fd > 0) {
+    if (priv->monitor_fd >= 0) {


priv->monitor_fd will be initialized to 0, and _ppp_cleanup() sets it to 0. I think with this change, the "invalid" value should be -1.

    if (priv->monitor_fd) {
        /* Get the stats one last time */
        monitor_cb (manager);
        close (priv->monitor_fd);
        priv->monitor_fd = 0;
    }

should become

    if (priv->monitor_fd >= 0) {
        /* Get the stats one last time */
        monitor_cb (manager);
        close (priv->monitor_fd);
        priv->monitor_fd = -1;
    }



I would HAVE_PPPD_IPV6_NOTIFIER not make a compile time depencendy. All you need is *one* function like:

static struct notifier *
_pppd_ipv6_up_notifier() 
{
    static struct notifier *notifier;
    static char loaded;

    if (!G_UNLIKELY (loaded)) {
        /* load module, and access extern variable */
        loaded = TRUE;
    }
    return notifier;
}


I would think, that it is about the same lines of code (saving Makefile.am changes) and it just works out of the box (depending on what your libppp supports at *runtime*).

(Note that
»···ipv6cp_options *ho = &ipv6cp_hisoptions[0];
»···ipv6cp_options *go = &ipv6cp_gotoptions[0];
is present already since <=2000. AFAIS)

Yeah, I know you guys like the compile time way... me, not so much :)

(In reply to comment #17)
> 
> > platform: assert against the maximum length of link_get_address()
> 
> The API doesn't use fixed-length buffers, so there's no need for it to enforce
> a max length; the caller should just do the right thing with the length it
> returns. NMDevice requires that the length be <= NM_UTILS_HWADDR_MAX_LEN, but
> that's NMDevice's problem, not NMPlatform's.

Hm. Ok. But I would expect that whenever platform comes up with a HW address that is longer then MAX_LEN, other places start to assert or crash. So, this would assert (once and for all) at the earliest possible point and even save the day by returning 0. But feel free to drop it. 



(In reply to comment #16)
> (In reply to comment #10)
> > That was my first suggestions. I revised it because I think NMPlatformHwAddress
> > has the following pro and cons.:
> > 
> > +:
> >   A) it's more explicit(!)
> >   B) if needed, we could easily extend it to add the HwAddress type field.
> >   C) can be embedded in a private struct
> >      (no need to dispose(), cannot be NULL << less typing)
> > -:
> >   a) not ref-countable (OTOH, also a 'const GByteArray *' would need to be 
> >      copied most of the time)
> >   b) gobject properties should probably still be exposed as GByteArray*
> 
> FYI, in 1.0's libnm, hardware address properties will be strings, not
> GByteArrays. (It turns out that most of NM would rather have them in string
> form than in binary form anyway.)
> 
> I've been pondering whether the remaining GByteArray-valued properties
> (NMSetting8021x's cert/key/password blobs, and NMSettingWireless:ssid) should
> become GBytes instead. (GBytes is like GByteArray, except that it's inherently
> "const", and thus can always be safely refcounted rather than copied.)

Interesting.  In this regard GBytes sounds better suited. I really like immutable types.

(In reply to comment #17)
> > core: track a Device's IP interface hardware address
> >
> > The IP interface may have it's own hardware address (like the net
> 
> "its"

Fixed.

> > core: use Interface Identifiers for IPv6 SLAAC addresses
> 
> >+gboolean
> >+nm_platform_get_interface_identifier (NMLinkType link_type,
> >+                                      const guint8 *hwaddr,
> >+                                      guint hwaddr_len,
> >+                                      NMPlatformIfaceId *out_iid)
> 
> If this is going to be in nm-platform, I think I would expect:
> 
>   gboolean
>   nm_platform_link_get_interface_identifier (int ifindex,
>                                              NMPlatformIfaceId *out_iid);

Two reasons:

1) it's a utility function, and doesn't need to know the interface itself.  Thomas had suggested putting it in NetworkManagerUtils.c, but I felt that since it contains specific knowledge of interface types, that it was a better fit in the platform.  I feel that it's similar to nm_platform_addr_flags2str() or the sysctl functions here in that it doesn't need a specific interface.

2) the hardware address comes from the IP interface

> >+        guint64 id;
> >+        guint8 id_u8[8];
> >+        guint16 id_u16[4];
> >+        guint32 id_u32[2];
> >+        guint64 id_u64[1];
> 
> might as well align id_u8 with the others

Done.

> >+		/* Adding the Interface Identifier is left to consumers */
> 
> This seems like a bad failure of abstraction... why should nm-device have to
> know that particular detail of how SLAAC works?

There's a conflict here: on the one hand devices shouldn't really care how SLAAC works, but on the other hand, should SLAAC really need deep knowledge of how interface identifiers are constructed from device-specific details?

Since we have so much device-specific stuff in the devices, I leaned towards putting most of the logic there to keep the rdisc code smaller.  The second argument for keeping it in nm-device is that the IID would also get used for IPv6LL addresses too, not just SLAAC.  We might soon start doing the LL stuff in NM too.

> > ppp: add IPv6 support
> 
> >+	if (!val || !G_VALUE_HOLDS (val, G_TYPE_UINT64)) {
> >+		nm_log_dbg (LOGD_PPP, "pppd plugin property '%s' missing or not a uchar array", prop);
> 
> "not a uint64"

Fixed.

> >+	/* Construct an IPv6 LL address from the interface identifier */
> 
> specify where this logic comes from?

Done (RFC 4291 and RFC 5072)

> >+	memcpy(&iid, &eui, sizeof (eui));
> 
> missing space before "(", and maybe
> 
>   G_STATIC_ASSERT (sizeof (iid) == sizeof (eui))

Both done.

(In reply to comment #18)
> I would HAVE_PPPD_IPV6_NOTIFIER not make a compile time depencendy. All you
> need is *one* function like:
> 
> static struct notifier *
> _pppd_ipv6_up_notifier() 
> {
>     static struct notifier *notifier;
>     static char loaded;
> 
>     if (!G_UNLIKELY (loaded)) {
>         /* load module, and access extern variable */
>         loaded = TRUE;
>     }
>     return notifier;
> }
> 
> 
> I would think, that it is about the same lines of code (saving Makefile.am
> changes) and it just works out of the box (depending on what your libppp
> supports at *runtime*).

HAVE_PPPD_IPV6_NOTIFIER is there to protect access to "ipv6_up_notifier", which is only defined in some pppd versions.  I don't see how we can conditionally use that symbol?  The plugin is already being dlopened by pppd itself, so what module would the plugin be loading to find the extern?

(In reply to comment #21)
> (In reply to comment #18)
> > I would HAVE_PPPD_IPV6_NOTIFIER not make a compile time depencendy. All you
> > need is *one* function like:
> > 
> > static struct notifier *
> > _pppd_ipv6_up_notifier() 
> > {
> >     static struct notifier *notifier;
> >     static char loaded;
> > 
> >     if (!G_UNLIKELY (loaded)) {
> >         /* load module, and access extern variable */
> >         loaded = TRUE;
> >     }
> >     return notifier;
> > }
> > 
> > 
> > I would think, that it is about the same lines of code (saving Makefile.am
> > changes) and it just works out of the box (depending on what your libppp
> > supports at *runtime*).
> 
> HAVE_PPPD_IPV6_NOTIFIER is there to protect access to "ipv6_up_notifier", which
> is only defined in some pppd versions.  I don't see how we can conditionally
> use that symbol?  The plugin is already being dlopened by pppd itself, so what
> module would the plugin be loading to find the extern?


Disclaimer: I did not test this, but I think it should work just fine:

-#if HAVE_PPPD_IPV6_NOTIFIER
-    add_notifier (&ipv6_up_notifier, nm_ip6_up, NULL);
-#endif
+    {
+         static struct notifier **notifier = NULL;
+         static gsize load_once = 0;
+
+         if (g_once_init_enter (&load_once)) {
+              void *handle = dlopen(NULL, RTLD_NOW | RTLD_GLOBAL);
+
+              if (handle) {
+                   notifier = dlsym (handle, "ipv6_up_notifier");
+                   dlclose (handle);
+              }
+              g_once_init_leave (&load_once, 1);
+         }
+         if (notifier)
+             add_notifier (notifier, nm_ip6_up, NULL);
+    }

results in:
  configure.ac                     | 15 ---------------
  src/ppp-manager/nm-pppd-plugin.c | 25 +++++++++++++++++--------
  2 files changed, 17 insertions(+), 23 deletions(-)

(In reply to comment #20)
> > >+nm_platform_get_interface_identifier (NMLinkType link_type,

I forget exactly what was eventually agreed upon on IRC, but I'm OK with whatever it was.

> > >+		/* Adding the Interface Identifier is left to consumers */
> > 
> > This seems like a bad failure of abstraction... why should nm-device have to
> > know that particular detail of how SLAAC works?
> 
> There's a conflict here: on the one hand devices shouldn't really care how
> SLAAC works, but on the other hand, should SLAAC really need deep knowledge of
> how interface identifiers are constructed from device-specific details?

No, but NMDevice could generate the IID itself, and then pass it to NMRDisc, which could then use it in the appropriate place.

> The second
> argument for keeping it in nm-device is that the IID would also get used for
> IPv6LL addresses too, not just SLAAC.  We might soon start doing the LL stuff
> in NM too.

Though as discussed earlier, nm-device is way too big and maybe it would be better to put the IPv6LL stuff in rdisc/ with the SLAAC stuff.

But either way, I agree with *generating* the IID in NMDevice.

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #18)
> > > I would HAVE_PPPD_IPV6_NOTIFIER not make a compile time depencendy. All you
> > > need is *one* function like:
> > > 
> > > static struct notifier *
> > > _pppd_ipv6_up_notifier() 
> > > {
> > >     static struct notifier *notifier;
> > >     static char loaded;
> > > 
> > >     if (!G_UNLIKELY (loaded)) {
> > >         /* load module, and access extern variable */
> > >         loaded = TRUE;
> > >     }
> > >     return notifier;
> > > }
> > > 
> > > 
> > > I would think, that it is about the same lines of code (saving Makefile.am
> > > changes) and it just works out of the box (depending on what your libppp
> > > supports at *runtime*).
> > 
> > HAVE_PPPD_IPV6_NOTIFIER is there to protect access to "ipv6_up_notifier", which
> > is only defined in some pppd versions.  I don't see how we can conditionally
> > use that symbol?  The plugin is already being dlopened by pppd itself, so what
> > module would the plugin be loading to find the extern?
> 
> 
> Disclaimer: I did not test this, but I think it should work just fine:
> 
> -#if HAVE_PPPD_IPV6_NOTIFIER
> -    add_notifier (&ipv6_up_notifier, nm_ip6_up, NULL);
> -#endif
> +    {
> +         static struct notifier **notifier = NULL;
> +         static gsize load_once = 0;
> +
> +         if (g_once_init_enter (&load_once)) {
> +              void *handle = dlopen(NULL, RTLD_NOW | RTLD_GLOBAL);
> +
> +              if (handle) {
> +                   notifier = dlsym (handle, "ipv6_up_notifier");
> +                   dlclose (handle);
> +              }
> +              g_once_init_leave (&load_once, 1);
> +         }
> +         if (notifier)
> +             add_notifier (notifier, nm_ip6_up, NULL);
> +    }

This appears to work, so I'll incorporate this.

Ok, repushed with addressing:

1) all of danw's comments about the WWAN parts
2) thaller's automatic PPP plugin IPv6 detection
3) a slightly different approach for the IID stuff as discussed on IRC

Please review.

(In reply to comment #17)
> > wwan: read supported IP types from ModemManager
> 
> >+	NM_MODEM_IP_TYPE_IPV4 = 0x1,
> >+	NM_MODEM_IP_TYPE_IPV6 = 0x2,
> >+	NM_MODEM_IP_TYPE_IPV4V6 = 0x4
> 
> So... if supported_types is 0x3, that means that it can do IPv4, and it can do
> IPv6, but it can't do them both at the same time?

Correct, but not *on the same bearer* at the same time.

These actually come from WWAN PDP context types, which are the type of bearer you can actually create from an IP perspective.  A modem can support any combination of the three.

Modems can often do dual-stack without V4V6 support, but you need to create two bearers, one for V4 and one for V6.  That also implies two IP interfaces (eg, wwan0/v4 and ppp0/v6), but neither NM nor ModemManager support this quite yet.  But I hope to in the future since a lot of modems out there don't support V4V6 contexts yet.

> If so, maybe "TYPE_IPV4_ONLY", "TYPE_IPV6_ONLY" and "TYPE_DUAL_STACK" would be clearer. (And
> if not, then it's not clear why you need TYPE_IPV4V6, since it would mean the
> same thing as TYPE_IPV4 | TYPE_IPV6.)

Unfortunately V4 | V6 isn't quite the same as V4V6.  I'm not wild about V4_ONLY/V6_ONLY, because then a combination like TYPE_V4_ONLY | TYPE_V6_ONLY doesn't really make literal sense...

Does that clear things up?  Should I add comments about this sort of thing through the code, or is there something else I can do to make it less confusing?

I've added some code doc to NMModemIpType.  Is that better?

Created attachment 279462 [details]
Debug-level journal from MM+NM

I've done some functionality testing of this branch now, using an Ericsson F5321gw (a.k.a. "HP hs2350 HSPA+") in MBIM mode and a Nokia 21M-02 in both Icera and PPP modes. Unfortunately I didn't get to test the Ericsson in its non-MBIM mode because its ethernet port doesn't show up for some reason (perhaps a kernel bug).

The software I used was Fedora 20 + NM dcbw/www-ipv6 + MM git master + libmbim git master + ppp-2.4.6-3.fc21.x86_64. ISP is Telenor Norway, which supports PDP type IPv6 on the "telenor.mobil" APN and IPv4 PDP type on the "telenor" APN. They have no support for PDP type IPv4v6.

I found a few issues. I'll post one issue per comment (in order of severity) and attach MM/NM debug-level logs from connection attempts while reproducing each case.


Issue #1: Missing failback from ip-type ipv4v6 to ipv4 and/or ipv6.

When adding a new mobile broadband connection profile using nm-connection-editor's wizard, the connection profile will have method=auto both for [ipv4] and (implicitly) for [ipv6]. This in turn causes NM to try using ip-type ipv4v6, if the modem supports it. While I believe this is a good and future-proof default, the lack of failback to the single-stack ip-types ipv4/ipv6 is likely to cause problems for regular users.

My Ericsson, when in MBIM mode, advertises that it supports the ipv4v6 ip-type, as reported by mmcli:

  -------------------------
  Hardware |   manufacturer: 'Ericsson MBM'
           |          model: 'MBIM [03F0:3D1D]'
           |       revision: 'R3C11 (Pro), R3E14 (App)'
           |      supported: 'gsm-umts'
           |        current: 'gsm-umts'
           |   equipment id: '359166043474358'
  -------------------------
  [...]
  -------------------------
  IP       |      supported: 'ipv4, ipv6, ipv4v6'
  -------------------------

This is actually not the case. The hardware only supports the ipv4 and ipv6 types; *not* ipv4v6 (when in non-MBIM mode, that's what MM reports). So what happens when I try to connect to the default connection profile is that NetworkManager requests ip-type ipv4v6, and ModemManager immediately fails while reporting the error 'NoDeviceSupport'. The overall connection attempt ends with that failure,

While you could certainly argue that ModemManager or libmbim is at fault here for incorrectly claiming the device supports ipv4v6 when it really doesn't, you would end up in the exact same situation if the modem did support ipv4v6, but the network provider didn't. The ipv4v6 PDP type is relatively new, so it is still rare for providers to support it (the only one I'm aware to do so is Verizon Wireless in the USA).

So in summary, without a failback from ipv4v6 to ipv4/ipv6, users will experience failures to connect either if MM incorrectly reports modem support for ipv4v6, or if the modem correctly supports it but the network provider does not. In order to make it work, the user would need to manually disable either IPv4 and IPv6 in the connection profile to force the user of either ipv6 or ipv4 ip-type, respectively. (This is currently not possible using GUI tools.)

All users with a F5321gw (or possibly: any MBIM device), or a modem that does support ipv4v6 but whose provider does not, would experience this as a regression, as connection profiles that worked previously would stop working after upgrading NetworkManager. Therefore I feel that this really needs to be handled better before new NM packages hits the distros.

One suggestion on how such a fallback could be implemented would be to make MM understand something like «simple-connect='apn=telenor,ip-type=ipv4v6,ipv4,ipv6'» where it would try each requested ip-type in order, returning the first one that succeeded. Or NM could do the same, of course.

I believe that if ipv4v6 fails initially, it would currently be sensible to try ipv4 second and ipv6 third. However, when/if NM gains support for 464XLAT, it would make sense to try ipv6 second and ipv4 third, as it appears that most if not all of the 3GPP network operators preferred IPv6 deployment method is the use of ipv6 PDP type in conjunction with 464XLAT. (As I understand it, in 3GPP networks ipv4v6 has some issues involving roaming that possibly are serious enough to make ipv4v6 never see real deployment. CDMA2000 networks like Verizon Wireless aren't impacted by this, however.)

Tore

Created attachment 279466 [details]
Debug-level journal from MM+NM - issue #2

Issue #2: Nokia 21M-02/Icera failure towards IPv4-only APN

As described in my previous comment, by default, a connection profile will cause NM to use ip-type ipv4v6 if supported by the modem. The Nokia does support ipv4v6, but when attempting to connect to my provider's default "telenor" APN (which only supports ip-type ipv4), the connection attempt fails with the following error message:

  Couldn't connect bearer: 'Couldn't parse IPv6 address '::''

What's happening here is that the Nokia (and possibly other Icera-based modems) implements a in-firmware fallback from ipv4v6 to parallel ipv4 + ipv6 PDP contexts. Since the network/APN doesn't support ipv4v6, that fallback kicks in, but since the network/APN doesn't support ipv6 either, only the ipv4 one gets succeeds. Therefore the IPv6 fields in AT%IPDPADDR are all undefined/'::', but it would appear that MM fails to cope with this situation.

This would be a regression for 21M-02/Icera users, as a previously working connection profile would start failing after upgrading NM.

Note that the requested ip-type is merely a suggestion to the network - the network isn't required to comply. So this could possibly happen even if there is no in-modem failback also - the modem could request the ipv4v6 PDP type from the network, which could then respond with a ipv4 PDP type (instead of refusing the ipv4v6 request, as Telenor does).

Tore

Created attachment 279467 [details]
Debug-level journal from MM+NM - issue #3

Issue #3: Interface ID of global IPv6 address assigned to WWAN ethernet interface is the special all-zeroes Subnet-Router anycast address

When setting up IPv6 connectivity on the modems' emulated ethernet interfaces, the global address has all 64 bits of the interface ID is all zeroes. The attached log shows the address 2a02:2121:1:be5e:: being added when connecting to Telenor's IPv6-only APN using the Nokia. The same thing happens with the Ericsson as well.

When the Nokia is in non-Icera (PPP) mode, this does not happen - the interface ID part of the address gets set to something (I don't know where that "something" comes from though as it's different from the IPV6CP-assigned interface ID).

While it doesn't really matter what interface ID is used as the entire /64 gets routed to the host anyway, the all-zeroes one is reserved for "Subnet-Router anycast" (RFC 4291 section 2.6.1). For me, this issue has no impact - everything works fine, but if in the future the NM implementation is extended to include a sharing/hotspot feature (cf. RFC 7278) the misuse of the Subnet-Router anycast address as a regular unicast address could potentially cause connectivity issues between the NM host and other hosts connected to the hotspot.

If it's the intention to use a static value, I'd suggest you use ::1 or some other non-reserved value, otherwise you could use the network-assigned Interface ID (also used by the link-local address), or simply run the EUI-64 algorithm on the emulated ethernet interface's MAC address.

Tore

Created attachment 279468 [details]
Debug-level journal from MM+NM - issue #4

Issue #4: F5321gw/MBIM adds '::' as a secondary DNS server to /etc/resolv.conf

My ISP only providers a single IPv6 DNS server. When connecting, however, an array of two DNS servers gets returned by MM, where the second is '::'. I would assume the intention here that it should mean 'undefined' or something like that, but it does end up as 'nameserver ::' in /etc/resolv.conf.

For me this issue is only cosmetic, however I can imagine that this could be problematic if e.g. options:rotate is also declared in resolv.conf or some other program like dnsmasq is handling DNS requests.

Tore

(In reply to comment #26)
> Correct, but not *on the same bearer* at the same time.

> Modems can often do dual-stack without V4V6 support, but you need to create two
> bearers, one for V4 and one for V6.  That also implies two IP interfaces (eg,
> wwan0/v4 and ppp0/v6), but neither NM nor ModemManager support this quite yet. 

OK, that clears things up. I'd suggest adding comments to the enum definition clarifying that this is what the modem supports per-bearer, and that in theory it would be possible to do IPv4+IPv6 on a TYPE_IPV4|TYPE_IPV6 modem by using two bearers, but we don't currently support that.

(In reply to comment #31)
> Created an attachment (id=279468) [details]
> Debug-level journal from MM+NM - issue #4
> 
> Issue #4: F5321gw/MBIM adds '::' as a secondary DNS server to /etc/resolv.conf
> 
> My ISP only providers a single IPv6 DNS server. [...]

Actually, this doesn't seem to be the case. When the F5321gw is in non-MBIM mode (modprobe cdc_ncm prefer_mbim=0), I can see that the network provisions two DNS servers:

juli 01 16:06:27 envy.fud.no ModemManager[17534]: <debug> (ttyACM2): --> 'AT*E2IPCFG?<CR>'
juli 01 16:06:27 envy.fud.no ModemManager[17534]: <debug> (ttyACM2): <-- '<CR><LF>*E2IPCFG: (1,"fe80:0000:0000:0000:0000:000b:71ec:f201")(3,"2001:4600:0004:0fff:0000:0000:0000:0054")(3,"2001:4600:0004:1fff:0000:0000:0000:0054")<CR><LF>'
juli 01 16:06:27 envy.fud.no ModemManager[17534]: <debug> (ttyACM2): <-- '<CR><LF>OK<CR><LF>'

So for some reason, the second one gets replaced by :: when in MBIM mode - I guess either by MM or libmbim?

Tore

> core: use IP interface hardware address for IP-level operations

Although the changes themselves are fine, they leave the hwaddr-handling code in weird shape... Particularly, get_hw_address_length() is now constant for any given device type, but we still call it on every nm_device_update_hw_address() call for its side effect of telling us whether or not the device's hw address can actually change.

Maybe the "hwaddr can't be updated" info should be part of NMDeviceCapabilities?



> core: use Interface Identifiers for IPv6 SLAAC addresses

>+		g_return_val_if_fail (hwaddr_len == INFINIBAND_ALEN, FALSE);
>+		memcpy (out_iid->id_u8, hwaddr + 12, 8);

"hwaddr + INFINIBAND_ALEN - 8" ?

>+	case NM_LINK_TYPE_GRE:
>+	case NM_LINK_TYPE_GRETAP:

After spending all that time deciding where to put the IP mask matching code, it occurs to me that gre is IPv4-only anyway, isn't it?

>+		NMUtilsIPv6IfaceId iid = NM_UTILS_IPV6_IFACE_ID_INIT;
>+
>+		nm_device_get_ip_iface_identifier (device, &iid);

This is redundant, since nm_device_get_ip_iface_identifier() guarantees that it sets iid to 0 on failure anyway...

>+		/* Add the Interface Identifier to the lower 64 bits */
>+		memcpy (address.address.s6_addr + 8, iid.id_u8, 8);

I still think that should be handled in NMRDisc, not NMDevice. Just replace nm_rdisc_set_lladdr() with nm_rdisc_set_iid().



> ppp: add IPv6 support

>+		void *handle = dlopen(NULL, RTLD_NOW | RTLD_GLOBAL);
>+
>+		if (handle) {
>+			notifier = dlsym (handle, "ipv6_up_notifier");

I really don't like this. You're trading away compile-time type safety so that we can get a feature that nobody expects software on Linux to have anyway.



> wwan: read supported IP types from ModemManager

>+ * NMModemIpType:

Capital "P", for consistency with other NM types. (Likewise with NMModemIpMethod later.)

>+ * Indicates what IP protocols the modem supports.  Any combination of flags
>+ * is possible.  For example, (NM_MODEM_IP_TYPE_IPV4 | NM_MODEM_IP_TYPE_IPV6)
>+ * indicates that the modem supports IPv4 and IPv6 but not simultaneously on
>+ * the same bearer.

Good.



> wwan: clean up and split IP method values

NM_MODEM_IP_METHOD_DHCP should probably be METHOD_AUTO, since for IPv6 it means "SLAAC and maybe also DHCP", right?



> wwan: add infrastructure for IPv6 config results

>+	NMIP6Config *  wwan_ip6_config;

This seems weird... shouldn't we just have a "dev_ip6_config" to go with "dev_ip4_config"?

>+	/* Merge WWAN config *last* to ensure modem-given settings overwrite
>+	 * any external stuff set by pppd or other scripts.
>+	 */

Why do we need to do that for IPv6 but not IPv4?

>+	/* Re-enable IPv6 on the interface */
>+	nm_device_ipv6_sysctl_set (device, "disable_ipv6", "0");

A bit awkward that we do this from nm-device-modem, since it means that if we change the way IPv6-disabling works in nm-device, we're going to need to update nm-device-modem too. Maybe there should be a higher-level abstraction than sysctl_set? (Or maybe it's not really an issue, since we're doing both the disable=1 and the disable=0 here...)

>+	g_set_error_literal (error,
>+			             NM_MODEM_ERROR,
>+			             NM_MODEM_ERROR_CONNECTION_INCOMPATIBLE,
>+	                     "Connection specified no IP configuration!");

indent



> platform: assert against the maximum length of link_get_address()

If we're keeping this, I'd be happier if there was a define in NMPlatform representing "the largest ALEN NMPlatform deals with", rather than reusing the one from nm-utils representing "the largest ALEN nm_utils_hwaddr_aton() deals with".

(In reply to comment #34)
> >+	case NM_LINK_TYPE_GRE:
> >+	case NM_LINK_TYPE_GRETAP:
> 
> After spending all that time deciding where to put the IP mask matching code,
> it occurs to me that gre is IPv4-only anyway, isn't it?

The kernel doesn't seem to think so, given net/ipv6/ip6_gre.c and ARPHRD_IP6GRE.  Not sure what we have to do in NM to account for that?

> >+		/* Add the Interface Identifier to the lower 64 bits */
> >+		memcpy (address.address.s6_addr + 8, iid.id_u8, 8);
> 
> I still think that should be handled in NMRDisc, not NMDevice. Just replace
> nm_rdisc_set_lladdr() with nm_rdisc_set_iid().

I disagree for two reasons:

1) The IID/EUI64 is *consumed* by IPv6 Neighbor Discovery, and will also be consumed by LL addressing when NM takes that over from the kernel to fix the IFF_UP vs. carrier-detect vs. IPv6LL bug.

2) Some addressing methods (eg, PPP) give us the IID/EUI64 as part of the protocol, which we need to use to set the LL addres (eg, #1) for those links.

So I'd prefer to keep it in the NMDevice code, or the platform, because it's not specific to IPv6 Neighbor Discovery.

> > ppp: add IPv6 support
> 
> >+		void *handle = dlopen(NULL, RTLD_NOW | RTLD_GLOBAL);
> >+
> >+		if (handle) {
> >+			notifier = dlsym (handle, "ipv6_up_notifier");
> 
> I really don't like this. You're trading away compile-time type safety so that
> we can get a feature that nobody expects software on Linux to have anyway.

I don't care which way we go with this... but I think for the most part, we expect that the compile environment of NetworkManager (even for a cross-build) will be reasonably close to the runtime environment.  Which implies that the compile-time check is not a problem.  However, I don't mind the dlopen stuff either.

So, fight! :)

(In reply to comment #35)
> > >+		/* Add the Interface Identifier to the lower 64 bits */
> > >+		memcpy (address.address.s6_addr + 8, iid.id_u8, 8);
> > 
> > I still think that should be handled in NMRDisc, not NMDevice. Just replace
> > nm_rdisc_set_lladdr() with nm_rdisc_set_iid().
> 
> I disagree for two reasons:

I'm not saying that rdisc should generate the IID; I agree that that belongs in NMDevice. But then NMDevice should pass the IID to rdisc, and rdisc should merge the prefix and IID together itself before returning the address to NMDevice. Because "Add the Interface Identifier to the lower 64 bits" is part of the definition of SLAAC, so it belongs in rdisc.

Comment on attachment 279468 [details]
Debug-level journal from MM+NM - issue #4

Confirming that http://cgit.freedesktop.org/ModemManager/ModemManager/commit/?id=915a5beac0e10b0246adc9eefdb2dc29eb2c071e fixes issue #4.

Tore

(In reply to comment #36)
> (In reply to comment #35)
> > > >+		/* Add the Interface Identifier to the lower 64 bits */
> > > >+		memcpy (address.address.s6_addr + 8, iid.id_u8, 8);
> > > 
> > > I still think that should be handled in NMRDisc, not NMDevice. Just replace
> > > nm_rdisc_set_lladdr() with nm_rdisc_set_iid().
> > 
> > I disagree for two reasons:
> 
> I'm not saying that rdisc should generate the IID; I agree that that belongs in
> NMDevice. But then NMDevice should pass the IID to rdisc, and rdisc should
> merge the prefix and IID together itself before returning the address to
> NMDevice. Because "Add the Interface Identifier to the lower 64 bits" is part
> of the definition of SLAAC, so it belongs in rdisc.

Fair enough.  Does "core: use Interface Identifiers for IPv6 SLAAC addresses" now look better to you?  I replaced nm_rdisc_set_lladdr() with nm_rdisc_set_iid() and NMLNDPRDisc now handles creation of the address from the prefix + IID.

(In reply to comment #33)
> (In reply to comment #31)
> > Created an attachment (id=279468) [details] [details]
> > Debug-level journal from MM+NM - issue #4
> > 
> > Issue #4: F5321gw/MBIM adds '::' as a secondary DNS server to /etc/resolv.conf
> > 
> > My ISP only providers a single IPv6 DNS server. [...]
> 
> Actually, this doesn't seem to be the case. When the F5321gw is in non-MBIM
> mode (modprobe cdc_ncm prefer_mbim=0), I can see that the network provisions
> two DNS servers:
> 
> juli 01 16:06:27 envy.fud.no ModemManager[17534]: <debug> (ttyACM2): -->
> 'AT*E2IPCFG?<CR>'
> juli 01 16:06:27 envy.fud.no ModemManager[17534]: <debug> (ttyACM2): <--
> '<CR><LF>*E2IPCFG:
> (1,"fe80:0000:0000:0000:0000:000b:71ec:f201")(3,"2001:4600:0004:0fff:0000:0000:0000:0054")(3,"2001:4600:0004:1fff:0000:0000:0000:0054")<CR><LF>'
> juli 01 16:06:27 envy.fud.no ModemManager[17534]: <debug> (ttyACM2): <--
> '<CR><LF>OK<CR><LF>'
> 
> So for some reason, the second one gets replaced by :: when in MBIM mode - I
> guess either by MM or libmbim?

I looked at the MM and libmbim code briefly and couldn't see anything wrong, so some further debugging there would be useful...  Maybe using mbimcli to manually retrieve the details and see what the MBIM IP Configuration Response contains?  Or adding "mbim_utils_set_traces_enabled (TRUE)" somewhere in src/mm-bearer-mbim.c?

(In reply to comment #34)
> > core: use IP interface hardware address for IP-level operations
> 
> Although the changes themselves are fine, they leave the hwaddr-handling code
> in weird shape... Particularly, get_hw_address_length() is now constant for any
> given device type, but we still call it on every nm_device_update_hw_address()
> call for its side effect of telling us whether or not the device's hw address
> can actually change.
> 
> Maybe the "hwaddr can't be updated" info should be part of
> NMDeviceCapabilities?

Good point, but we can clean this up even better.  Please see "core: simplify hardware address reading"; does that look OK?

> > core: use Interface Identifiers for IPv6 SLAAC addresses
> 
> >+		g_return_val_if_fail (hwaddr_len == INFINIBAND_ALEN, FALSE);
> >+		memcpy (out_iid->id_u8, hwaddr + 12, 8);
> 
> "hwaddr + INFINIBAND_ALEN - 8" ?

Fixed.

> >+	case NM_LINK_TYPE_GRE:
> >+	case NM_LINK_TYPE_GRETAP:
> 
> After spending all that time deciding where to put the IP mask matching code,
> it occurs to me that gre is IPv4-only anyway, isn't it?

See comment 35; I think there's IPv6 GRE support these days too.

> >+		NMUtilsIPv6IfaceId iid = NM_UTILS_IPV6_IFACE_ID_INIT;
> >+
> >+		nm_device_get_ip_iface_identifier (device, &iid);
> 
> This is redundant, since nm_device_get_ip_iface_identifier() guarantees that it
> sets iid to 0 on failure anyway...

Fixed.

> >+		/* Add the Interface Identifier to the lower 64 bits */
> >+		memcpy (address.address.s6_addr + 8, iid.id_u8, 8);
> 
> I still think that should be handled in NMRDisc, not NMDevice. Just replace
> nm_rdisc_set_lladdr() with nm_rdisc_set_iid().

Yeah, you're right, done per comment 38.

> > ppp: add IPv6 support
> 
> >+		void *handle = dlopen(NULL, RTLD_NOW | RTLD_GLOBAL);
> >+
> >+		if (handle) {
> >+			notifier = dlsym (handle, "ipv6_up_notifier");
> 
> I really don't like this. You're trading away compile-time type safety so that
> we can get a feature that nobody expects software on Linux to have anyway.

I'm OK with the current approach, I was also OK with the previous compile-time approach too.  So either way we go, I don't care, somebody argue for it.  If nobody does, I'll just go with the dlopen() approach.

> > wwan: read supported IP types from ModemManager
> 
> >+ * NMModemIpType:
> 
> Capital "P", for consistency with other NM types. (Likewise with
> NMModemIpMethod later.)

Done.

> > wwan: clean up and split IP method values
> 
> NM_MODEM_IP_METHOD_DHCP should probably be METHOD_AUTO, since for IPv6 it means
> "SLAAC and maybe also DHCP", right?

Done.

> >+	/* Re-enable IPv6 on the interface */
> >+	nm_device_ipv6_sysctl_set (device, "disable_ipv6", "0");
> 
> A bit awkward that we do this from nm-device-modem, since it means that if we
> change the way IPv6-disabling works in nm-device, we're going to need to update
> nm-device-modem too. Maybe there should be a higher-level abstraction than
> sysctl_set? (Or maybe it's not really an issue, since we're doing both the
> disable=1 and the disable=0 here...)

This will get fixed now with the LL address genmode stuff that Jiri Pirko did and which just got merged into the kernel, since we'll stop setting disable_ipv6 entirely, and just turn off kernel LL assignment instead.

Not sure how we'll do the backwards compat though...

> >+	g_set_error_literal (error,
> >+			             NM_MODEM_ERROR,
> >+			             NM_MODEM_ERROR_CONNECTION_INCOMPATIBLE,
> >+	                     "Connection specified no IP configuration!");
> 
> indent

Fixed.

====

Issues I still have to address:

> > wwan: add infrastructure for IPv6 config results
> 
> >+	NMIP6Config *  wwan_ip6_config;
> 
> This seems weird... shouldn't we just have a "dev_ip6_config" to go with
> "dev_ip4_config"?
> 
> >+	/* Merge WWAN config *last* to ensure modem-given settings overwrite
> >+	 * any external stuff set by pppd or other scripts.
> >+	 */
> 
> Why do we need to do that for IPv6 but not IPv4?
> 
> > platform: assert against the maximum length of link_get_address()
> 
> If we're keeping this, I'd be happier if there was a define in NMPlatform
> representing "the largest ALEN NMPlatform deals with", rather than reusing the
> one from nm-utils representing "the largest ALEN nm_utils_hwaddr_aton() deals
> with".

(In reply to comment #40)
> Good point, but we can clean this up even better.  Please see "core: simplify
> hardware address reading"; does that look OK?

Yes, mostly...

Nothing ever looks at the return value of nm_device_update_hw_address(), FWIW.

I'm not sure why you added NM_DEVICE_INT_HW_ADDRESS and made NM_DEVICE_HW_ADDRESS non-writable, since this makes things *less* convenient for the only place that uses it (nm_device_bt_new()). If you do keep it, then:

>+		hw_addr = g_value_get_boxed (value);
...
>+			memcpy (priv->hw_addr, hw_addr, hw_addr_len);

you need to say "g_bytes_get_data (hw_addr, NULL)" at the end there, not "hw_addr".

> See comment 35; I think there's IPv6 GRE support these days too.

OK, we need to update NMDeviceGre for that at some point...

> > > ppp: add IPv6 support

> I'm OK with the current approach, I was also OK with the previous compile-time
> approach too.  So either way we go, I don't care, somebody argue for it.  If
> nobody does, I'll just go with the dlopen() approach.

1. It's weird and not the way C is designed to work and is a bad precedent. (Yes, I know there's already another place where NM is doing this and I don't like it there either.)
2. It probably has no effect for people using distro packages, since the upgrade from non-IPv6-supporting pppd to IPv6-supporting pppd would probably happen as part of a distro version bump, meaning there'd be a new NetworkManager build along with it anyway.
3. It probably has no effect for people building from source either, since pppd 2.4.6 has already been released, and NM 1.0 hasn't, so anyone building NM 1.0 in the future (or building bleeding-edge NM from git now) is likely to already have pppd 2.4.6.

(In reply to comment #41)
> (In reply to comment #40)
> > Good point, but we can clean this up even better.  Please see "core: simplify
> > hardware address reading"; does that look OK?
> 
> Yes, mostly...
> 
> Nothing ever looks at the return value of nm_device_update_hw_address(), FWIW.
> 
> I'm not sure why you added NM_DEVICE_INT_HW_ADDRESS and made
> NM_DEVICE_HW_ADDRESS non-writable, since this makes things *less* convenient
> for the only place that uses it (nm_device_bt_new()). If you do keep it, then:

I did this because otherwise there's no way for NMDevice to read the hardware address size, because HW_ADDRESS is a string type.  So we either  need a new internal byte-array variable (I chose GBytes because it's const and becuase there's a gsystem local alloc item for it already) or we need a way for subclasses to specify the size of their hardware address when it cannot be read from the platform.

> >+		hw_addr = g_value_get_boxed (value);
> ...
> >+			memcpy (priv->hw_addr, hw_addr, hw_addr_len);
> 
> you need to say "g_bytes_get_data (hw_addr, NULL)" at the end there, not
> "hw_addr".

Oh right.

> > See comment 35; I think there's IPv6 GRE support these days too.
> 
> OK, we need to update NMDeviceGre for that at some point...

Yeah.

> > > > ppp: add IPv6 support
> 
> > I'm OK with the current approach, I was also OK with the previous compile-time
> > approach too.  So either way we go, I don't care, somebody argue for it.  If
> > nobody does, I'll just go with the dlopen() approach.
> 
> 1. It's weird and not the way C is designed to work and is a bad precedent.
> (Yes, I know there's already another place where NM is doing this and I don't
> like it there either.)
> 2. It probably has no effect for people using distro packages, since the
> upgrade from non-IPv6-supporting pppd to IPv6-supporting pppd would probably
> happen as part of a distro version bump, meaning there'd be a new
> NetworkManager build along with it anyway.
> 3. It probably has no effect for people building from source either, since pppd
> 2.4.6 has already been released, and NM 1.0 hasn't, so anyone building NM 1.0
> in the future (or building bleeding-edge NM from git now) is likely to already
> have pppd 2.4.6.

The only argument I really have for the dlopen() stuff is that it's so small and not-ugly that I'm slightly inclined to keep it...

But also, even if we do have compile-time checks, that doesn't guarantee that the system you install NM on will have the right pppd since there aren't any shared library dependencies between the plugin and pppd.  So maybe the dlopen() approach can fail gracefully instead of segfaulting.  Yeah, it's a stretch to call this an advantage since pppd is such a slow-moving target, and also because of your (2) and (3) above.

Thomas, any thoughts/reactions here?

(In reply to comment #42)
> I did this because otherwise there's no way for NMDevice to read the hardware
> address size, because HW_ADDRESS is a string type.

Count the ':'s and add 1?

(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > I'm OK with the current approach, I was also OK with the previous compile-time
> > > approach too.  So either way we go, I don't care, somebody argue for it.  If
> > > nobody does, I'll just go with the dlopen() approach.
> > 
> > 1. It's weird and not the way C is designed to work and is a bad precedent.
> > (Yes, I know there's already another place where NM is doing this and I don't
> > like it there either.)

dlopen() et al. is POSIX. What do you mean "the way C is designed to work"? Subjectively, I don't think that this is weired.

> > 2. It probably has no effect for people using distro packages, since the
> > upgrade from non-IPv6-supporting pppd to IPv6-supporting pppd would probably
> > happen as part of a distro version bump, meaning there'd be a new
> > NetworkManager build along with it anyway.

some distros have rolling (non-) releases. And it would be simple to take pppd SRPM from rawhide and recompile it yourself if you want to backport this feature. With compile time dependency, you also have to recompile NM.

> > 3. It probably has no effect for people building from source either, since pppd
> > 2.4.6 has already been released, and NM 1.0 hasn't, so anyone building NM 1.0
> > in the future (or building bleeding-edge NM from git now) is likely to already
> > have pppd 2.4.6.

2.) and 3.) says that this feature is not very important to most people. I agree.

But the dynamic solution has more advantages than disadvantages. It will just work, depending on the pppd version you are using. It's less lines of code and those lines span over less then a page (that you can look at and understand at once).

> The only argument I really have for the dlopen() stuff is that it's so small
> and not-ugly that I'm slightly inclined to keep it...
> 
> But also, even if we do have compile-time checks, that doesn't guarantee that
> the system you install NM on will have the right pppd since there aren't any
> shared library dependencies between the plugin and pppd.  So maybe the dlopen()
> approach can fail gracefully instead of segfaulting.  Yeah, it's a stretch to
> call this an advantage since pppd is such a slow-moving target, and also
> because of your (2) and (3) above.

I agree.

(In reply to comment #43)
> (In reply to comment #42)
> > I did this because otherwise there's no way for NMDevice to read the hardware
> > address size, because HW_ADDRESS is a string type.
> 
> Count the ':'s and add 1?

Done.

> > wwan: add infrastructure for IPv6 config results
> 
> >+	NMIP6Config *  wwan_ip6_config;
> 
> This seems weird... shouldn't we just have a "dev_ip6_config" to go with
> "dev_ip4_config"?

This is because with IPv4, there can only be on concurrent IP addressing mode, eg IPv4LL or DHCP, so the results of whichever one we have gets put into dev_ip4_config.  But with IPv6 we can have RA + DHCPv6 + WWAN concurrently, so they each need their own separate NMIP6Config private member.

> >+	/* Merge WWAN config *last* to ensure modem-given settings overwrite
> >+	 * any external stuff set by pppd or other scripts.
> >+	 */
> 
> Why do we need to do that for IPv6 but not IPv4?

Good question; probably because we've always done it this way.

But I'm happy to change that, which would involve a new nm_device_set_wwan_ip4_config() method and a new priv->wwan_ip4_config private member, so that we could apply the WWAN config after everything.  We can't use dev_ip4_config here because it usually contains the DHCP or IPv4LL config, and we don't want whatever is in that to overwrite externally-added stuff.

Adding nm_device_set_wwan_ip4_config() would bring WWAN IPv4 and IPv6 handling into sync, which is probably a good thing.  I have done this.

> > platform: assert against the maximum length of link_get_address()
> 
> If we're keeping this, I'd be happier if there was a define in NMPlatform
> representing "the largest ALEN NMPlatform deals with", rather than reusing the
> one from nm-utils representing "the largest ALEN nm_utils_hwaddr_aton() deals
> with".

I've done this with "platform: define the largest hardware address the platform supports" and updated nm-device.c and nm-linux-platform.c as appropriate.

Please re-review, thanks!

>> core: simplify hardware address reading

Already before your patch.
I think the else part "if (hwaddrlen)" in nm_device_update_hw_address() should check hw_addr_len changed.

Eg.

-         /* hw_addr_len is now 0; see if hw_addr was already empty */
-         for (i = 0; i < sizeof (priv->hw_addr) && !changed; i++) {
-              if (priv->hw_addr[i])
-                   changed = TRUE;
-         }
+         changed = priv->hw_addr_len != 0;


>> core: use Interface Identifiers for IPv6 SLAAC addresses

In general, I would not pass around pointers to NMUtilsIPv6IfaceId. The whole struct is only 8 bytes. Just copy it by value.

Eg.
-void nm_rdisc_set_iid (NMRDisc *rdisc, NMUtilsIPv6IfaceId *iid);
+void nm_rdisc_set_iid (NMRDisc *rdisc, NMUtilsIPv6IfaceId iid);

or alternatively:
+void nm_rdisc_set_iid (NMRDisc *rdisc, const NMUtilsIPv6IfaceId *iid);


Trailing space in receive_ra()


receive_ra() still creates /64 addresses with host-part zero, and the 'if' in
+                   if (NM_UTILS_IPV6_IFACE_ID_VALID (rdisc->iid))
+                        memcpy (address.address.s6_addr + 8, rdisc->iid.id_u8, 8);
has no effect (because not valid means == 0, ... meaning the address host-part stays at 0).


should be:

-              if (route.plen == 64) {
+              if (route.plen == 64 && NM_UTILS_IPV6_IFACE_ID_VALID (rdisc->iid)) {

(In reply to comment #46)
> >> core: simplify hardware address reading
> 
> Already before your patch.
> I think the else part "if (hwaddrlen)" in nm_device_update_hw_address() should
> check hw_addr_len changed.
> 
> Eg.
> 
> -         /* hw_addr_len is now 0; see if hw_addr was already empty */
> -         for (i = 0; i < sizeof (priv->hw_addr) && !changed; i++) {
> -              if (priv->hw_addr[i])
> -                   changed = TRUE;
> -         }
> +         changed = priv->hw_addr_len != 0;

Done.  See "fixup! core: simplify hardware address reading"

> >> core: use Interface Identifiers for IPv6 SLAAC addresses
> 
> In general, I would not pass around pointers to NMUtilsIPv6IfaceId. The whole
> struct is only 8 bytes. Just copy it by value.
> 
> Eg.
> -void nm_rdisc_set_iid (NMRDisc *rdisc, NMUtilsIPv6IfaceId *iid);
> +void nm_rdisc_set_iid (NMRDisc *rdisc, NMUtilsIPv6IfaceId iid);
> 
> or alternatively:
> +void nm_rdisc_set_iid (NMRDisc *rdisc, const NMUtilsIPv6IfaceId *iid);
> 
> 
> Trailing space in receive_ra()
> 
> 
> receive_ra() still creates /64 addresses with host-part zero, and the 'if' in
> +                   if (NM_UTILS_IPV6_IFACE_ID_VALID (rdisc->iid))
> +                        memcpy (address.address.s6_addr + 8, rdisc->iid.id_u8,
> 8);
> has no effect (because not valid means == 0, ... meaning the address host-part
> stays at 0).
> 
> 
> should be:
> 
> -              if (route.plen == 64) {
> +              if (route.plen == 64 && NM_UTILS_IPV6_IFACE_ID_VALID
> (rdisc->iid)) {

So I did all of these in:

fixup! use IIDs
fixup! ppp manager ipv6 support
fixup! wwan IPv6 infrastructure

but in addition to this, I went all the way and simplified the IID stuff so that functions return the IID instead of passing it by reference.  Unfortunately that's not easily possible with the union that I had before, so I removed the union and did 'typedef guint64 NMUtilsIPv6IfaceId' instead.  The only two places in the code that care how big it is are the Utils (where it's defined) and the rdisc code.  I could add some helpers or something if you like, I but I think it should be OK.

>> core: use Interface Identifiers for IPv6 SLAAC addresses

minor complain: could you split this commit to add first new IfaceId type and utils functions, then refactor nm-device, lastly rdisc.


>> ppp: add IPv6 support

maybe downgrade the g_warning() in add_ip6_notifier() to g_message(). IMO not warn-worthy, if somebody uses an older ppp implementation.


>> wwan: read supported IP types from ModemManager

trailing space in src/devices/wwan/nm-modem.h

I again reworked the IID stuff so that it's now a guint64 so we can just return the IID from functions.  That's now in dcbw/wwan-ipv6-2.  It also encapsulates all the byte-accesses (that would have used the union) into NetworkManagerUtils.c, so that other stuff doesn't have to care.

I did try to keep the union, but then we can't use that as a return type for stuff like g_return_val_if_fail() and creating a static const for that return value seems kinda ugly...

Created attachment 280887 [details]
Log when NM fails to configure 

(In reply to comment #47)

> fixup! use IIDs
> fixup! ppp manager ipv6 support
> fixup! wwan IPv6 infrastructure

I think there must be a bug in one of these commits, as when I now connect on an IPv6-only APN I get no global IPv6 address on the interface at all. What I end up with is only two link-local addresses:

tore@envy:~$ ip -6 a l dev wwp0s26u1u5i6
3: wwp0s26u1u5i6: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qlen 1000
    inet6 fe80::22:a725:c501/64 scope link 
       valid_lft forever preferred_lft forever
    inet6 fe80::fc32:e0ff:fefb:cefd/64 scope link 
       valid_lft forever preferred_lft forever

The first one is the network-assigned one that are added by NM as the device is being connected. I do see RAs with a global prefix being advertised:

22:06:02.336712 IP6 (hlim 255, next-header ICMPv6 (58) payload length: 8) envy.fud.no > ff02::2: [icmp6 sum ok] ICMP6, router solicitation, length 8
22:06:02.752768 IP6 (hlim 255, next-header ICMPv6 (58) payload length: 56) fe80::215:e0ff:feec:101 > ff02::1: [icmp6 sum ok] ICMP6, router advertisement, length 56
	hop limit 255, Flags [other stateful], pref medium, router lifetime 65535s, reachable time 0ms, retrans time 0ms
	  prefix info option (3), length 32 (4): 2a02:2121:1:28c::/64, Flags [auto], valid time infinity, pref. time infinity
	  source link-address option (1), length 8 (1): 00:15:e0:ec:01:01

This is with a fresh git pull from dcbw/wwan-ipv6 (HEAD at 1c85efa932). I'm attaching a debug-level log from device activation. Earlier, I'd get a working global address on the device (with the interface ID being all zeroes). I'll try to bisect it, but I can't guarantee I'll have time tonight.

Tore

(In reply to comment #50)
> Created an attachment (id=280887) [details]
> Log when NM fails to configure 
> 
> (In reply to comment #47)
> 
> > fixup! use IIDs
> > fixup! ppp manager ipv6 support
> > fixup! wwan IPv6 infrastructure
> 
> I think there must be a bug in one of these commits, as when I now connect on
> an IPv6-only APN I get no global IPv6 address on the interface at all. What I
> end up with is only two link-local addresses:
> 
> tore@envy:~$ ip -6 a l dev wwp0s26u1u5i6
> 3: wwp0s26u1u5i6: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qlen 1000
>     inet6 fe80::22:a725:c501/64 scope link 
>        valid_lft forever preferred_lft forever
>     inet6 fe80::fc32:e0ff:fefb:cefd/64 scope link 
>        valid_lft forever preferred_lft forever
> 
> The first one is the network-assigned one that are added by NM as the device is
> being connected. I do see RAs with a global prefix being advertised:
> 
> 22:06:02.336712 IP6 (hlim 255, next-header ICMPv6 (58) payload length: 8)
> envy.fud.no > ff02::2: [icmp6 sum ok] ICMP6, router solicitation, length 8
> 22:06:02.752768 IP6 (hlim 255, next-header ICMPv6 (58) payload length: 56)
> fe80::215:e0ff:feec:101 > ff02::1: [icmp6 sum ok] ICMP6, router advertisement,
> length 56
>     hop limit 255, Flags [other stateful], pref medium, router lifetime 65535s,
> reachable time 0ms, retrans time 0ms
>       prefix info option (3), length 32 (4): 2a02:2121:1:28c::/64, Flags
> [auto], valid time infinity, pref. time infinity
>       source link-address option (1), length 8 (1): 00:15:e0:ec:01:01
> 
> This is with a fresh git pull from dcbw/wwan-ipv6 (HEAD at 1c85efa932). I'm
> attaching a debug-level log from device activation. Earlier, I'd get a working
> global address on the device (with the interface ID being all zeroes). I'll try
> to bisect it, but I can't guarantee I'll have time tonight.
> 
> Tore

Try dcbw/wwan-ipv6-2, or cherry-pick the top 2 patches to dcbw/wwan-ipv6.  That should fix up the IID issues you're having.  It works for me with PPP, Icera, and QMI.

(In reply to comment #45)
> > If we're keeping this, I'd be happier if there was a define in NMPlatform
> > representing "the largest ALEN NMPlatform deals with", rather than reusing the
> > one from nm-utils representing "the largest ALEN nm_utils_hwaddr_aton() deals
> > with".
> 
> I've done this with "platform: define the largest hardware address the platform
> supports" and updated nm-device.c and nm-linux-platform.c as appropriate.

Hm... if we're going to assert that they're equal anyway, then it does seem kind of pointless to have two defines. So maybe forget about this and just use the UTILS define from nm-platform. (But at least update the nm-utils doc to say "the largest NetworkManager deals with" rather than "the largest nm_utils_hwaddr_aton deals with" or whatever.)

(In reply to comment #47)
> removed the union and did 'typedef guint64 NMUtilsIPv6IfaceId' instead.

This definitely makes things cleaner, but it makes me reflexively worry about byte order. I guess we never actually treat the iid as an integer, so it doesn't actually matter, but then that makes me feel like we shouldn't be storing it in an integer type... Maybe just add a comment that it's really a guint8[8], but we store it as a guint64 for convenience.

How about the fixup commits in dcbw/wwan-ipv6 now?  This commit does:

typedef guint8 NMUtilsIPv6IfaceId[8];

to make it clear that it's not really a guint64.  It also preserves the *out_iid stuff we had before.

Now you are using this guint8[8] construct.

It also cannot be returned from a function or conveniently checked "if(id)". So where is the advantage to the union from before?

with the union you could conveniently copy the entire id:

-    NM_MODEM_GET_PRIVATE (self)->iid.id = iid->id;
+    memcpy (NM_MODEM_GET_PRIVATE (self)->iid, iid, sizeof (*iid)));

you even could somewhat conveniently distingish "unset" from "set":
     if (NM_MODEM_GET_PRIVATE (self)->id) 

The union does prevent byte ordering ambiguities too. Byte ordering becomes only relevant, if you try to mess with parts of the guint64. When accessing parts of it, you must use the array alias id_u8, where you have full (obvious) control over the byte ordering. If you just care about the id as a whole, byte ordering doesn't matter, and you can access the "id" alias. And 0 is 0, regardless of byte ordering.

Just do the *out_iid stuff plus the union as before.

Created attachment 281171 [details] [review]
nm-modem.c syntax fix

(In reply to comment #53)
> How about the fixup commits in dcbw/wwan-ipv6 now?

dcbw/wwan-ipv6 currently doesn't build because a syntax failure. Patch attached. With that fixed, connecting to an IPv6-only APN works very well (tried Ericsson in MBIM mode, and the Nokia in both PPP and Icera modes), and I get a global IPv6 address with a network-assigned Interface ID:

$ ip -6 a l dev wwp0s26u1u5i6
3: wwp0s26u1u5i6: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qlen 1000
    inet6 2a02:2121:1:682d:0:18:d3dd:4801/64 scope global noprefixroute
       valid_lft forever preferred_lft forever
    inet6 fe80::18:d3dd:4801/64 scope link
       valid_lft forever preferred_lft forever
    inet6 fe80::29:9dff:fe38:4073/64 scope link
       valid_lft forever preferred_lft forever

Therefore I consider my original issue #3 fixed, and will obsolete that attachment shortly.

However there are some other problems with the current state of the dcbw/wwan-ipv6 code though:

1) When connecting to the wireless network, NM crashes:

[...]
<info> Activation (wlo1) successful, device activated.
bound to 192.168.0.18 -- renewal in 1679 seconds.
*** stack smashing detected ***: /usr/sbin/NetworkManager terminated
======= Backtrace: =========
/lib64/libc.so.6(+0x3433075cff)[0x7fc715e98cff]
/lib64/libc.so.6(__fortify_fail+0x37)[0x7fc715f29b17]
/lib64/libc.so.6(__fortify_fail+0x0)[0x7fc715f29ae0]
/usr/sbin/NetworkManager(+0x36ec3)[0x7fc71a4ffec3]
/usr/sbin/NetworkManager(+0x3a517)[0x7fc71a503517]
/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x143)[0x7fc71688d283]
/lib64/libglib-2.0.so.0(+0x3435049628)[0x7fc71688d628]
/lib64/libglib-2.0.so.0(g_main_loop_run+0x6a)[0x7fc71688da3a]
/usr/sbin/NetworkManager(main+0xc36)[0x7fc71a4f95f6]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fc715e44d65]
/usr/sbin/NetworkManager(+0x32799)[0x7fc71a4fb799]
[...]

2) Connecting to an IPv4-only APN, there's an assert, and the connection hangs forever (with all my modems in all modes):

[...]
<info> (cdc-wdm2): IPv4 static configuration:
<info>   address 46.66.30.183/28
<info>   gateway 46.66.30.178
<info>   DNS 193.213.112.4
<info>   DNS 130.67.15.198
nm_device_activate_schedule_ip4_config_result: assertion 'NM_IS_IP4_CONFIG (config)' failed
[...]

3) Connecting to an IPv4-only APN using the Nokia 21M-02 in PPP mode additionally yields errors about failing to read hw address size, and something about a "pass-filter" failing to get set. While IPv4 addressing and routing does get configured on ppp0, /etc/resolv.conf is not written, and the connection attempts hangs forever (as in #2):

[...]
pppd[4559]: Connect: ppp0 <--> /dev/ttyACM5
NetworkManager[2952]: Using interface ppp0
NetworkManager[2952]: Connect: ppp0 <--> /dev/ttyACM5
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (nm_phasechange): status 5 / phase 'establish'
NetworkManager[2952]: <error> [1405764081.618880] [devices/nm-device.c:7051] nm_device_update_hw_address(): (ppp0): failed to read hardware address size
NetworkManager[2952]: <info> (ppp0): new Generic device (driver: 'unknown' ifindex: 9)
NetworkManager[2952]: <info> (ppp0): exported as /org/freedesktop/NetworkManager/Devices/11
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (nm_phasechange): status 6 / phase 'authenticate'
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (get_credentials): passwd-hook, requesting credentials...
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (get_credentials): got credentials from NetworkManager
pppd[4559]: Remote message: Icera PPP - Password Verified OK
NetworkManager[2952]: Remote message: Icera PPP - Password Verified OK
NetworkManager[2952]: PAP authentication succeeded
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (nm_phasechange): status 8 / phase 'network'
NetworkManager[2952]: Couldn't set pass-filter in kernel: Invalid argument
pppd[4559]: PAP authentication succeeded
pppd[4559]: Couldn't set pass-filter in kernel: Invalid argument
pppd[4559]: local  IP address 46.157.166.73
pppd[4559]: remote IP address 10.0.0.1
pppd[4559]: primary   DNS address 193.213.112.4
pppd[4559]: secondary DNS address 130.67.15.198
NetworkManager[2952]: <info> PPP manager (IPv4 Config Get) reply received.
NetworkManager[2952]: nm_device_activate_schedule_ip4_config_result: assertion 'NM_IS_IP4_CONFIG (config)' failed
NetworkManager[2952]: local  IP address 46.157.166.73
NetworkManager[2952]: remote IP address 10.0.0.1
NetworkManager[2952]: primary   DNS address 193.213.112.4
NetworkManager[2952]: secondary DNS address 130.67.15.198
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (nm_phasechange): status 9 / phase 'running'
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (nm_ip_up): ip-up event
NetworkManager[2952]: nm-pppd-plugin-Message: nm-ppp-plugin: (nm_ip_up): sending IPv4 config to NetworkManager...
[...]

Comment on attachment 279467 [details]
Debug-level journal from MM+NM - issue #3

(In reply to comment #30)

> Issue #3: Interface ID of global IPv6 address assigned to WWAN ethernet
> interface is the special all-zeroes Subnet-Router anycast address

As described in comment #55, this issue appears fixed now; the IPv6 address uses the network-assigned Interface ID in all cases I can test. Obsoleting the related attachment.

Tore

Comment on attachment 279466 [details]
Debug-level journal from MM+NM - issue #2

As this is really a ModemManager issue, I'm obsoleting this attachment. I've opened bug #733394 in ModemManager instead.

(In reply to comment #54)
> Now you are using this guint8[8] construct.
> 
> It also cannot be returned from a function or conveniently checked "if(id)". So
> where is the advantage to the union from before?
> 
> with the union you could conveniently copy the entire id:
> 
> -    NM_MODEM_GET_PRIVATE (self)->iid.id = iid->id;
> +    memcpy (NM_MODEM_GET_PRIVATE (self)->iid, iid, sizeof (*iid)));
> 
> you even could somewhat conveniently distingish "unset" from "set":
>      if (NM_MODEM_GET_PRIVATE (self)->id) 
> 
> The union does prevent byte ordering ambiguities too. Byte ordering becomes
> only relevant, if you try to mess with parts of the guint64. When accessing
> parts of it, you must use the array alias id_u8, where you have full (obvious)
> control over the byte ordering. If you just care about the id as a whole, byte
> ordering doesn't matter, and you can access the "id" alias. And 0 is 0,
> regardless of byte ordering.
> 
> Just do the *out_iid stuff plus the union as before.

Done.  To satisfy some of the concerns about treating the non-integer IID as a guint64, I added a comment to the union and some accessor functions.  Also added some 'const' in places where the IID gets passed into a function and shouldn't change.

(In reply to comment #55)
> [...]
> <info> Activation (wlo1) successful, device activated.
> bound to 192.168.0.18 -- renewal in 1679 seconds.
> *** stack smashing detected ***: /usr/sbin/NetworkManager terminated
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x3433075cff)[0x7fc715e98cff]
> /lib64/libc.so.6(__fortify_fail+0x37)[0x7fc715f29b17]
> /lib64/libc.so.6(__fortify_fail+0x0)[0x7fc715f29ae0]
> /usr/sbin/NetworkManager(+0x36ec3)[0x7fc71a4ffec3]
> /usr/sbin/NetworkManager(+0x3a517)[0x7fc71a503517]
> /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x143)[0x7fc71688d283]
> /lib64/libglib-2.0.so.0(+0x3435049628)[0x7fc71688d628]
> /lib64/libglib-2.0.so.0(g_main_loop_run+0x6a)[0x7fc71688da3a]
> /usr/sbin/NetworkManager(main+0xc36)[0x7fc71a4f95f6]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7fc715e44d65]
> /usr/sbin/NetworkManager(+0x32799)[0x7fc71a4fb799]
> [...]

Are you sure you re-installed the wifi plugin?  That sometimes happens to me if I don't "cd src/devices/wifi/; sudo make install".  Since some of the object members have changed, that would be required.  If it still happens after that, could you run under valgrind?

> 2) Connecting to an IPv4-only APN, there's an assert, and the connection hangs
> forever (with all my modems in all modes):
> 
> [...]
> <info> (cdc-wdm2): IPv4 static configuration:
> <info>   address 46.66.30.183/28
> <info>   gateway 46.66.30.178
> <info>   DNS 193.213.112.4
> <info>   DNS 130.67.15.198
> nm_device_activate_schedule_ip4_config_result: assertion 'NM_IS_IP4_CONFIG
> (config)' failed
> [...]

Fixed.

> 3) Connecting to an IPv4-only APN using the Nokia 21M-02 in PPP mode
> additionally yields errors about failing to read hw address size, and 

Fixed.

something
> about a "pass-filter" failing to get set. While IPv4 addressing and routing

No idea about the "pass-filter" thing.

> does get configured on ppp0, /etc/resolv.conf is not written, and the
> connection attempts hangs forever (as in #2):

Same cause as #2, should be fixed now.

I just skimmed the updated iid stuff, but it's mostly like it was originally again, right? Looks good.

>> ppp: add IPv6 support

as I said, I would replace g_warning() by g_message() in add_ip6_notifier(). No need to warn every time if pppd does not support ipv6.




>> core: use Interface Identifiers for IPv6 SLAAC addresses

in addrconf6_start_with_link_ready(), could you move the new lines a bit up to the place where you removed the previous ones?


-    /* FIXME: what if interface has no lladdr, like PPP? */
-    hw_addr = nm_platform_link_get_address (nm_device_get_ip_ifindex (self), &hw_addr_len);
-    if (hw_addr_len)
-         nm_rdisc_set_lladdr (priv->rdisc, (const char *) hw_addr, hw_addr_len);
-
+    if (NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, &iid))
+         nm_rdisc_set_iid (priv->rdisc, iid);
+    else
+         nm_log_warn (LOGD_IP6, "(%s): failed to get interface identifier", nm_device_get_ip_iface (self));



 
I still think, that addresses should not be created if the IfaceId is unset:

          if (ndp_msg_opt_prefix_flag_auto_addr_conf (msg, offset)) {
-              if (route.plen == 64) {
+              if (route.plen == 64 && rdisc->iid.id) {
                    memset (&address, 0, sizeof (address));


Rest looks good to me

(In reply to comment #61)
> >> ppp: add IPv6 support
> 
> as I said, I would replace g_warning() by g_message() in add_ip6_notifier(). No
> need to warn every time if pppd does not support ipv6.

Fixed.

> >> core: use Interface Identifiers for IPv6 SLAAC addresses
> 
> in addrconf6_start_with_link_ready(), could you move the new lines a bit up to
> the place where you removed the previous ones?
> 
> 
> -    /* FIXME: what if interface has no lladdr, like PPP? */
> -    hw_addr = nm_platform_link_get_address (nm_device_get_ip_ifindex (self),
> &hw_addr_len);
> -    if (hw_addr_len)
> -         nm_rdisc_set_lladdr (priv->rdisc, (const char *) hw_addr,
> hw_addr_len);
> -
> +    if (NM_DEVICE_GET_CLASS (self)->get_ip_iface_identifier (self, &iid))
> +         nm_rdisc_set_iid (priv->rdisc, iid);
> +    else
> +         nm_log_warn (LOGD_IP6, "(%s): failed to get interface identifier",
> nm_device_get_ip_iface (self));

Done.

> I still think, that addresses should not be created if the IfaceId is unset:
> 
>           if (ndp_msg_opt_prefix_flag_auto_addr_conf (msg, offset)) {
> -              if (route.plen == 64) {
> +              if (route.plen == 64 && rdisc->iid.id) {
>                     memset (&address, 0, sizeof (address));

I'm a bit concerned that we aren't able to generate IIDs for some kinds of interfaces, but obviously the interface must have some kind of IID to hash into the address, otherwise two hosts on the network with the same code could conflict.  So this is OK.

But, I'd like to prevent that from happening earlier than this point, so I added:

trivial: move addrconf6_start_with_link_ready() above caller
core: fail IPv6 router discovery if IID cannot be generated

to the middle of the branch.  This should fail IPv6 (but not IPv4) if the IID cannot be generated, instead of leaving the interface with an IPv6 config but no IPv6 address.

Better now?

(In reply to comment #62)
> (In reply to comment #61)

> > I still think, that addresses should not be created if the IfaceId is unset:
> > 
> >           if (ndp_msg_opt_prefix_flag_auto_addr_conf (msg, offset)) {
> > -              if (route.plen == 64) {
> > +              if (route.plen == 64 && rdisc->iid.id) {
> >                     memset (&address, 0, sizeof (address));
> 
> I'm a bit concerned that we aren't able to generate IIDs for some kinds of
> interfaces, but obviously the interface must have some kind of IID to hash into
> the address, otherwise two hosts on the network with the same code could
> conflict.  So this is OK.
> 
> But, I'd like to prevent that from happening earlier than this point, so I
> added:
> 
> trivial: move addrconf6_start_with_link_ready() above caller
> core: fail IPv6 router discovery if IID cannot be generated
> 
> to the middle of the branch.  This should fail IPv6 (but not IPv4) if the IID
> cannot be generated, instead of leaving the interface with an IPv6 config but
> no IPv6 address.
> 
> Better now?

Yes.

I see you added also the following:
-              if (route.plen == 64 && lladdrlen == 6) {
+              if (route.plen == 64 && rdisc->iid.id) {
with failing IPv6 on missing IID, this should not actually matter. But good to ensure at this place that we don't add addresses with host-part zero.

LGTM now.

I pushed this in two merges for clarity:

1) the non-WWAN IPv6 Interface Identifier stuff
2) the WWAN IPv6 support

Let's open new bugs for anything that turns up in the future, or for existing stuff like fallback if IPV4V6 PDP contexts fail to activate.