Lora: Make automatic uplink message configurable #6628

kivaisan · 2018-04-13T07:30:21Z

Description

Currently lora stack will automatically send an empty uplink message to lora gateway in case of:

Node received message with pending bit set.
Node received MAC command which requires instant response (sticky MAC command)
Node received confirmed message in class C mode

This commit makes this configurable via config item

    "automatic-uplink-message": {
        "help": "In case of pending bit, class c confirmed message or sticky MAC command, stack will automatically send empty uplink message",
        "value": true
    }

Default value is true. If sending an empty message fails, stack will send event AUTOMATIC_UPLINK_ERROR application.

If automatic uplink sending is disabled, stack will send application UPLINK_REQUIRED -event to indicate
application should issue a new uplink to gateway as soon as possible.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

kivaisan · 2018-04-13T07:31:13Z

@hasnainvirk @AnttiKauppila @kjbracey-arm please review.

hasnainvirk

Looks really good to me. Only a minor textual change in the help field of json is required.

hasnainvirk · 2018-04-13T07:35:39Z

features/lorawan/mbed_lib.json

@@ -60,6 +60,10 @@
        "lbt-on": {
            "help": "Enables/disables LBT. NOTE: [This feature is not yet integrated].",
            "value": false
+        },
+        "automatic-uplink-message": {
+            "help": "In case of pending bit, class c confirmed message or sticky MAC command, stack will automatically send empty uplink message",


Reading the code I can see that even in Class A an automatic uplink can be generated. The help field should therefore be changed.

kivaisan · 2018-04-16T05:47:18Z

@hasnainvirk Please review again. I removed the possible scenarios from help completely and made the description more generic.

hasnainvirk · 2018-04-16T07:58:34Z

features/lorawan/LoRaWANStack.cpp

@@ -61,7 +61,7 @@ using namespace events;
 bool LoRaWANStack::is_port_valid(uint8_t port)
 {
    //Application should not use reserved and illegal port numbers.
-    if (port >= 224 || port == 0) {


Isn't it like opening a backdoor for application to invoke a network level function without the stack noticing it ?
Port 0 is like a control channel, application should not have access to it. What do you think @kivaisan @janjongboom ?

Actually Semtech has removed the same check already some time ago from their stack. And if someone would like to use port 0 to something suspicious, what would prevent them from removing this check anyway from their source.

I'd say the only reason to bar it is if it would fundamentally break the stack in some way. As long as the stack doesn't get fundamentally confused by port-0 messages it didn't generate, should be fine.

This is quite analogous to ICMP in IP, where the stack normally generates and processes ICMP messages, but applications can also generate and process them. (eg ping).

By symmetry, if an app can send port 0 messages, it should also be able to receive them - does this ensure that?

On "backdoor", if this was Unix or something, conceivably port 0 might be limited to privileged applications. Eg ping does have to run setuid root because of that.

But in an OS without privilege levels, anyone can send raw IP/ICMP messages, and that makes sense here.

Traffic on port 0 is intended for network stacks, i.e., they act upon requests made by the network on port 0 and they respond to the network against those requests on port 0. That's why we had barred any user traffic on it previously. I am not sure what useful thing a user can retrieve by receiving at port 0.

Things which spring to mind are traffic monitor/logging applications, and applications that want to send some sort of higher-level network command they know but the stack core doesn't. Eg some sort of vendor extension in the command space.

On a higher-level API I'd be inclined to have some sort of gating - eg SOCK_RAW allowing any port, but SOCK_DGRAM only using proper user ones. This is a pretty low-level API though.

Also, in our email conversation from yesterday night it was stated that multicast / data frag should apparently be a separate library. You cannot do that if you can't do things on port 224/225.

We currently implement lorawan 1.0.2 which clearly states 224 is reserved for compliance testing and 225-255 for future standardized application extensions. So I think our current check (allowing only ports 0-223 for application) is correct for the spec version we support at the moment. Of course future spec version can/and probably will change this, so we also change the check accordingly.

@kivaisan Yes, application extensions. The Multicast and data frag spec are extensions on top of LoRaWAN 1.02 and use these port numbers. So if you do it like this no-one can implement these extensions.

It's not the network layer's job to act as police for whether applications are using known valid port IDs. Its default behaviour should always be to just pass stuff through unless it's actually invalid, or explicitly defined to be "not for applications". From the specs, only port 0 is "special MAC internal" - everything else is application layer, even if reserved for future standards, and even the port 224 test protocol is explicitly stated as being an application protocol.

Given the current "single-user" API, it would make sense to give it all incoming traffic except for port 0, and let it send any outgoing traffic except for port 0.

I'm reminded of a horrible PCI hard drive controller I once encountered that stopped us using large discs - it had built-in knowledge of which ATA commands were DMA commands, and would only let you request DMA with those known commands. Which meant when extended the filing system to use LBA48 commands to access large discs, we found we couldn't do DMA transfers on that machine - the chip refused to start DMA because we were using a new command it hadn't heard of. If it had just passed through commands to the drive and started DMA when requested, rather than trying to put checks in, it would have worked fine.

If you ever do want the network layer to function as police for application ports or whatever - as a firewall - it's still not the network layer's job to have built-in knowledge of application ports and protocols. It should provide an API for filters to be set up by system management code.

kivaisan · 2018-04-17T12:56:21Z

Changed is_port_valid to only block port 0 from application. Please review again.

Currently lora stack will automatically send an empty uplink message to lora gateway in case of: - Node received message with pending bit set. - Node received MAC command which requires instant response (sticky MAC command) - Node received confirmed message in class C mode This commit makes this configurable via config item "automatic-uplink-message": { "help": "In case of pending bit, class c confirmed message or sticky MAC command, stack will automatically send empty uplink message", "value": true } Default value is true. If sending an empty message fails, stack will send event AUTOMATIC_UPLINK_ERROR application. If automatic uplink sending is disabled, stack will send application UPLINK_REQUIRED -event to indicate application should issue a new uplink to gateway as soon as possible.

kivaisan · 2018-04-18T06:50:44Z

Rebased against the latest mbed-os.

0xc0170 · 2018-04-18T08:13:07Z

/morph build

mbed-ci · 2018-04-18T09:18:35Z

Build : SUCCESS

Build number : 1782
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6628/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

0xc0170 · 2018-04-18T11:45:22Z

/morph uvisor-test

mbed-ci · 2018-04-18T14:22:57Z

Exporter Build : SUCCESS

Build number : 1426
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6628/

mbed-ci · 2018-04-18T16:04:43Z

Test : SUCCESS

Build number : 1592
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6628/1592

hasnainvirk approved these changes Apr 13, 2018

View reviewed changes

0xc0170 previously approved these changes Apr 13, 2018

View reviewed changes

0xc0170 added the needs: work label Apr 13, 2018

kivaisan dismissed 0xc0170’s stale review via 64c961c April 16, 2018 05:45

0xc0170 added needs: review and removed needs: work labels Apr 16, 2018

hasnainvirk reviewed Apr 16, 2018

View reviewed changes

hasnainvirk approved these changes Apr 17, 2018

View reviewed changes

kjbracey approved these changes Apr 18, 2018

View reviewed changes

0xc0170 requested a review from AnttiKauppila April 18, 2018 08:12

0xc0170 added ready for merge release-version: 5.9.0-rc1 and removed needs: review labels Apr 19, 2018

0xc0170 merged commit 13913c7 into ARMmbed:master Apr 19, 2018

0xc0170 removed the ready for merge label Apr 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lora: Make automatic uplink message configurable #6628

Lora: Make automatic uplink message configurable #6628

kivaisan commented Apr 13, 2018

kivaisan commented Apr 13, 2018

hasnainvirk left a comment

hasnainvirk Apr 13, 2018

kivaisan commented Apr 16, 2018

hasnainvirk Apr 16, 2018

kivaisan Apr 16, 2018

kjbracey Apr 16, 2018

kjbracey Apr 16, 2018

hasnainvirk Apr 16, 2018

kjbracey Apr 16, 2018

janjongboom Apr 17, 2018

kivaisan Apr 17, 2018

janjongboom Apr 17, 2018

kjbracey Apr 17, 2018

kivaisan commented Apr 17, 2018

kivaisan commented Apr 18, 2018

0xc0170 commented Apr 18, 2018

mbed-ci commented Apr 18, 2018

0xc0170 commented Apr 18, 2018

mbed-ci commented Apr 18, 2018

mbed-ci commented Apr 18, 2018

Lora: Make automatic uplink message configurable #6628

Lora: Make automatic uplink message configurable #6628

Conversation

kivaisan commented Apr 13, 2018

Description

Pull request type

kivaisan commented Apr 13, 2018

hasnainvirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kivaisan commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kivaisan commented Apr 17, 2018

kivaisan commented Apr 18, 2018

0xc0170 commented Apr 18, 2018

mbed-ci commented Apr 18, 2018

Triggering tests

0xc0170 commented Apr 18, 2018

mbed-ci commented Apr 18, 2018

mbed-ci commented Apr 18, 2018