Usb tethering chains for IPv6

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: pgravel, Assigned: vchang)

Tracking

unspecified
2.2 S13 (29may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [caf priority: p2][CR 827032])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
[Blocking Requested - why for this release]: Needed for IPv6 usb tethering to work.

The usb tether enable and disable chain both require an additional netd command in the IPv6 case.

During enabling, the following netd command is required
   "tether interface add_upstream %s" (with %s = externalIname, ex: rmnet0)

During disabling, the following netd command is required
   "tether interface remove_upstream %s" (with %s = externalIfname)

Modified chains:
>CommandFunc NetworkUtils::sUSBEnableChainIPv6[] = {
>  NetworkUtils::setInterfaceUp,
>  NetworkUtils::enableNat,
>  NetworkUtils::setIpForwardingEnabled,
>  NetworkUtils::tetherInterface,
>  NetworkUtils::tetheringStatus,
>  NetworkUtils::startTethering,
>  NetworkUtils::setDnsForwarders,
>+  NetworkUtils::addUpstreamInterface,
>  NetworkUtils::usbTetheringSuccess
>};
> 
>CommandFunc NetworkUtils::sUSBDisableChainIPv6[] = {
>  NetworkUtils::untetherInterface,
>  NetworkUtils::preTetherInterfaceList,
>  NetworkUtils::postTetherInterfaceList,
>+  NetworkUtils::removeUpstreamInterface,
>  NetworkUtils::disableNat,
>  NetworkUtils::setIpForwardingEnabled,
>  NetworkUtils::stopTethering,
>  NetworkUtils::usbTetheringSuccess
>};

Comment 1

5 years ago
Hey Bhavana, sorry about raising this issue so late; took us a long time to chase the issue with other teams internally to get to this point.
Hi Chuck, please check if we can do any thing for this bug? Thanks.
Flags: needinfo?(chulee)
I think it would be more work than adding two entry into the chain, and not likely be done in 2.0 scope:
1. add_upstream/remove_upstream is not support in netd of android platform [1], yet. So it seems to be a platform-specific command. If we add this command into current chain, usb tethering of platforms don't support this command will break. We have to find a way to distinguish this.
2. We don't determine if external interface runs IPv4 or IPv6 or both now, we have to add this in network manager. Or maybe there will be side effect if we use this command while external interface is running IPv4.
3. We don't have the environment for testing, so it will take much time in the process of update patch and ask partner to get test result.

