Closed Bug 975813 Opened 6 years ago Closed 6 years ago

[IPv6] To support IPv6 in network manager.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: kchang, Assigned: chucklee)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(3 files, 13 obsolete files)

1.77 KB, text/plain
Details
3.17 KB, text/plain
Details
15.35 KB, patch
Details | Diff | Splinter Review
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.
Chuck, pleas help to take this bug.
Assignee: nobody → chulee
Anshul, Can you provide the information of IPv6 which commercial RIL wants to pass to network manager for this bug? Thanks.
Flags: needinfo?(anshulj)
Blocks: 957917
(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.
Whiteboard: [FT:RIL]
Target Milestone: --- → 1.4 S2 (28feb)
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.
Flags: needinfo?(anshulj) → needinfo?(pgravel)
Blocks: 960372
(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.
Status: NEW → ASSIGNED
Blocks: 976427
> 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.
Flags: needinfo?(pgravel)
One additional note: APN information will need to be expanded to allow users to explicitly set IPv4 vs IPv6.
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.
(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
(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.
(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.
(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?
Flags: needinfo?(anshulj)
Attachment #8381335 - Attachment is obsolete: true
Attachment #8381950 - Flags: feedback?(vyang)
Attachment #8381950 - Flags: feedback?(anshulj)
Attached patch Test - RIL IPv6 Simulation (obsolete) — Splinter Review
Simulate IPv6 address from RIL to verify routing table commands.
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.
(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.
Depends on: 976959
(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"]'}
Flags: needinfo?(anshulj)
Set IPv6 DNS based on https://code.google.com/p/android/issues/detail?id=3389#c30.
Attachment #8381950 - Attachment is obsolete: true
Attachment #8381950 - Flags: feedback?(vyang)
Attachment #8381950 - Flags: feedback?(anshulj)
Attachment #8382740 - Flags: review?(vyang)
Attachment #8382740 - Flags: review?(anshulj)
Attached patch Test - RIL IPv6 Simulation (obsolete) — Splinter Review
Add IPv6 DNS address.
Attachment #8381951 - Attachment is obsolete: true
Attachment #8382740 - Flags: review?(vyang)
Attachment #8382740 - Flags: review?(anshulj)
Attachment #8382740 - Flags: feedback?(vyang)
Attachment #8382740 - Flags: feedback?(anshulj)
Modify based on bug 976959
Attachment #8382740 - Attachment is obsolete: true
Attachment #8382740 - Flags: feedback?(vyang)
Attachment #8382740 - Flags: feedback?(anshulj)
Attachment #8382867 - Flags: feedback?(vyang)
Attachment #8382867 - Flags: feedback?(anshulj)
Attached patch Test - RIL IPv6 Simulation (obsolete) — Splinter Review
Modify IPv6 simulation parameter.
Attachment #8382741 - Attachment is obsolete: true
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.
Attachment #8382867 - Flags: feedback?(vyang) → feedback+
Attached patch Support IPv6 in NetworkUtils. (obsolete) — Splinter Review
Add hostname chekc per comment 25.
Attachment #8382867 - Attachment is obsolete: true
Attachment #8382867 - Flags: feedback?(anshulj)
Attachment #8382930 - Flags: review?(vyang)
Attachment #8382930 - Flags: feedback?(anshulj)
Attached patch Test - RIL IPv6 Simulation (obsolete) — Splinter Review
Rebase
Attachment #8382869 - Attachment is obsolete: true
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.
Attachment #8382930 - Flags: review?(vyang)
Attached patch Support IPv6 in NetworkUtils. V2 (obsolete) — Splinter Review
Fix NetworkUtils::removeDefaultRoute()
Attachment #8382930 - Attachment is obsolete: true
Attachment #8382930 - Flags: feedback?(anshulj)
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?
Attachment #8382971 - Flags: feedback?(pgravel)
(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 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
Attachment #8382971 - Flags: review?(vyang) → feedback+
Attached patch Support IPv6 in NetworkUtils. V3 (obsolete) — Splinter Review
Address my previous comments and fix the problem reported in bug 957917 comment 23.
Attachment #8382971 - Attachment is obsolete: true
Attachment #8382971 - Flags: feedback?(pgravel)
Attachment #8383200 - Flags: review?(fabrice)
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
Attachment #8383200 - Flags: review?(fabrice) → review+
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.
(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).
Tested V3 patch, was able to get a successful ping on an ipv6 connection.
Address comment 34 and comment 35, except the uuid part.
Attachment #8383200 - Attachment is obsolete: true
(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 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
(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.
Address comment 40 and comment 41, thanks to Vicamo!
Attachment #8383480 - Attachment is obsolete: true
Rebase based on bug 960865.
Attachment #8382934 - Attachment is obsolete: true
Attachment #8383521 - Attachment is obsolete: true
I think the oranges are not related to this patch, let's check in.
Keywords: checkin-needed
(In reply to Edgar Chen [:edgar][:echen] from comment #47)
> https://hg.mozilla.org/integration/b2g-inbound/rev/e9b4e110a6a7
Thanks, Edgar.
https://hg.mozilla.org/mozilla-central/rev/e9b4e110a6a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.