net: added ng_udp implementation by haukepetersen · Pull Request #2430 · RIOT-OS/RIOT

haukepetersen · 2015-02-10T22:26:08Z

this PR depends on many other ng_ PRs, will detail them out once this PR gets in a state of being testable/reviewable. I will try to provide a first approach for a socket implementation to make this code better understandable.

While writing this code, I came across some issues we should think about:
~~1. how do we handle the checksum calculation when sending data and the src address is still unknown?~~
~~2. where do we get the source port from, if not using sockets but talking directly to UDP via NETAPI?~~

UPDATE: This implementation is now ready for testing

~~UPDATE 2: rebased on #2801~~

miri64 · 2015-02-11T09:22:00Z

sys/net/transport_layer/ng_udp/ng_udp.c

~~Since there should only be one UDP thread it might be worth thinking about making this public.~~

miri64 · 2015-02-11T09:28:47Z

how do we handle the checksum calculation when sending data and the src address is still unknown?

I'm still not 100% sure, but thinking about it I came to 2 different solutions:

as we already discussed: Implementing callbacks that are called after the IPv6 module found the corresponding source address to a given destination address.
a netreg_build_header(nettype_t type, void *src, size_t src_len, void* dest, size_t dest_len, size_t payload_len) where if src == NULL || src_len == 0 the source address will be selected by the corresponding type function. If this is called before the next header calculates its checksum, there is no problem, because everything needed for the Internet checksum is there

where do we get the source port from, if not using sockets but talking directly to UDP via NETAPI?

I more or less assumed the user application is setting this in the UDP header, also using some netreg_build_header-function.

miri64 · 2015-02-11T09:31:49Z

sys/net/transport_layer/ng_udp/ng_udp.c

Since port 0 is reserved, we maybe should use uint32_t as demux type and go to 0x10000 for ALL.

Source: http://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml

I think exactly because it is reserved, we can use the 0 internally for "everyone who is interested in UDP general"...
And the pkt->type in the line above would be IP...

And the pkt->type in the line above would be IP...

Ah, sure… but in context of #2430 (comment): can we be sure of this?

I think exactly because it is reserved, we can use the 0 internally for "everyone who is interested in UDP general"...

Aren't we then exceptionally vulnerable against port 0 attacks?

LudwigKnuepfer · 2015-02-11T09:35:23Z

Usually the network layer decides the source port. As the network layer in question is IP, it should be the decision of the IP module how to enable this (I suggest a uint16_t ip_get_free_source_port() function).

Usually the transport layer decides the source port. As the transport layer in question is UDP, it should be the decision of the UDP module how to enable this (I suggest a uint16_t udp_get_free_source_port() function).

miri64 · 2015-02-11T10:03:42Z

Why does the network layer does this oO?

LudwigKnuepfer · 2015-02-11T11:09:40Z

Because I'm stupid ;) (comment updated)

miri64 · 2015-02-11T11:31:45Z

sys/net/transport_layer/ng_udp/ng_udp.c

Shouldn't the next header be set here somewhere? At least that's what the beginning of this function implies, where you start searching for the UDP header and stop if you don't find it. If yes, how do we get the length and the type of the next header?
Or do we pre-parse the packet in the link-layer/driver? If yes: why? The link-layer/driver then has to have knowledge of all header types, or it must be stored somewhere so the driver can handle it. And I'm wondering why we do it so complicated.

miri64 · 2015-02-11T11:52:16Z

Usually the transport layer decides the source port. As the transport layer in question is UDP, it should be the decision of the UDP module how to enable this (I suggest a uint16_t udp_get_free_source_port() function).

Can the UDP port change in "one transmission"? (since one transmission in UDP is normally exactly one packet, I think I know the answer :D)

LudwigKnuepfer · 2015-02-11T12:31:37Z

What do you mean with "one transmission"? At least for IPv4 you can have several fragmented IP packets for the same UDP datagram.

miri64 · 2015-02-11T12:33:14Z

I'm not sure either. I guess I meant a stream of data (yes I know there are streams in UDP)… Was more or less a brainfart.

