1148319 - per-app network routing setting

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Comment 1

[Gary Chen [:xeonchen]]

we're going to implement a feature that allows every app to have its own host routing setting.

A possible solution is to set |SO_MARK| via |setsockopt()|, and filter packets using |ip rule| and |ip route| commands.

Comment 2

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1148319-per-app-network-routing-setting.patch

[experiment] set AppId as fwmark to HTTP packets

@mcmanus, we're going to support per-app routing feature, and my current
approach (and the first step) is to set per-app fwmark.

I can't find a proper place, where both |nsISocketTransport| and AppId are
available, to manage this behavior. |NS_NewChannel| might be a good entry
point, but I have to modify a lot of code to pass AppId in.

Besides, other non-URI-based connections (TCP/UDP) should be taken into
consideration as well.

Do you have any idea of this feature?

Comment 3

[Patrick McManus [:mcmanus]]

please work with dragana on this..

Comment 4

[Dragana Damjanovic [:dragana]]

You could add appID after:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4918

Probably you could take a look at patch https://bug1053650.bugzilla.mozilla.org/attachment.cgi?id=8588983
:henry did something similar for networkInterfaceId for firefox os.

nsHttpChannel already has appId.


UDPSocket already has appId
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsUDPSocket.cpp#798
so you only need to add options

there is a TCP socket initialize here, if you are interested in extending this too:
http://mxr.mozilla.org/mozilla-central/source/dom/network/TCPSocket.js#260

Comment 5

[Dragana Damjanovic [:dragana]]

Maybe have a option to set SO_MARK not for all apps just for some that needs it.

Comment 6

[Gary Chen [:xeonchen]]

Attachment: Part 1: enable SO_MARK

Comment 7

[Gary Chen [:xeonchen]]

Attachment: Part 2: set mark by app id on TCP

Comment 8

[Gary Chen [:xeonchen]]

Attachment: Part 3: set mark by app id on UDP

Comment 9

[Gary Chen [:xeonchen]]

Attachment: Part 1: enable SO_MARK in NSPR

Comment 10

[Gary Chen [:xeonchen]]

Attachment: Part 2: add SocketMarkerService to store AppID

add this service to store App IDs that have this feature enabled.

Comment 11

[Gary Chen [:xeonchen]]

Attachment: Part 3: set mark by app id on TCP

Comment 12

[Gary Chen [:xeonchen]]

Attachment: Part 4: set mark by app id on UDP

Comment 13

[Gary Chen [:xeonchen]]

Attachment: Part 2: add SocketMarkerService

add mode lines

Comment 14

[Gary Chen [:xeonchen]]

Attachment: Part 3: set mark by app id on TCP

add comment for idl

Comment 15

[Dragana Damjanovic [:dragana]]

Comment on attachment 8631416 [details] [diff] [review]
Part 3: set mark by app id on TCP

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

::: dom/network/TCPSocket.js
@@ +260,5 @@
> +                      .getService(Ci.nsISocketTransportService)
> +                      .createTransport(options, 1, host, port, null);
> +
> +#ifdef MOZ_WIDGET_GONK
> +    transport.sockOptMark = this._appId;

here sockOptMark is equal appid?

::: netwerk/base/nsSocketTransport2.cpp
@@ +1377,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    if (CheckMark(mSocketOptionMark)) {
> +        PRSocketOptionData opt;
> +        opt.option = PR_SockOpt_Mark;
> +        opt.value.mark = mSocketOptionMark;

This is confusing. The name of the variable mSocketOptionMark does not mention appid, but CheckMark parameter is called appId. Is a mark always to have the same value as the appid of the app?

If mSocketOptionMark is appId just call it like that.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4959,5 @@
>      } else {
>          LOG(("nsHttpChannel %p Using default connection info", this));
>          mConnectionInfo = new nsHttpConnectionInfo(host, port, EmptyCString(), mUsername, proxyInfo, isHttps);
> +        uint32_t appId = GetLoadContextInfo(this)->AppId();
> +        mConnectionInfo->SetAppId(appId);

Why is this inside if, actually only for else part?

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +62,5 @@
>      mProxyInfo = proxyInfo;
>      mEndToEndSSL = e2eSSL;
>      mUsingConnect = false;
>      mNPNToken = npnToken;
> +    mAppId = NECKO_NO_APP_ID;

You will need to change BuildHashKey and Clone functions. Because to connection that have different route marks are not the same.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3026,5 @@
>      }
>  
> +#ifdef MOZ_WIDGET_GONK
> +    uint32_t appId = args->mTrans->ConnectionInfo()->GetAppId();
> +    ent->mConnInfo->SetAppId(appId);

You probably do not need this if you change BuildHashKey and Clone functions (see nsHttpConnectionInfo.cpp comment).

