Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gnrc_sock_udp: add udp portrange #6212

Merged
merged 4 commits into from
Jan 18, 2017
Merged

gnrc_sock_udp: add udp portrange #6212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c5f07b
gnrc_sock: add dynamic port range
smlng Dec 15, 2016
e2cf221
gnrc_sock_udp: get source port from dynmic port range
smlng Dec 15, 2016
a9cb29c
gnrc_sock_udp: optimized remote ep handling
smlng Dec 15, 2016
e28d14f
gnrc, sock_udp: correct includes
smlng Jan 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions sys/net/gnrc/sock/include/gnrc_sock_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "net/af.h"
#include "net/gnrc.h"
#include "net/gnrc/netreg.h"
#include "net/iana/portrange.h"
#include "net/sock/ip.h"

#include "sock_types.h"
Expand All @@ -36,6 +37,22 @@
extern "C" {
#endif

/**
* @brief Default port range for dynamic source port allocation
*/
#define GNRC_SOCK_DYN_PORTRANGE_MIN (IANA_DYNAMIC_PORTRANGE_MIN)
#define GNRC_SOCK_DYN_PORTRANGE_MAX (IANA_DYNAMIC_PORTRANGE_MAX)
#define GNRC_SOCK_DYN_PORTRANGE_NUM (GNRC_SOCK_DYN_PORTRANGE_MAX - GNRC_SOCK_DYN_PORTRANGE_MIN + 1)
#define GNRC_SOCK_DYN_PORTRANGE_ERR (0)

/**
* @brief Offset for next dynamic port
*
* Currently set to a static (prime) offset, but could be random, too
* see https://tools.ietf.org/html/rfc6056#section-3.3.3
*/
#define GNRC_SOCK_DYN_PORTRANGE_OFF (17U)
Copy link
Member

Choose a reason for hiding this comment

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

23 is a nice prime, too. j/k

Does it have to be a prime? Couldn't find that in the RFC. But anyway - if no prime is required, 17 should be as good as any other number (except for 42).

Copy link
Member Author

Choose a reason for hiding this comment

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

the RFC actually says it has to be a random component, see code comment. However, I felt it might not be good to have the overhead of random by default in RIOT

Copy link
Member

Choose a reason for hiding this comment

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

Hm, well, security comes mostly at the price of some overhead. However, I understand and I agree your concern. I think your solution here is better than the current state, but I would like to have a separate discussion about how RFC6056 compliant we want to be. Would you mind opening an issue or should I?

Copy link
Member Author

Choose a reason for hiding this comment

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

per issue or via mailing list?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the mailing list would be more appropriate indeed.

Copy link
Member Author

@smlng smlng Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding the prime value for the offset: in general (and 17 in specific) has the merits of not being a devisor divisor of GNRC_SOCK_DYN_PORTRANGE_NUM -> (65535 - 49152 +1)/17 = 963.765, that way even if many (many, many) dynamic ports a generated over time, collisions are less likely even on roll over


/**
* @brief Internal helper functions for GNRC
* @internal
Expand Down
157 changes: 91 additions & 66 deletions sys/net/gnrc/sock/udp/gnrc_sock_udp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,66 @@
#include "net/gnrc/udp.h"
#include "net/sock/udp.h"
#include "net/udp.h"
#include "random.h"

#include "gnrc_sock_internal.h"

#ifdef MODULE_GNRC_SOCK_CHECK_REUSE
static sock_udp_t *_udp_socks = NULL;
#endif

static uint16_t _dyn_port_next = 0;

/**
* @brief Checks if a given UDP port is already used by another sock
*/
static bool _dyn_port_used(uint16_t port)
{
#ifdef MODULE_GNRC_SOCK_CHECK_REUSE
for (sock_udp_t *ptr = _udp_socks; ptr != NULL;
ptr = (sock_udp_t *)ptr->reg.next) {
bool spec_addr = false;
Copy link
Member

Choose a reason for hiding this comment

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

spec_addr? Unspecified/specified might be the wrong terminology here, since it is more like any in sockets. As such me and @OlegHahm are a little bit confused about this whole section, since it may well be, that the sock is bound to any address, but the port is set. So it makes no sense to skip entries with all-zero addresses.

Copy link
Member Author

Choose a reason for hiding this comment

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

this wasn't introduced by me, this is copy-paste from the original code. I didn't (want to) tamper with that 😄

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I see. In this case I would propose to leave it as is and we open a separate PR to fix this.

for (unsigned i = 0; i < sizeof(ptr->local.addr); i++) {
const uint8_t *const p = (uint8_t *)&ptr->local.addr;
if (p[i] != 0) {
spec_addr = true;
}
}
if (spec_addr) {
continue;
}
if (ptr->local.port == port) {
/* port already in use by another sock */
return true;
}
}
#else
(void) port;
#endif /* MODULE_GNRC_SOCK_CHECK_REUSE */
return false;
}

/**
* @brief returns a UDP port, and checks for reuse if required
*
* complies to RFC 6056, see https://tools.ietf.org/html/rfc6056#section-3.3.3
*/
static uint16_t _get_dyn_port(sock_udp_t *sock)
{
uint16_t port;
unsigned count = GNRC_SOCK_DYN_PORTRANGE_NUM;
do {
port = GNRC_SOCK_DYN_PORTRANGE_MIN +
(_dyn_port_next * GNRC_SOCK_DYN_PORTRANGE_OFF) % GNRC_SOCK_DYN_PORTRANGE_NUM;
_dyn_port_next++;
if ((sock == NULL) || (sock->flags & SOCK_FLAGS_REUSE_EP) ||
!_dyn_port_used(port)) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be _dyn_port_used() checked in any case, even if sock == NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

@OlegHahm it is, because that's an or expression, or what do you suggest/mean?

Copy link
Member

Choose a reason for hiding this comment

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

I guess what @OlegHahm means is, if (sock == NULL) lazy-evaluation kicks in and it isn't evaluated ;-).

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, oversaw that one 😞 However, if sock == NULL its a onetime (fire and forget) usage, i.e. for sending, hence, no check required as no listing will happen. So checking the port is not needed, or do I oversee something (again)?

return port;
}
--count;
} while (count > 0);
return GNRC_SOCK_DYN_PORTRANGE_ERR;
}