[1] https://android.googlesource.com/platform/system/netd/+/master/CommandListener.cpp
Flags: needinfo?(chulee)
(In reply to Anshul from comment #1)
> Hey Bhavana, sorry about raising this issue so late; took us a long time to
> chase the issue with other teams internally to get to this point.

Anshul, this sounds like a new feature to me, and given where we are for 2.0 or even 2.1 this might be hard to scope in at this point.

Comment 5

5 years ago
(In reply to bhavana bajaj [:bajaj] from comment #4)
> (In reply to Anshul from comment #1)
> > Hey Bhavana, sorry about raising this issue so late; took us a long time to
> > chase the issue with other teams internally to get to this point.
> 
> Anshul, this sounds like a new feature to me, and given where we are for 2.0
> or even 2.1 this might be hard to scope in at this point.
Bhavana, then lets just wait to see if MADAI asks for this feature. If they do then we can revisit it at that time.
[Blocking Requested - why for this release]:

Ken, I am moving this to backlog for now, please set it to appropriate relase.
blocking-b2g: 2.0? → 2.1?
blocking-b2g: 2.1? → backlog
Vincent, I know this is a device specific feature. But if we need to have this feature, what should we do and what is our design in firefox OS? Please check if someone from your team can start this evaluation.
Assignee: nobody → chulee
Flags: needinfo?(vchang)
(In reply to Ken Chang[:ken] from comment #7)
> Vincent, I know this is a device specific feature. But if we need to have
> this feature, what should we do and what is our design in firefox OS? Please
> check if someone from your team can start this evaluation.

Yeah, that's right. We should do some study about IPV6 for both wifi and usb tethering. I recall that AOSP doesn't support IPV6 for tethering. But I might be wrong. BTW, can we have pgravel's opinions on comment 3? Is it that easy to support IPV6 by executing upstream command? Do we have specifications/scopes on what we should support for IPV6.
Flags: needinfo?(vchang)
Flags: needinfo?(pgravel)
Flags: needinfo?(bbajaj)

Updated

5 years ago
No longer blocks: CAF-v2.0-CC-metabug
(In reply to Vincent Chang[:vchang] from comment #8)
> (In reply to Ken Chang[:ken] from comment #7)
> > Vincent, I know this is a device specific feature. But if we need to have
> > this feature, what should we do and what is our design in firefox OS? Please
> > check if someone from your team can start this evaluation.
> 
> Yeah, that's right. We should do some study about IPV6 for both wifi and usb
> tethering. I recall that AOSP doesn't support IPV6 for tethering. But I
> might be wrong. BTW, can we have pgravel's opinions on comment 3? Is it that
> easy to support IPV6 by executing upstream command? Do we have
> specifications/scopes on what we should support for IPV6.

 I think we should get pgravel's opinion here if scope isn't clear and also do some research around this area and then set the scope for future improvements here
Flags: needinfo?(bbajaj)
1. Set upstream if external interface has IPv6 address.
2. Still need a way to decide if netd support upstream command.
Attachment #8480327 - Flags: feedback?(vchang)
Attachment #8480327 - Flags: feedback?(pgravel)
(Reporter)

Comment 11

5 years ago
Comment on attachment 8480327 [details] [diff] [review]
WIP - Add USB tethring command supporting IPv6 outgoing interface.

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +731,5 @@
> +  }
> +
> +  bool isIPv6 = false;
> +  char interface[32];
> +  while(fscanf(file, "%*s %*s %*s %*s %32s", interface)) {

currently this goes into an infinite loop. Needs one more %*s to properly match up with the format of the file.

/proc/net $ cat if_inet6
2002c0239c1748035be391489ba1d82e 03 40 00 00   rmnet0
fe800000000000005be391489ba1d82e 03 40 20 80   rmnet0
fe800000000000006c9adbfffedd652e 1b 40 20 80   rndis0
00000000000000000000000000000001 01 80 10 80       lo

@@ +746,5 @@
> +void NetworkUtils::addUpstreamInterface(CommandChain* aChain,
> +                                        CommandCallback aCallback,
> +                                        NetworkResultOptions& aResult)
> +{
> +  const char *interface = GET_CHAR(mExternalIfname);

Need to assign this to a nsCString. GET_CHAR's return value is only valid temporarily and by the time the snprintf occurs below this pointer is no longer valid and we get trash for the ifname.
Attachment #8480327 - Flags: feedback?(pgravel) → feedback-
(Reporter)

Updated

5 years ago
Flags: needinfo?(pgravel)
(Reporter)

Comment 13

5 years ago
Comment on attachment 8487743 [details] [diff] [review]
WIP - Add USB tethring command supporting IPv6 outgoing interface.

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

Thanks, new patch looks good.
Attachment #8487743 - Flags: feedback?(pgravel) → feedback+
Rebase.
I can't actually test if it works in Taiwan, but should work based on feedback+ per comment 13.
Attachment #8487743 - Attachment is obsolete: true
Attachment #8491312 - Flags: review?(vyang)
Attachment #8491312 - Flags: review?(fabrice)
Comment on attachment 8491312 [details] [diff] [review]
Add USB tethring command supporting IPv6 outgoing interface.

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

I defer to Vicamo.
Attachment #8491312 - Flags: review?(fabrice)
I am going to take this one. Feel free to take it if you are still working on it.
Assignee: chuckli0706 → vchang
blocking-b2g: backlog → ---
(Reporter)

Comment 18

4 years ago
Any chance this could be brought in for 2.2?

Our test teams are trying complete their test suites and their ipv6 tethering tests are blocked by this issue.

The patch provided by :chucklee still applies to 2.2 after a trivial rebase. I've done some sanity testing with the patch on 2.2 and verified it makes ipv6 usb tethering works and doesn't affect ipv4 usb tethering.
blocking-b2g: --- → 2.2?
Whiteboard: [CR 827032]
Hi! Vincent,

How do you think?

--
Keven
Flags: needinfo?(vchang)
The command is not supported by Lollipop 5.1 [1] yet, I would like to suggest that the parter picks up the patch and maintains in their tree. 

[1] http://androidxref.com/5.1.0_r1/xref/system/netd/server/CommandListener.cpp#609
Flags: needinfo?(vchang)
(In reply to Vincent Chang[:vchang] from comment #8)
> Yeah, that's right. We should do some study about IPV6 for both wifi and usb
> tethering. I recall that AOSP doesn't support IPV6 for tethering.
Is IPV6 supported for tethering in LL 5.1?
(In reply to Ken Chang[:ken] from comment #21)
> (In reply to Vincent Chang[:vchang] from comment #8)
> > Yeah, that's right. We should do some study about IPV6 for both wifi and usb
> > tethering. I recall that AOSP doesn't support IPV6 for tethering.
> Is IPV6 supported for tethering in LL 5.1?

Not quite sure, but I don't find officially statement about IPV6 tethering support in LL5.1.
I don't see related implementation in [1], either.     

[1] http://androidxref.com/5.1.0_r1/xref/system/netd/server/TetherController.cpp
Whiteboard: [CR 827032] → [caf priority: p2][CR 827032]
I would like to clarify the problem a little bit. The big concern here is that the patch uses "tether interface add_upstream [interface name]" command which is not supported in AOSP. Even if we can use the property like "ro.ipv6tethering.supported" to distinguish from different platfroms, how can we identify the responsibility when we find the bugs?
Rebase and use readonly property to distinguish platform with IPV6 tethering support. The property name is defined as "ro.tethering.ipv6".
Attachment #8491312 - Attachment is obsolete: true
Attachment #8601932 - Flags: review?(hchang)
Comment on attachment 8601932 [details] [diff] [review]
Patch v1.0 for usb IPV6 tethering

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

Looks good to me generally except the correctness of skipping current command chain entry. Other than that are all my
personal opinions. Not a must to address. Thanks!

::: dom/system/gonk/NetworkUtils.cpp
@@ +54,5 @@
> +// IPV6 Tethering is not supported in AOSP, use the property to
> +// identify vendor specific support in IPV6. We can remove this flag
> +// once upstream Android support IPV6 in tehtering.
> +static const char* IPV6_TETHERING = "ro.tethering.ipv6";
> +

Would it be better to add some prefix to this qcom proprietary feature to avoid potential naming pollution?

@@ +85,5 @@
>  
>  static const int32_t SUCCESS = 0;
>  
>  static uint32_t SDK_VERSION;
> +static uint32_t SUPPORT_IPV6_TETHERING;

It's not a constant actually. But since we already use SDK_VERSION for similar purpose, I think it's fine to me.

@@ +865,5 @@
> +    interface = GET_CHAR(mCurExternalIfname);
> +  }
> +
> +  if (SUPPORT_IPV6_TETHERING == 0 || !isCommandChainIPv6(aChain, interface.get())) {
> +    nextNetdCommand();

I doubt if this works. One of the right ways to skip current command is to callback but not "nextNetdCommand". nextNetdCommand will execute the netd command we queue in this command chain entry. Since we didn't queue any command, we will not get a callback from netd and move to next command chain entry.

@@ +882,5 @@
> +{
> +  nsCString interface(GET_CHAR(mExternalIfname));
> +  if (!interface.get()[0]) {
> +    interface = GET_CHAR(mPreExternalIfname);
> +  }

Is there any reason that we cannot do this in isCommandChainIPv6? Since we will pass the entire command chain to the function and the function name is so specific and clear, it makes perfect sense to determine if a command chain is ipv6 by only "command chain".
Attachment #8601932 - Flags: review?(hchang)
triage: blocking CAF 2.2
blocking-b2g: 2.2? → 2.2+
Posted patch Patch v02 (obsolete) — Splinter Review
Address the review comment.
Attachment #8601932 - Attachment is obsolete: true
Attachment #8603300 - Flags: review?(hchang)
Comment on attachment 8603300 [details] [diff] [review]
Patch v02

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

Thanks Vincent!
Attachment #8603300 - Flags: review?(hchang) → review+
Would like to get some feedbacks and test results from our partner
Flags: needinfo?(pgravel)
(Reporter)

Comment 30

4 years ago
Comment on attachment 8603300 [details] [diff] [review]
Patch v02

Thanks vincent,

Patch v02 worked for me.

Small nit-pick: typo in comment on line 56 "tehtering"
Flags: needinfo?(pgravel)
Attachment #8603300 - Flags: feedback+
Posted patch Patch v03Splinter Review
Update the patch to address the nit.
Attachment #8603300 - Attachment is obsolete: true
Comment on attachment 8605042 [details] [diff] [review]
Patch v03

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no 
User impact if declined: can't support ipv6 in tethering. 
Testing completed: partner has approved and verified the patch 
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: no
Attachment #8605042 - Flags: approval-mozilla-b2g37?
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Attachment #8605042 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

4 years ago
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/9c01d964504c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S13 (29may)
Blocks: 1183002
You need to log in before you can comment on or make changes to this bug.