975813 - [IPv6] To support IPv6 in network manager.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ken Chang[:kenkai]]

To support IPv6 for partner's requirements, we need to support IPv6 in network manager. RIL needs to get the the relevant information of IPv6 from carrier's network and pass this information to network manager, and then network manager will do the configuration accordingly.

Comment 1

[Ken Chang[:kenkai]]

Chuck, pleas help to take this bug.

Comment 2

[Ken Chang[:kenkai]]

Anshul, Can you provide the information of IPv6 which commercial RIL wants to pass to network manager for this bug? Thanks.

Comment 3

[Ken Chang[:kenkai]]

(In reply to Ken Chang[:ken] from comment #2)
> Anshul, Can you provide the information of IPv6 which commercial RIL wants
> to pass to network manager for this bug? Thanks.

We already support IPv6 feature in Necko. Currently, we are not sure if IPv6 are supported in network manager. But we think it dons. May you have a trial? If there is any problem occurred, please let us know what problem you have in network manager for IPv6.

Comment 4

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

It seems that gecko do nothing about settings IP of data call, maybe done by rild.
In Network Utils:
|setDefaultRouteAndDNS()| and |removeNetworkRoute()| might fail because it uses inet_addr().
DHCP and DNS related functions might fail if gonk doesn't support.
Others like |SetHostRoute()| seems fine.

So for Network Manager, the main problem is handling IPv6 routing rules, like set default route.

Comment 5

[Ken Chang[:kenkai]]

(In reply to Chuck Lee [:chucklee] from comment #4)
> It seems that gecko do nothing about settings IP of data call, maybe done by
> rild.
> In Network Utils:
> |setDefaultRouteAndDNS()| and |removeNetworkRoute()| might fail because it
> uses inet_addr().
Thanks for this analysation, but let's check what we have to do after getting the test result which partner provide.

Comment 6

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: WIP - 0001. Support IPv6 in NetworkUtils.

Use IPv6-supported functions.

Comment 7

[pgravel]

> D/DHCP    (  216): ifc_add_route: different address families: 0.0.0.0 and 2002:c023:9c17:4135:5c23:4743:df47:0eca
> D/DHCP    (  216): getaddrinfo failed: invalid address
> D/NetUtils(  216): failed to remove default route for rmnet0: No such process
> D/NetUtils(  216): failed to add 255.255.255.255 as default route for rmnet0: No such process
> D/DHCP    (  216): ifc_add_route: different address families: 0.0.0.0 and 2002:c023:9c17:4135:5c23:4743:df47:0eca
> D/DHCP    (  216): getaddrinfo failed: invalid address
> D/NetUtils(  216): failed to remove default route for rmnet0: No such process
> D/NetUtils(  216): failed to add 255.255.255.255 as default route for rmnet0: No such process
> E/BandwidthController(  209): No such iface rmnet0 to delete

As :chucklee mentioned, some of the network utils code expects an ipv4 address and doesn't know what to do with ipv6-formatted addresses.

To answer an earlier question, RIL will be providing the same information as when on IPv4, which is IP address, gateways, DNS, netmask, and broadcast.
The network manage could distinguish between IPv4 and IPv6 simply based on the format & length of the IP address, or nsIRilNetworkInterface could be expanded to include an explicit GetVersion() function.

Comment 8

[pgravel]

One additional note: APN information will need to be expanded to allow users to explicitly set IPv4 vs IPv6.

Comment 9

[Ken Chang[:kenkai]]

Thanks for your help. 
Chuck will base on your result and review his patch. May need your help to test his patch. Thanks.

(In reply to pgravel from comment #8)
> One additional note: APN information will need to be expanded to allow users
> to explicitly set IPv4 vs IPv6.
Carrier said they didn't need seeting for configuring ipv4/ipv6, so we don't plan to have this in 1.4.

Comment 10

[Anshul]

(In reply to Ken Chang[:ken] from comment #9)
> Thanks for your help. 
> Chuck will base on your result and review his patch. May need your help to
> test his patch. Thanks.
> 
> (In reply to pgravel from comment #8)
> > One additional note: APN information will need to be expanded to allow users
> > to explicitly set IPv4 vs IPv6.
> Carrier said they didn't need seeting for configuring ipv4/ipv6, so we don't
> plan to have this in 1.4.
Sure, but just to make it clear, this information still needs to be passed to gecko as part of the APN settings so modem know to bring up an IPv4, IPv6 or both

Comment 11

[Ken Chang[:kenkai]]

(In reply to Anshul from comment #10)
> to gecko as part of the APN settings so modem know to bring up an IPv4, IPv6
> or both.
For now, gecko only sets both for this setting.

Comment 12

[Ken Chang[:kenkai]]

(In reply to Ken Chang[:ken] from comment #11)
> For now, gecko only sets both for this setting.
By the way, protocol setting have been passed to RIL now, commercial RIL can directly use this protocol setting.

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Anshul from comment #10)
> (In reply to Ken Chang[:ken] from comment #9)
> > Thanks for your help. 
> > Chuck will base on your result and review his patch. May need your help to
> > test his patch. Thanks.
> > 
> > (In reply to pgravel from comment #8)
> > > One additional note: APN information will need to be expanded to allow users
> > > to explicitly set IPv4 vs IPv6.
> > Carrier said they didn't need seeting for configuring ipv4/ipv6, so we don't
> > plan to have this in 1.4.
> Sure, but just to make it clear, this information still needs to be passed
> to gecko as part of the APN settings so modem know to bring up an IPv4, IPv6
> or both
Hi Anshul,
  If it's set to both, does modem bring up both IPv4 and IPv6 information at the same time if carrier supports both, or just one of them?

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: WIP - 0001. Support IPv6 in NetworkUtils.

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Test - RIL IPv6 Simulation

Simulate IPv6 address from RIL to verify routing table commands.

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Network status before enable data connection (Unagi.)

Comment 17

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Network status after enable data connection (Unagi.)

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I overwrite the IP, prefix, and gateway of data connection interface while it's connected in the Test patch(attachment 8381951 [details] [diff] [review]), and apply test on unagi.

IP and Routing table of IPv6 for data connection(rmnet0) are not set before enabling data connection , as attachment 8381952 [details].

After enabling data connection, IPv6 address "20010db80000f1010000000000000002" is assigned to rmnet0(I did this in test patch, should be rild I think), and two routing rules(subnet route 20010db80000f1010000000000000000 and default 00000000000000000000000000000000) are added to IPv6 routing table through rmnet0.

I think the WIP patch might work with IPv6 data connection.

Comment 19

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Chuck Lee [:chucklee] from comment #14)
> Created attachment 8381950 [details] [diff] [review]
> WIP - 0001. Support IPv6 in NetworkUtils.

Note: In this patch, IPv6 address expects prefix of subnet instead of netmask.

Comment 20

[pgravel]

(In reply to Chuck Lee [:chucklee] from comment #13)
> (In reply to Anshul from comment #10)
> > (In reply to Ken Chang[:ken] from comment #9)
> > > Thanks for your help. 
> > > Chuck will base on your result and review his patch. May need your help to
> > > test his patch. Thanks.
> > > 
> > > (In reply to pgravel from comment #8)
> > > > One additional note: APN information will need to be expanded to allow users
> > > > to explicitly set IPv4 vs IPv6.
> > > Carrier said they didn't need seeting for configuring ipv4/ipv6, so we don't
> > > plan to have this in 1.4.
> > Sure, but just to make it clear, this information still needs to be passed
> > to gecko as part of the APN settings so modem know to bring up an IPv4, IPv6
> > or both
> Hi Anshul,
>   If it's set to both, does modem bring up both IPv4 and IPv6 information at
> the same time if carrier supports both, or just one of them?

Depends on the network, but both is possible. Example:
> UNSOL_DATA_CALL_LIST_CHANGED { ver=9, num=1, connection[0]:{ver : 0x9, status : 0, retry : -1, cid : 0, active : 1, type : 'IPV4V6', ifname : 'rmnet0', addresses : '["172.20.184.116/29","2002:c023:9c17:41aa:2285:0e0d:f30d:5726/64"]', dnses : '["192.35.156.212","192.35.156.19","2001:04f8:0000:0002:0000:0000:0000:0014"]', gateways : '["172.20.184.117","2002:c023:9c17:41aa:68bf:8e28:689e:4cf5"]'}

Comment 21

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: WIP - 0001. Support IPv6 in NetworkUtils.

Set IPv6 DNS based on https://code.google.com/p/android/issues/detail?id=3389#c30.

Comment 22

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Test - RIL IPv6 Simulation

Add IPv6 DNS address.

Comment 23

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: WIP - 0001. Support IPv6 in NetworkUtils.

Modify based on bug 976959

Comment 24

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Test - RIL IPv6 Simulation

Modify IPv6 simulation parameter.

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8382867 [details] [diff] [review]
WIP - 0001. Support IPv6 in NetworkUtils.

Review of attachment 8382867 [details] [diff] [review]:
-----------------------------------------------------------------

Need to handle IPv6 and empty hostname in addHostRoute/removeHostRoute as well.

Comment 26

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Support IPv6 in NetworkUtils.

Add hostname chekc per comment 25.

Comment 27

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Test - RIL IPv6 Simulation

Rebase

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8382930 [details] [diff] [review]
Support IPv6 in NetworkUtils.

Review of attachment 8382930 [details] [diff] [review]:
-----------------------------------------------------------------

D/NetUtils(   45): failed to remove default route for eth1: No such process

Looks like we need to support IPv6 in NetworkUtils::removeDefaultRoute, too.  We'll need an additional parameter 'gateway', so probably we should have nsINetworkService::removeDefaultRoute accept a nsINetworkInterface instead.

Comment 29

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Support IPv6 in NetworkUtils. V2

Fix NetworkUtils::removeDefaultRoute()

Comment 30

[Ken Chang[:kenkai]]

Comment on attachment 8382971 [details] [diff] [review]
Support IPv6 in NetworkUtils. V2

Philippe, could you please also test this patch to see if it is workable?

Comment 31

[Ken Chang[:kenkai]]

(In reply to Ken Chang[:ken] from comment #30)
> Comment on attachment 8382971 [details] [diff] [review]
> Support IPv6 in NetworkUtils. V2
> 
> Philippe, could you please also test this patch to see if it is workable?
You also need to base on the patch of bug 976959. Thanks.

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8382971 [details] [diff] [review]
Support IPv6 in NetworkUtils. V2

Review of attachment 8382971 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/NetworkUtils.cpp
@@ +1162,5 @@
>   */
>  bool NetworkUtils::removeDefaultRoute(NetworkParams& aOptions)
>  {
> +  // Get gateway address.
> +  const char *gateway = GET_CHAR(mGateway);

nit: append "removeDefaultRoute" to the comments of NetworkCommandOptions::gateway in "dom/webidl/NetworkOptions.webidl".

@@ +1261,5 @@
> +
> +  if (type == AF_INET6) {
> +    // Calculate subnet.
> +    unsigned char ipBuf[16] = {0}, subnetBuf[16] = {0};
> +    if (inet_pton(AF_INET6, ipStr, ipBuf) != 1) {

struct in6_addr in6;
if (inet_pton(AF_INET6, ipStr, &in6) != 1) {

@@ +1274,5 @@
> +        subnetBuf[i] |= (mask & ipBuf[i]);
> +        mask >>= 1;
> +        prefixLength--;
> +      }
> +    }

uint32_t p, i, p1, mask;
p = prefixLength;
i = 0;
while (i < 4) {
  p1 = p > 32 ? 32 : p;
  p -= p1;
  mask = p1 ? ~0x0 << (32 - p1) : 0;
  in6.s6_addr32[i++] &= htonl(mask);
}

@@ +1277,5 @@
> +      }
> +    }
> +
> +    char subnet[INET6_ADDRSTRLEN];
> +    if (!inet_ntop(AF_INET6, subnetBuf, subnet, INET6_ADDRSTRLEN)) {

char subnetStr[INET6_ADDRSTRLEN];
if (!inet_ntop(AF_INET6, &in6, subnetStr, sizeof subnetStr)) {

@@ +1285,5 @@
> +    // Remove default route.
> +    mNetUtils->do_ifc_remove_route(GET_CHAR(mIfname), "::", 0, NULL);
> +
> +    // Remove subnet route.
> +    mNetUtils->do_ifc_remove_route(GET_CHAR(mIfname), subnet, prefixLength, NULL);

nit: subnetStr

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Support IPv6 in NetworkUtils. V3

Address my previous comments and fix the problem reported in bug 957917 comment 23.

Comment 34

[[:fabrice] Fabrice Desré]

Comment on attachment 8383200 [details] [diff] [review]
Support IPv6 in NetworkUtils. V3

Review of attachment 8383200 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed

::: dom/system/gonk/NetworkService.js
@@ +11,5 @@
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  Cu.import("resource://gre/modules/FileUtils.jsm");
>  
>  const NETWORKSERVICE_CONTRACTID = "@mozilla.org/network/service;1";
> +const NETWORKSERVICE_CID = Components.ID("{f96461fa-e844-45d2-a6c3-8cd23ab0916b}");

no need to change that.

::: dom/system/gonk/NetworkService.manifest
@@ +1,3 @@
>  # NetworkService.js
> +component {f96461fa-e844-45d2-a6c3-8cd23ab0916b} NetworkService.js
> +contract @mozilla.org/network/service;1 {f96461fa-e844-45d2-a6c3-8cd23ab0916b}

No need to change these.

::: dom/system/gonk/NetworkUtils.cpp
@@ +289,5 @@
>    snprintf(key, PROPERTY_KEY_MAX - 1, "net.%s.dns2", ifname);
>    property_get(key, prop.dns2, "");
>  }
>  
> +static int getIpType(const char *ip) {

nit: s/ip/aIp

Comment 35

[[:fabrice] Fabrice Desré]

Comment on attachment 8383200 [details] [diff] [review]
Support IPv6 in NetworkUtils. V3

Review of attachment 8383200 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/NetworkUtils.cpp
@@ +1126,5 @@
> +    snprintf(key, sizeof key - 1, "net.%s.gw", autoIfname.get());
> +    property_get(key, gateway, "");
> +  } else {
> +    MOZ_ASSERT(strlen(GET_CHAR(mGateway_str)) < sizeof gateway);
> +    strcpy(gateway, GET_CHAR(mGateway_str));

Please use strncpy as the MOZ_ASSERT is a no-op in release builds.

Comment 36

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Fabrice Desré [:fabrice] from comment #34)
> Comment on attachment 8383200 [details] [diff] [review]
> Support IPv6 in NetworkUtils. V3
> 
> Review of attachment 8383200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +11,5 @@
> >  Cu.import("resource://gre/modules/NetUtil.jsm");
> >  Cu.import("resource://gre/modules/FileUtils.jsm");
> >  
> >  const NETWORKSERVICE_CONTRACTID = "@mozilla.org/network/service;1";
> > +const NETWORKSERVICE_CID = Components.ID("{f96461fa-e844-45d2-a6c3-8cd23ab0916b}");
> 
> no need to change that.
> 
> ::: dom/system/gonk/NetworkService.manifest
> @@ +1,3 @@
> >  # NetworkService.js
> > +component {f96461fa-e844-45d2-a6c3-8cd23ab0916b} NetworkService.js
> > +contract @mozilla.org/network/service;1 {f96461fa-e844-45d2-a6c3-8cd23ab0916b}
> 
> No need to change these.

If I don't change these values, I got black screen on device (Unagi).

Comment 37

[pgravel]

Tested V3 patch, was able to get a successful ping on an ipv6 connection.

Comment 38

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Support IPv6 in Network Manager. V4

Address comment 34 and comment 35, except the uuid part.

Comment 39

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try : https://tbpl.mozilla.org/?tree=Try&rev=11cb2857e3af

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Chuck Lee [:chucklee] from comment #36)
> If I don't change these values, I got black screen on device (Unagi).

@@ -242,7 +242,7 @@ SystemWorkerManager::RegisterNfcWorker(JS::Handle<JS::Value> aWorker,
 nsresult
 SystemWorkerManager::InitNetd(JSContext *cx)
 {
-  nsCOMPtr<nsIWorkerHolder> worker = do_GetService(kNetworkServiceCID);
+  nsCOMPtr<nsIWorkerHolder> worker = do_GetService("@mozilla.org/network/service;1");
   NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);
   mNetdWorker = worker;
   return NS_OK;

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8383480 [details] [diff] [review]
Support IPv6 in Network Manager. V4

Review of attachment 8383480 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/NetworkService.js
@@ +309,1 @@
>      if(DEBUG) debug("Remove default route for " + ifname);

nit: network.name

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Chuck Lee [:chucklee] from comment #36)
> If I don't change these values, I got black screen on device (Unagi).

Filed bug 977995 to completely remove NetworkService stuff from SystemWorkerManager.

Comment 43

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Support IPv6 in Network Manager. V5

Address comment 40 and comment 41, thanks to Vicamo!

Comment 44

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try : https://tbpl.mozilla.org/?tree=Try&rev=1a92a0acf90e

Comment 45

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Support IPv6 in Network Manager. V6

Rebase based on bug 960865.

Comment 46

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I think the oranges are not related to this patch, let's check in.

Comment 47

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e9b4e110a6a7

Comment 48

[Ken Chang[:kenkai]]

(In reply to Edgar Chen [:edgar][:echen] from comment #47)
> https://hg.mozilla.org/integration/b2g-inbound/rev/e9b4e110a6a7
Thanks, Edgar.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9b4e110a6a7