cgundogan · 2015-02-11T13:36:10Z

Can the UDP port change in "one transmission"? (since one transmission in UDP is normally exactly one packet, I think I know the answer :D)

Since the Src[Port:IP]-Dst[Port:IP] tuple is an End-To-End connection, the port cannot change for "one transmission". However, after this transmission, a new Src[Port:IP]-Dst[Port:IP] tuple can be generated aka a new Socket can be created to serve as the new communication endpoint :)

cgundogan · 2015-02-11T13:37:35Z

What do you mean with "one transmission"? At least for IPv4 you can have several fragmented IP packets for the same UDP datagram.

Since the fragmentation is below UDP, the port is fixed

LudwigKnuepfer · 2015-02-11T14:03:38Z

Sure, but can it be reused for a new "connection" to the same destination in parallel?

cgundogan · 2015-02-11T14:22:00Z

Sure, but can it be reused for a new "connection" to the same destination in parallel?

No, once fixed to a socket a port cannot be reused if the respective socket was not closed. And a good implementation should not take the port immediatly after closing, but choose a random one thereafter

LudwigKnuepfer · 2015-02-11T14:26:41Z

That is why we need to keep track of the (source) ports in use. I thought the respective layer was a good point to do so.

cgundogan · 2015-02-11T14:32:55Z

That is why we need to keep track of the source ports in use. I thought the respective layer was a good point to do so.

👍

OlegHahm · 2015-02-13T11:14:16Z

how do we handle the checksum calculation when sending data and the src address is still unknown?

I recently thought about this and came to the conclusion that it seems actually the most sensible to me to do the upper layer checksum calculation directly in IP itself. IP has to know about the upper layer anyway and it knows the correct source address, so there's no advantage of doing it in the upper layer (and having a probability that is has to be done again in IP).

miri64 · 2015-02-13T12:00:46Z

I recently thought about this and came to the conclusion that it seems actually the most sensible to me to do the upper layer checksum calculation directly in IP itself.

So we do the callback solution. Will find something in my IPv6 implementation :-)

OlegHahm · 2015-02-13T13:41:13Z

Actually, I was not talking about callbacks. Seems overkill to me, if the same function is called for every upper layer anyway.

miri64 · 2015-03-22T15:47:19Z

sys/include/net/ng_udp.h

I always think these macros are kind of redundant. The same value can be retrieved with sizeof(ng_udp_hdr_t) (and it is more portable).

I'm staying with my redundancy argument. :D

miri64 · 2015-04-24T14:37:46Z

ACK if travis is happy.

haukepetersen · 2015-04-24T16:27:53Z

lets merge this as is (otherwise it gets a neverending story) and then feel free to PR that change. Sound reasonable?

miri64 · 2015-04-24T16:38:44Z

👍

sgso · 2015-04-24T16:39:53Z

Sure, I didn't intend to stall anything. I have been using ng_udp for about a week now in mostly this form and it worked very well.

miri64 · 2015-04-24T17:46:57Z

sys/include/net/ng_udp.h

Doxygen has a problem finding the reference to NG_NETTYPE_UDP, since MODULE_NG_UDP in its scope is not defined.

miri64 · 2015-04-24T17:49:36Z

ERROR: The following new files generate doxygen warnings:
sys/include/net/ng_udp.h

Solutions for that:

miri64 · 2015-04-25T19:11:48Z

sys/include/net/ng_udp.h

#ifdef __cplusplus } #endif

missing

haukepetersen · 2015-04-26T11:00:45Z

fixed issues (doxygen, indention, cplusplus closing bracket, param checking when sending) and rebased.

miri64 · 2015-04-26T11:25:43Z

Re-ACK

miri64 · 2015-04-26T11:26:18Z

(please SQUASH)

haukepetersen · 2015-04-26T11:53:14Z

squashed

miri64 · 2015-04-26T13:31:08Z

sys/net/transport_layer/ng_udp/ng_udp.c

cppcheck is complaining about len not being used for the #ifndef MODULE_NG_IPV6 build-path.

