1057091 - Usb tethering chains for IPv6

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[pgravel]

[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

[Anshul]

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.

Comment 2

[Ken Chang[:kenkai]]

Hi Chuck, please check if we can do any thing for this bug? Thanks.

Comment 3

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

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

Comment 4

[bhavana bajaj [:bajaj]]

(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

[Anshul]

(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.

Comment 6

[bhavana bajaj [:bajaj]]

[Blocking Requested - why for this release]:

Ken, I am moving this to backlog for now, please set it to appropriate relase.

Comment 7

[Ken Chang[:kenkai]]

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.

Comment 8

[Vincent Chang[:vchang][changyihsin]]

(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.

Comment 9

[bhavana bajaj [:bajaj]]

(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

Comment 10

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

Attachment: WIP - Add USB tethring command supporting IPv6 outgoing interface.

1. Set upstream if external interface has IPv6 address.
2. Still need a way to decide if netd support upstream command.

Comment 11

[pgravel]

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.

Comment 12

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

Attachment: WIP - Add USB tethring command supporting IPv6 outgoing interface.

Comment 13

[pgravel]

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.

Comment 14

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

Attachment: Add USB tethring command supporting IPv6 outgoing interface.

Rebase.
I can't actually test if it works in Taiwan, but should work based on feedback+ per comment 13.

Comment 15

[[:fabrice] Fabrice Desré]

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.

Comment 16

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

Comment on attachment 8491312 [details] [diff] [review]
Add USB tethring command supporting IPv6 outgoing interface.

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

Ashes to ashes, dust to dust, non-AOSP to its own bug.

https://android.googlesource.com/platform/system/netd/+/android-4.3_r2.1/CommandListener.cpp#595
https://android.googlesource.com/platform/system/netd/+/android-4.4.2_r1/CommandListener.cpp#767
https://android.googlesource.com/platform/system/netd/+/l-preview/CommandListener.cpp#772
https://android.googlesource.com/platform/system/netd/+/master/CommandListener.cpp#772

Comment 17

[Vincent Chang[:vchang][changyihsin]]

I am going to take this one. Feel free to take it if you are still working on it.

Comment 18

[pgravel]

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.

Comment 19

[Keven Kuo [:kkuo]]

Hi! Vincent,

How do you think?

--
Keven

Comment 20

[Vincent Chang[:vchang][changyihsin]]

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

Comment 21

[Ken Chang[:kenkai]]

(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?

Comment 22

[Vincent Chang[:vchang][changyihsin]]

(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

Comment 23

[Vincent Chang[:vchang][changyihsin]]

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?

Comment 24

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0 for usb IPV6 tethering

Rebase and use readonly property to distinguish platform with IPV6 tethering support. The property name is defined as "ro.tethering.ipv6".

Comment 25

[Henry Chang [: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".

Comment 26

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: blocking CAF 2.2

Comment 27

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v02

Address the review comment.

Comment 28

[Henry Chang [:hchang]]

Comment on attachment 8603300 [details] [diff] [review]
Patch v02

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

Thanks Vincent!

Comment 29

[Vincent Chang[:vchang][changyihsin]]

Would like to get some feedbacks and test results from our partner

Comment 30

[pgravel]

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"

Comment 31

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v03

Update the patch to address the nit.

Comment 32

[Vincent Chang[:vchang][changyihsin]]

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

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c01d964504c

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c01d964504c

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d50110422703