int sock_udp_create(sock_udp_t *sock, const sock_udp_ep_t *local,
const sock_udp_ep_t *remote, uint16_t flags)
{
Expand Down Expand Up @@ -76,8 +128,7 @@ int sock_udp_create(sock_udp_t *sock, const sock_udp_ep_t *local,
}
if (local != NULL) {
/* listen only with local given */
gnrc_sock_create(&sock->reg, GNRC_NETTYPE_UDP,
local->port);
gnrc_sock_create(&sock->reg, GNRC_NETTYPE_UDP, local->port);
}
sock->flags = flags;
return 0;
Expand Down Expand Up @@ -166,23 +217,29 @@ ssize_t sock_udp_send(sock_udp_t *sock, const void *data, size_t len,
gnrc_pktsnip_t *payload, *pkt;
uint16_t src_port = 0, dst_port;
sock_ip_ep_t local;
sock_ip_ep_t rem;
sock_ip_ep_t *rem;

assert((sock != NULL) || (remote != NULL));
Copy link
Member

Choose a reason for hiding this comment

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

Huh?

Copy link
Member Author

Choose a reason for hiding this comment

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

ups, delete by accident

Copy link
Member Author

Choose a reason for hiding this comment

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

on the other hand, I thought the agenda is: don't comment the obvious?

Copy link
Member

Choose a reason for hiding this comment

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

Hard to say what's obvious... I wouldn't say it's obvious that the code below is an implicit binding

Copy link
Member Author

Choose a reason for hiding this comment

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

so I shall re-add?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @miri64. If in doubt: document it! (The change would be unrelated anyway.)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok will re-add

assert((len == 0) || (data != NULL)); /* (len != 0) => (data != NULL) */
if ((remote != NULL) && (sock != NULL) &&
(sock->local.netif != SOCK_ADDR_ANY_NETIF) &&
(remote->netif != SOCK_ADDR_ANY_NETIF) &&
(sock->local.netif != remote->netif)) {
return -EINVAL;
}
if ((remote != NULL) && ((remote->port == 0) ||
gnrc_ep_addr_any((const sock_ip_ep_t *)remote))) {
return -EINVAL;

if (remote != NULL) {
if (remote->port == 0) {
return -EINVAL;
}
else if (gnrc_ep_addr_any((const sock_ip_ep_t *)remote)) {
return -EINVAL;
}
else if (gnrc_af_not_supported(remote->family)) {
return -EAFNOSUPPORT;
}
else if ((sock != NULL) &&
(sock->local.netif != SOCK_ADDR_ANY_NETIF) &&
(remote->netif != SOCK_ADDR_ANY_NETIF) &&
(sock->local.netif != remote->netif)) {
return -EINVAL;
}
}
if ((remote == NULL) &&
/* sock can't be NULL as per assertion above */
(sock->remote.family == AF_UNSPEC)) {
else if (sock->remote.family == AF_UNSPEC) {
Copy link
Member

Choose a reason for hiding this comment

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

The static code analyzer isn't complaining here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, because of the assert((sock != NULL) || (remote != NULL)), i.e., this else if only fires if remote == NULL and per assert then sock != NULL.

Copy link
Member

Choose a reason for hiding this comment

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

How are this and the changes above related to this PR?

return -ENOTCONN;
}
/* compiler evaluates lazily so this isn't a redundundant check and cppcheck
Expand All @@ -191,33 +248,10 @@ ssize_t sock_udp_send(sock_udp_t *sock, const void *data, size_t len,
/* cppcheck-suppress nullPointer */
if ((sock == NULL) || (sock->local.family == AF_UNSPEC)) {
/* no sock or sock currently unbound */
while (src_port == 0) {
src_port = (uint16_t)(random_uint32() & UINT16_MAX);
#ifdef MODULE_GNRC_SOCK_CHECK_REUSE
if ((sock == NULL) || !(sock->flags & SOCK_FLAGS_REUSE_EP)) {
/* check if port already registered somewhere */
for (sock_udp_t *ptr = _udp_socks; ptr != NULL;
ptr = (sock_udp_t *)ptr->reg.next) {
bool spec_addr = false;
for (unsigned i = 0; i < sizeof(ptr->local.addr); i++) {
const uint8_t *const p = (uint8_t *)&ptr->local.addr;
if (p[i] != 0) {
spec_addr = true;
}
}
if (spec_addr) {
continue;
}
if (ptr->local.port == src_port) {
/* we already have one of this port registered
* => generate a new one */
src_port = 0;
}
}
}
#endif
}
memset(&local, 0, sizeof(local));
if ((src_port = _get_dyn_port(sock)) == GNRC_SOCK_DYN_PORTRANGE_ERR) {
return -EINVAL;
}
if (sock != NULL) {
/* bind sock object implicitly */
sock->local.port = src_port;
Expand All @@ -227,44 +261,35 @@ ssize_t sock_udp_send(sock_udp_t *sock, const void *data, size_t len,
else {
sock->local.family = remote->family;
}
gnrc_sock_create(&sock->reg, GNRC_NETTYPE_UDP, src_port);
#ifdef MODULE_GNRC_SOCK_CHECK_REUSE
/* prepend to current socks */
sock->reg.next = (gnrc_sock_reg_t *)_udp_socks;
_udp_socks = sock;
#endif
gnrc_sock_create(&sock->reg, GNRC_NETTYPE_UDP, src_port);
#endif /* MODULE_GNRC_SOCK_CHECK_REUSE */
}
}
else {
src_port = sock->local.port;
memcpy(&local, &sock->local, sizeof(local));
}
/* sock can't be NULL at this point */
if (remote == NULL) {
/* sock can't be NULL at this point */
memcpy(&rem, &sock->remote, sizeof(rem));
rem = (sock_ip_ep_t *)&sock->remote;
dst_port = sock->remote.port;
}
else {
memcpy(&rem, remote, sizeof(rem));
rem = (sock_ip_ep_t *)remote;
Copy link
Member

Choose a reason for hiding this comment

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

Again this seems to be unrelated to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

its (just) an optimization I found reasonable to safe the memcpy, making an extra PR for this seems to much overhead

Copy link
Member

Choose a reason for hiding this comment

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

At least it should go into a separate commit. And for my taste: more, but smaller PRs are less overhead.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

dst_port = remote->port;
}
if ((remote != NULL) && (remote->family == AF_UNSPEC) &&
(sock != NULL) && (sock->remote.family != AF_UNSPEC)) {
/* remote was set on create so take its family */
rem.family = sock->remote.family;
}
else if ((remote != NULL) && gnrc_af_not_supported(remote->family)) {
return -EAFNOSUPPORT;
}
else if ((local.family == AF_UNSPEC) && (rem.family != AF_UNSPEC)) {
/* local was set to 0 above */
local.family = rem.family;
/* check for matching address families in local and remote */
if (local.family == AF_UNSPEC) {
local.family = rem->family;
}
else if ((local.family != AF_UNSPEC) && (rem.family == AF_UNSPEC)) {
/* local was given on create, but remote family wasn't given by user and
* there was no remote given on create, take from local */
rem.family = local.family;
else if (local.family != rem->family) {
return -EINVAL;
}
/* generate payload and header snips */
payload = gnrc_pktbuf_add(NULL, (void *)data, len, GNRC_NETTYPE_UNDEF);
if (payload == NULL) {
return -ENOMEM;
Expand All @@ -274,11 +299,11 @@ ssize_t sock_udp_send(sock_udp_t *sock, const void *data, size_t len,
gnrc_pktbuf_release(payload);
return -ENOMEM;
}
res = gnrc_sock_send(pkt, &local, &rem, PROTNUM_UDP);
if (res <= 0) {
return res;
res = gnrc_sock_send(pkt, &local, rem, PROTNUM_UDP);
if (res > 0) {
res -= sizeof(udp_hdr_t);
}
return res - sizeof(udp_hdr_t);
return res;
}

/** @} */