sys/net/transport_layer/ng_udp/ng_udp.c:71: style (unreadVariable): Variable 'len' is assigned a value that is never used.

we can fix this after merge, or you add a (void)len to this case and squash.

haukepetersen · 2015-04-26T19:53:34Z

now cppcheck shouldn't complain anymore

miri64 · 2015-04-26T21:06:38Z

It doesn't => finally go!

net: added ng_udp implementation

haukepetersen · 2015-04-27T09:09:04Z

this was an easy one :-)

haukepetersen added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: network Area: Networking NSTF State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 10, 2015

haukepetersen added this to the Network Stack Task Force milestone Feb 10, 2015

haukepetersen assigned miri64 Feb 10, 2015

miri64 mentioned this pull request Feb 10, 2015

Network stack refactoring: TODO list #2278

Closed

36 tasks

haukepetersen force-pushed the ng_udp branch from 346a286 to 62fe254 Compare February 10, 2015 22:30

miri64 reviewed Feb 11, 2015
View reviewed changes

miri64 reviewed Mar 22, 2015
View reviewed changes

haukepetersen removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Apr 24, 2015

miri64 reviewed Apr 24, 2015
View reviewed changes

miri64 reviewed Apr 25, 2015
View reviewed changes

sys/include/net/ng_udp.h

Copy link
Copy Markdown

Member

miri64 Apr 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#ifdef __cplusplus } #endif

missing

miri64 mentioned this pull request Apr 26, 2015

net: initial import of the ZEP protocol #2627

Merged

haukepetersen force-pushed the ng_udp branch from d5b48a1 to 11d4639 Compare April 26, 2015 10:59

haukepetersen force-pushed the ng_udp branch from 11d4639 to 6262200 Compare April 26, 2015 11:53

haukepetersen mentioned this pull request Apr 26, 2015

sys/shell: added UDP shell commands #2720

Closed

miri64 reviewed Apr 26, 2015
View reviewed changes

haukepetersen added 3 commits April 26, 2015 21:53

net: added NG_UDP implementation

b1d8752

sys/auto_init: added initialization of ng_udp

1441e2f

net/ng_netreg: added include of ng_udp.h

e66e1dc

haukepetersen force-pushed the ng_udp branch from 6262200 to e66e1dc Compare April 26, 2015 19:53

miri64 added a commit that referenced this pull request Apr 26, 2015

Merge pull request #2430 from haukepetersen/ng_udp

07de380

net: added ng_udp implementation

miri64 merged commit 07de380 into RIOT-OS:master Apr 26, 2015

haukepetersen deleted the ng_udp branch April 27, 2015 09:09

Conversation

haukepetersen commented Feb 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Feb 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudwigKnuepfer commented Feb 11, 2015

Uh oh!

miri64 commented Feb 11, 2015

Uh oh!

LudwigKnuepfer commented Feb 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Feb 11, 2015

Uh oh!

LudwigKnuepfer commented Feb 11, 2015

Uh oh!

miri64 commented Feb 11, 2015

Uh oh!

cgundogan commented Feb 11, 2015

Uh oh!

cgundogan commented Feb 11, 2015

Uh oh!

LudwigKnuepfer commented Feb 11, 2015

Uh oh!

cgundogan commented Feb 11, 2015

Uh oh!

LudwigKnuepfer commented Feb 11, 2015

Uh oh!

cgundogan commented Feb 11, 2015

Uh oh!

OlegHahm commented Feb 13, 2015

Uh oh!

miri64 commented Feb 13, 2015

Uh oh!

OlegHahm commented Feb 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Apr 24, 2015

Uh oh!

haukepetersen commented Apr 24, 2015

Uh oh!

miri64 commented Apr 24, 2015

Uh oh!

sgso commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Apr 26, 2015

Uh oh!

miri64 commented Apr 26, 2015

Uh oh!

miri64 commented Apr 26, 2015

Uh oh!

haukepetersen commented Apr 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Apr 26, 2015

Uh oh!

miri64 commented Apr 26, 2015