@@ +3208,5 @@
>      socketTransport->SetQoSBits(gHttpHandler->GetQoSBits());
>  
> +#ifdef MOZ_WIDGET_GONK
> +    uint32_t appId = mEnt->mConnInfo->GetAppId();
> +    socketTransport->SetSockOptMark(appId);

you can combine this into one.

Comment 16

[Dragana Damjanovic [:dragana]]

Comment on attachment 8631396 [details] [diff] [review]
Part 1: enable SO_MARK in NSPR

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

The nspr part needs to be reviewed by Wan-Teh Chang (wtc).

Is it possible to do the same by changing just the necko code without changing nspr, actually probably it is too much work except we have access to some set/getoption functions?  It is easier to make changes in necko code

Comment 17

[Dragana Damjanovic [:dragana]]

Comment on attachment 8631415 [details] [diff] [review]
Part 2: add SocketMarkerService

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

::: netwerk/base/nsISocketMarkerService.idl
@@ +8,5 @@
> +/**
> + * nsISocketMarkerService
> + *
> + * NOTE: This is a free-threaded interface, meaning that the methods on
> + * this interface may be called from any thread.

Some more info what is this used for, would be helpful.

Comment 18

[Gary Chen [:xeonchen]]

(In reply to Dragana Damjanovic [:dragana] from comment #16)
> Comment on attachment 8631396 [details] [diff] [review]
> Part 1: enable SO_MARK in NSPR
> 
> Review of attachment 8631396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The nspr part needs to be reviewed by Wan-Teh Chang (wtc).
> 
> Is it possible to do the same by changing just the necko code without
> changing nspr, actually probably it is too much work except we have access
> to some set/getoption functions?  It is easier to make changes in necko code

Do you mean I can modify socket options in necko code?

Because I didn't see any necko code (except sctp) directly access socket options,
is there any example doing this?

Comment 19

[Dragana Damjanovic [:dragana]]

(In reply to Gary Chen [:xeonchen] from comment #18)
> (In reply to Dragana Damjanovic [:dragana] from comment #16)
> > Comment on attachment 8631396 [details] [diff] [review]
> > Part 1: enable SO_MARK in NSPR
> > 
> > Review of attachment 8631396 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The nspr part needs to be reviewed by Wan-Teh Chang (wtc).
> > 
> > Is it possible to do the same by changing just the necko code without
> > changing nspr, actually probably it is too much work except we have access
> > to some set/getoption functions?  It is easier to make changes in necko code
> 
> Do you mean I can modify socket options in necko code?
> 
> Because I didn't see any necko code (except sctp) directly access socket
> options,
> is there any example doing this?

Actually this is better in NSPR.

Comment 20

[Dragana Damjanovic [:dragana]]

Is there any reason you are not just binding to an address?
Binding is probably enough.

Comment 21

[Gary Chen [:xeonchen]]

Attachment: Part 1: enable SO_MARK in NSPR

Comment 22

[Patrick McManus [:mcmanus]]

(In reply to Dragana Damjanovic [:dragana] from comment #20)
> Is there any reason you are not just binding to an address?
> Binding is probably enough.

I have this same question.

also, is SO_MARK portable at all? (maybe more than I know?) If not, then you don't want to put it in NSPR.. you could resort to the nasty  PR_FileDesc2NativeHandle() instead... but it seems like bind() has the potential to sidestep this entirely and be a portable solution.. right now you've got a linux/firefox-os specific solution right? But it seems like this is a platform problem.

Comment 23

[Dragana Damjanovic [:dragana]]

From a chat with Gary, the conclusion is that binding is enough.


The most part of this functionality is already in necko.

Comment 24

[Gary Chen [:xeonchen]]

Thank you mcmanus and dragana, I'll try this approach.

Comment 25

[Gary Chen [:xeonchen]]

Just discussed with Jessica and Edgar, we think address binding might not work in per-destination case.

For more generalized use case, what if an App wants to connect to a specific host via
a specific route for a private service, and connect to other host via default route?

Comment 26

[Dragana Damjanovic [:dragana]]

Using marking is not really common. You probably have an exact use case, can you give me more details?

Comment 27

[Wan-Teh Chang]

Comment on attachment 8637809 [details] [diff] [review]
Part 1: enable SO_MARK in NSPR

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

Hi Gary,

If you need to set the SO_MARK socket option, you
can call PR_FileDesc2NativeHandle() to extract the
Unix file descriptor from a PRFileDesc, and call
setsockopt() on the Unix file descriptor.

Although PR_FileDesc2NativeHandle() is declared in
"private/pprio.h", it is an exported function and
it is OK to use the function under special
circumstances. (The "private" in "private/pprio.h"
should be interpreted as the "friend" interface in
C++.)

Comment 28

[Gary Chen [:xeonchen]]

For example, an app want to download data from several content providers, one of the providers has a special offer to download contents via a special APN for free.

So the app wants to send all packets through default gateway, except those to the specific destination.

Comment 29

[Dragana Damjanovic [:dragana]]

I do not know much about providers business model that will need such think, this one is not really appealing to me but if the offer content for free over one APN I could send all my traffic over that one and for the special one I just do not get charged, UI will be interesting, complex to make for this if it is offer to all app developers... and i have other opinions about this.

But I will ask Patrick what he thinks, he is the module owner.

Comment 30

[Patrick McManus [:mcmanus]]

the model being discussed here is a variation on zero rating - the mozilla viewpoint on that is here https://blog.mozilla.org/netpolicy/2015/05/05/mozilla-view-on-zero-rating/

Comment 31

[Patrick McManus [:mcmanus]]

(In reply to Gary Chen [:xeonchen] from comment #25)
> Just discussed with Jessica and Edgar, we think address binding might not
> work in per-destination case.
> 
> For more generalized use case, what if an App wants to connect to a specific
> host via
> a specific route for a private service, and connect to other host via
> default route?

I need some more help, because most of the examples I can think of can be satisfied with src address binding.

please lay out this example more. Use host and service addresses and a hypotehtical routing table. 

Are you suggesting the host has a single address, but two routes to the same destination that it needs to be able to choose between on a per app basis? Please make sure your example is consistent with our netpolicy. (comment 30)

even if we manage to climb over those bars, necko is platform code and should strive to support our tier one platforms. What is your plan here for things other than firefox os?

Comment 32

[Gary Chen [:xeonchen]]

The scenario of this feature would be like this:

Normal Apps ──┐
              ├── Default APN ──── Internet
Media App ────┘ 
         └─────── Media APN ────── Media Content Provider
                                   (media-provider.com)

The Media App uses media APN to download media contents from media-provider.com.
It still uses default APN to download data, like other apps.
In additional, only privileged app (i.e. Media App) can access media-provider.com.

I think address-binding would not satisfy the requirement.
If we decide to use mark-based solution, it will be platform-dependent,
and I'd suggest to land on the 2.2r branch only, that will be Firefox OS platform.

Since it might be a policy issue, I think we need someone to make the decision?

Comment 33

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Based on the our net policy (comment #30), it's likely that we are not going to land this feature on m-c. In addition, this bug is very specific to one operator requirement which is only happened on v2.2r and only Firefox OS is supported on that branch. Also, the general solution proposed in comment #29 doesn't fully covered the requirement based on comment #32. Two approaches I have in mind in order to move this bug forward:

1. review and land the mark-based solution only on v2.2r.
2. review the mark-based solution but not check-in, let partner to manually merge it to their local repo.

The decision between 1 and 2 should be based on how strong our net policy is, which I would like Wesley's help on getting this information.

Patrick how do you think?

Comment 34

[Patrick McManus [:mcmanus]]

If we don't do zero rating, I don't understand why we would do it on 2.2r

Anyhow, comment 32 is not clear. Are there 2 IP addresses on the client, or not? Could there be?

Comment 35

[Gary Chen [:xeonchen]]

AFAIK, yes, I think it will get an extra local IP after connection to a new APN type.

Comment 36

[Patrick McManus [:mcmanus]]

(In reply to Gary Chen [:xeonchen] from comment #35)
> AFAIK, yes, I think it will get an extra local IP after connection to a new
> APN type.

So that's the IP you bind() to.

Comment 37

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

Hi Sandip,
From product perspective, should we not implement this feature given it somewhat conflicts with zero-rating policy[1]? 

Per comment 34, seems the only use case (I can think of) for the feature is zero-rating. 
The mitigation plan is that we provide WIP patch w/o actual landing on 2.2r, and let OEM pick to their own repo.

[1] https://blog.mozilla.org/netpolicy/2015/05/05/mozilla-view-on-zero-rating/

Comment 38

[Sandip Kamat]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #37)
> Hi Sandip,
> From product perspective, should we not implement this feature given it
> somewhat conflicts with zero-rating policy[1]? 
> 
> Per comment 34, seems the only use case (I can think of) for the feature is
> zero-rating. 
> The mitigation plan is that we provide WIP patch w/o actual landing on 2.2r,
> and let OEM pick to their own repo.
> 
> [1]
> https://blog.mozilla.org/netpolicy/2015/05/05/mozilla-view-on-zero-rating/

per below, no dependency any more. Clearing NI

https://bugzilla.mozilla.org/show_bug.cgi?id=1142878#c25

Comment 39

[Gary Chen [:xeonchen]]

Close because we don't need this feature currently, but the patch can be referenced some day in the future.