Skip to content

dhcpv6_client: refactor to use event_timeout for non-sock timeouts #16668

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

Merged
miri64 merged 7 commits into from
Jul 23, 2021
Merged

dhcpv6_client: refactor to use event_timeout for non-sock timeouts #16668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb577c9
dhcpv6_client: move timer setting to their own functions
miri64 Jul 20, 2021
02c0581
timex: introduce MS_PER_CS
miri64 Jul 21, 2021
d1613da
dhcpv6_client: use milliseconds rather than microseconds as base unit
miri64 Jul 21, 2021
ed9a682
dhcpv6_client: set timer by unit rather than integer width
miri64 Jul 21, 2021
805952c
dhcpv6_client: use event_timeout instead of xtimer
miri64 Jul 21, 2021
b5d9f78
dhcpv6_client: port timeout mechanism to event_timeout
miri64 Jul 21, 2021
6493241
dhcpv6_client: add ztimer backend support
miri64 Jul 21, 2021
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
6 changes: 5 additions & 1 deletion sys/Makefile.dep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,12 @@ endif

ifneq (,$(filter dhcpv6_client,$(USEMODULE)))
USEMODULE += event
USEMODULE += event_timeout
ifneq (,$(filter ztimer,$(USEMODULE)))
USEMODULE += event_timeout_ztimer
USEMODULE += ztimer_msec ztimer_sec
endif
USEMODULE += random
USEMODULE += xtimer
ifneq (,$(filter sock_dns,$(USEMODULE)))
USEMODULE += dhcpv6_client_dns
endif
Expand Down
5 changes: 4 additions & 1 deletion sys/include/net/dhcpv6/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,11 @@ void dhcpv6_client_init(event_queue_t *event_queue, uint16_t netif);
/**
* @brief Let the server start listening
*
* @pre @ref dhcpv6_client_init() was called (i.e. the internal event queue of
* he client was set).
*
* This needs to be called *after* all desired [configuration functions]
* (@ref net_dhcpv6_client_conf) where called.
* (@ref net_dhcpv6_client_conf) and @ref dhcpv6_client_init() were called.
*/
void dhcpv6_client_start(void);

Expand Down
5 changes: 5 additions & 0 deletions sys/include/timex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ extern "C" {
*/
#define US_PER_CS (10000U)

/**
* @brief The number of milliseconds per centisecond
*/
#define MS_PER_CS (10U)

/**
* @brief The number of nanoseconds per microsecond
*/
Expand Down
154 changes: 94 additions & 60 deletions sys/net/application_layer/dhcpv6/client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,20 @@
#include <stdbool.h>

#include "event.h"
#include "event/timeout.h"
#include "log.h"
#include "kernel_defines.h"
#include "net/dhcpv6/client.h"
#include "net/dhcpv6.h"
#include "net/sock/udp.h"
#include "random.h"
#include "timex.h"
#if IS_USED(MODULE_ZTIMER)
#include "ztimer.h"
#else
#include "xtimer.h"
#include "xtimer/implementation.h"
#endif

#define ENABLE_DEBUG 0
#include "debug.h"
Expand Down Expand Up @@ -72,7 +77,7 @@ static uint8_t best_adv[DHCPV6_CLIENT_BUFLEN];
static uint8_t duid[DHCPV6_CLIENT_DUID_LEN];
static pfx_lease_t pfx_leases[CONFIG_DHCPV6_CLIENT_PFX_LEASE_MAX];
static server_t server;
static xtimer_t timer, rebind_timer;
static event_timeout_t solicit_renew_timeout, rebind_timeout;
static event_queue_t *event_queue;
static sock_udp_t sock;
static sock_udp_ep_t local = { .family = AF_INET6, .port = DHCPV6_CLIENT_PORT };
Expand All @@ -88,12 +93,18 @@ static uint8_t duid_len = sizeof(dhcpv6_duid_l2_t);

static const char mud_url[] = CONFIG_DHCPV6_CLIENT_MUD_URL;

static void _post_solicit_servers(void *args);
static void _post_solicit_servers(void);
static void _solicit_servers(event_t *event);
static void _request(event_t *event);
static void _renew(event_t *event);
static void _rebind(event_t *event);

static void _set_event_timeout_ms(event_timeout_t *timeout, event_t *event,
uint32_t delay_ms);
static void _set_event_timeout_sec(event_timeout_t *timeout, event_t *event,
uint32_t delay_sec);
static void _clear_event_timeout(event_timeout_t *timeout);

static event_t solicit_servers = { .handler = _solicit_servers };
static event_t request = { .handler = _request };
static event_t renew = { .handler = _renew };
Expand Down Expand Up @@ -141,14 +152,14 @@ void dhcpv6_client_init(event_queue_t *eq, uint16_t netif)

void dhcpv6_client_start(void)
{
uint32_t delay = random_uint32_range(0, DHCPV6_SOL_MAX_DELAY * US_PER_SEC);

duid_len = dhcpv6_client_get_duid_l2(local.netif,
(dhcpv6_duid_l2_t *)&duid);
assert(event_queue != NULL);
if (duid_len > 0) {
uint32_t delay = random_uint32_range(0, DHCPV6_SOL_MAX_DELAY * MS_PER_SEC);

sock_udp_create(&sock, &local, NULL, 0);
timer.callback = _post_solicit_servers;
xtimer_set(&timer, delay);
_set_event_timeout_ms(&solicit_renew_timeout, &solicit_servers, delay);
}
}

Expand All @@ -174,24 +185,11 @@ void dhcpv6_client_req_ia_pd(unsigned netif, unsigned pfx_len)
}
}

static void _post_solicit_servers(void *args)
static void _post_solicit_servers(void)
{
(void)args;
event_post(event_queue, &solicit_servers);
}

static void _post_renew(void *args)
{
(void)args;
event_post(event_queue, &renew);
}

static void _post_rebind(void *args)
{
(void)args;
event_post(event_queue, &rebind);
}

static void _generate_tid(void)
{
transaction_id = random_uint32() & 0xffffff;
Expand All @@ -215,7 +213,20 @@ static inline bool _is_tid(dhcpv6_msg_t *msg)

static inline uint32_t _now_cs(void)
{
#if IS_USED(MODULE_ZTIMER)
return (uint32_t)(ztimer_now(ZTIMER_MSEC) / MS_PER_CS);
#else
return (uint32_t)(xtimer_now_usec64() / US_PER_CS);
#endif
}

static inline uint32_t _now_sec(void)
Copy link
Copy Markdown
Member

@maribu maribu Jul 23, 2021

Choose a reason for hiding this comment

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

There is a potential footgun with using both timestamps from ZTIMER_SEC and ZTIMER_MSEC, as both could be provided by different hardware. One could (or at least I can see myself doing so) assume that the timestamps can be converted to each other.

I don't see this incorrect assumption anywhere here in the code, but I'm not really familiar with it.

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 23, 2021

Choose a reason for hiding this comment

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

Yes, the timestamp should be used independent from each other and are only used for the respective timeouts using the different scales.

Copy link
Copy Markdown
Contributor

@kfessel kfessel Jul 23, 2021

Choose a reason for hiding this comment

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

there are multiple potential foot guns, but none of them is worse than it was before with just xtimer, especialiy within reasnonable limit of leasetime and T1 and T2 I would advice against giving longer than 1 month leases

Copy link
Copy Markdown
Contributor

@kfessel kfessel Jul 23, 2021

Choose a reason for hiding this comment

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

The drift pontential of msec vs sec is low enough for T1 and T2 application,

The problem left is one might uses SEC as some kind of RTC (which should not be done), combine that with DHCP and some NTP after getting the dhcp lease and you get a time scramble, this is a problem of the ztimer and not of this DHCP.

{
#if IS_USED(MODULE_ZTIMER)
return (uint32_t)ztimer_now(ZTIMER_SEC);
#else
return (uint32_t)(xtimer_now_usec64() / US_PER_SEC);
#endif
}

static inline uint16_t _compose_cid_opt(dhcpv6_opt_duid_t *cid)
Expand Down Expand Up @@ -244,7 +255,7 @@ static inline uint16_t _get_elapsed_time(void)
uint32_t elapsed_time = transaction_start - now;

if (elapsed_time > UINT16_MAX) {
/* xtimer_now_usec64() overflowed since transaction_start */
/* now overflowed since transaction_start */
elapsed_time = (UINT32_MAX - transaction_start) + now + 1;
}
return elapsed_time;
Expand Down Expand Up @@ -330,36 +341,36 @@ static inline size_t _add_ia_pd_from_config(uint8_t *buf, size_t len_max)
return msg_len;
}

static inline int32_t get_rand_us_factor(void)
static inline int32_t get_rand_ms_factor(void)
{
int32_t res = ((int32_t)random_uint32_range(0, 200 * US_PER_MS));
res -= 100 * US_PER_SEC;
int32_t res = ((int32_t)random_uint32_range(0, 200));
res -= 100;
return res;
}

static inline uint32_t _irt_us(uint16_t irt, bool greater_irt)
static inline uint32_t _irt_ms(uint16_t irt, bool greater_irt)
{
uint32_t irt_us = (irt * US_PER_SEC);
int32_t factor = get_rand_us_factor();
uint32_t irt_ms = (irt * MS_PER_SEC);
int32_t factor = get_rand_ms_factor();

if (greater_irt && (factor < 0)) {
factor = -factor;
}
irt_us += (factor * irt_us) / US_PER_SEC;
return irt_us;
irt_ms += (factor * irt_ms) / MS_PER_SEC;
return irt_ms;
}

static inline uint32_t _sub_rt_us(uint32_t rt_prev_us, uint16_t mrt)
static inline uint32_t _sub_rt_ms(uint32_t rt_prev_ms, uint16_t mrt)
{
uint32_t sub_rt_us = (2 * rt_prev_us) +
((get_rand_us_factor() * rt_prev_us) / US_PER_SEC);
uint32_t sub_rt_ms = (2 * rt_prev_ms) +
((get_rand_ms_factor() * rt_prev_ms) / MS_PER_SEC);

if (sub_rt_us > (mrt * US_PER_SEC)) {
uint32_t mrt_us = mrt * US_PER_SEC;
if (sub_rt_ms > (mrt * MS_PER_SEC)) {
uint32_t mrt_ms = mrt * MS_PER_SEC;

sub_rt_us = mrt_us + ((get_rand_us_factor() * mrt_us) / US_PER_SEC);
sub_rt_ms = mrt_ms + ((get_rand_ms_factor() * mrt_ms) / MS_PER_SEC);
}
return sub_rt_us;
return sub_rt_ms;
}

static inline size_t _opt_len(dhcpv6_opt_t *opt)
Expand Down Expand Up @@ -494,27 +505,21 @@ static int _preparse_advertise(uint8_t *adv, size_t len, uint8_t **buf)
static void _schedule_t2(void)
{
if (t2 < UINT32_MAX) {
uint64_t t2_usec = t2 * US_PER_SEC;

rebind_time = (xtimer_now_usec64() / US_PER_SEC) + t2;
xtimer_remove(&rebind_timer);
rebind_timer.callback = _post_rebind;
rebind_time = _now_sec() + t2;
Copy link
Copy Markdown
Contributor

@kfessel kfessel Jul 21, 2021

Choose a reason for hiding this comment

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

this and l.: 842 seem to be the only lines that access rebind_time, they cancel each other +t2 and -t2

at a first glance the -t2 in l.: 842 may be wrong (but might have been working around: failing renew since mrd seems to be used to evaluate if a renew is atempted '-t2' make this only be done if the renew is run in the same second as the "_schedule_t2").

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 21, 2021

Choose a reason for hiding this comment

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

This PR aims not to change the logic of the DHCPv6 implementation, but only refactors, so even if it is wrong, that change would belong into a different PR. Hovewer, L. 842 calculates the MRD for a RENEW transmission (please refer to the RFC to figure out what this means exactly, the implementation was a few years ago, I would need to reread it myself, to explain it properly myself; all the more reason why I don't want to change logic in this PR ... ;-)). This line only reschedules the next T2 event which was supplied in a REPLY message by the IA_PD option (or IA_NA option, once #16228 is in), so I don't see how the two operations would be related. I guess in L. 842 we want some kind of offset to T2, as for RENEW the MRD shall be "Remaining time until earliest T2".

Concerns about things happening at the same time don't make sense, since the client runs in a single thread ;-).

Copy link
Copy Markdown
Contributor

@kfessel kfessel Jul 22, 2021

Choose a reason for hiding this comment

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

Something is wrong with the mrd calculation in the case of "renew" and "rebind" (request does not calculate an mrd).

(rebind will never happen, renew may run longer than t2 allows)

The error does not influence how the system works if the first renew is successful -> dificult to see in the wild

I only looked at these lines because i checked on the xtimer ztimer conversion.

The error(s) was in there before.

everything done in this PR is working as expected

_clear_event_timeout(&rebind_timeout);
DEBUG("DHCPv6 client: scheduling REBIND in %lu sec\n",
(unsigned long)t2);
xtimer_set64(&rebind_timer, t2_usec);
_set_event_timeout_sec(&rebind_timeout, &rebind, t2);
}
}

static void _schedule_t1_t2(void)
{
if (server.t1 < UINT32_MAX) {
uint64_t t1_usec = server.t1 * US_PER_SEC;

xtimer_remove(&timer);
timer.callback = _post_renew;
_clear_event_timeout(&solicit_renew_timeout);
DEBUG("DHCPv6 client: scheduling RENEW in %lu sec\n",
(unsigned long)server.t1);
xtimer_set64(&timer, t1_usec);
_set_event_timeout_sec(&solicit_renew_timeout, &renew, server.t1);
}
_schedule_t2();
}
Expand All @@ -526,10 +531,9 @@ static void _parse_advertise(uint8_t *adv, size_t len)
/* might not have been executed when not received in first retransmission
* window => redo even if already done */
if (_preparse_advertise(adv, len, NULL) < 0) {
uint32_t delay = _irt_us(DHCPV6_SOL_TIMEOUT, true);
uint32_t delay = _irt_ms(DHCPV6_SOL_TIMEOUT, true);
/* SOLICIT new server */
timer.callback = _post_solicit_servers;
xtimer_set(&timer, delay);
_set_event_timeout_ms(&solicit_renew_timeout, &solicit_servers, delay);
return;
}
DEBUG("DHCPv6 client: scheduling REQUEST\n");
Expand Down Expand Up @@ -751,7 +755,7 @@ static void _solicit_servers(event_t *event)
dhcpv6_msg_t *msg = (dhcpv6_msg_t *)&send_buf[0];
dhcpv6_opt_elapsed_time_t *time;
uint8_t *buf = NULL;
uint32_t retrans_timeout = _irt_us(DHCPV6_SOL_TIMEOUT, true);
uint32_t retrans_timeout = _irt_ms(DHCPV6_SOL_TIMEOUT, true);
size_t msg_len = sizeof(dhcpv6_msg_t);
int res, best_res = 0;
bool first_rt = true;
Expand All @@ -773,28 +777,28 @@ static void _solicit_servers(event_t *event)
res = sock_udp_send(&sock, send_buf, msg_len, &remote);
assert(res > 0); /* something went terribly wrong */
while (((res = sock_udp_recv(&sock, recv_buf, sizeof(recv_buf),
retrans_timeout, NULL)) <= 0) ||
retrans_timeout * US_PER_MS, NULL)) <= 0) ||
(first_rt && (res > 0)) ||
((res > 0) && (recv_buf[0] != DHCPV6_ADVERTISE))) {
if (first_rt && (res > 0) && (recv_buf[0] == DHCPV6_ADVERTISE)) {
int parse_res;

DEBUG("DHCPv6 client: initial transmission, collect best advertise\n");
retrans_timeout -= (_get_elapsed_time() * US_PER_CS);
retrans_timeout -= (_get_elapsed_time() * MS_PER_CS);
parse_res = _preparse_advertise(recv_buf, res, &buf);
if (buf != NULL) {
best_res = res;
}
if ((parse_res == UINT8_MAX) ||
(retrans_timeout > (DHCPV6_SOL_MAX_RT * US_PER_SEC))) {
(retrans_timeout > (DHCPV6_SOL_MAX_RT * MS_PER_SEC))) {
/* retrans_timeout underflowed => don't retry to receive */
break;
}
}
else if (buf == NULL) {
DEBUG("DHCPv6 client: resend SOLICIT\n");
first_rt = false;
retrans_timeout = _sub_rt_us(retrans_timeout, DHCPV6_SOL_MAX_RT);
retrans_timeout = _sub_rt_ms(retrans_timeout, DHCPV6_SOL_MAX_RT);
_compose_elapsed_time_opt(time);
res = sock_udp_send(&sock, send_buf, msg_len, &remote);
assert(res > 0); /* something went terribly wrong */
Expand Down Expand Up @@ -857,15 +861,15 @@ static void _request_renew_rebind(uint8_t type)
if (mrd > 0) {
/* all leases already expired, don't try to rebind and
* solicit immediately */
_post_solicit_servers(NULL);
_post_solicit_servers();
return;
}
break;
}
default:
return;
}
retrans_timeout = _irt_us(irt, false);
retrans_timeout = _irt_ms(irt, false);
_generate_tid();
msg->type = type;
_set_tid(msg->tid);
Expand All @@ -884,12 +888,12 @@ static void _request_renew_rebind(uint8_t type)
_flush_stale_replies(&sock);
while (sock_udp_send(&sock, send_buf, msg_len, &remote) <= 0) {}
while (((res = sock_udp_recv(&sock, recv_buf, sizeof(recv_buf),
retrans_timeout, NULL)) <= 0) ||
retrans_timeout * US_PER_MS, NULL)) <= 0) ||
((res > 0) && (recv_buf[0] != DHCPV6_REPLY))) {
if ((mrd > 0) && (_get_elapsed_time() > (mrd * CS_PER_SEC))) {
break;
}
retrans_timeout = _sub_rt_us(retrans_timeout, mrt);
retrans_timeout = _sub_rt_ms(retrans_timeout, mrt);
if ((mrc > 0) && (++retrans) >= mrc) {
break;
}
Expand All @@ -906,7 +910,7 @@ static void _request_renew_rebind(uint8_t type)
}
}
else if (type == DHCPV6_REBIND) {
_post_solicit_servers(NULL);
_post_solicit_servers();
}
}

Expand All @@ -931,4 +935,34 @@ static void _rebind(event_t *event)
_request_renew_rebind(DHCPV6_REBIND);
}

static void _set_event_timeout_ms(event_timeout_t *timeout, event_t *event,
uint32_t delay_ms)
{
#if IS_USED(MODULE_EVENT_TIMEOUT_ZTIMER)
event_timeout_ztimer_init(timeout, ZTIMER_MSEC, event_queue, event);
event_timeout_set(timeout, delay_ms);
#else
event_timeout_init(timeout, event_queue, event);
event_timeout_set(timeout, delay_ms * US_PER_MS);
#endif
}

static void _set_event_timeout_sec(event_timeout_t *timeout, event_t *event,
uint32_t delay_sec)
{
#if IS_USED(MODULE_EVENT_TIMEOUT_ZTIMER)
event_timeout_ztimer_init(timeout, ZTIMER_SEC, event_queue, event);
event_timeout_set(timeout, delay_sec);
#else
event_timeout_init(timeout, event_queue, event);
/* use xtimer_set64 instead of event_timeout_set to prevent overflows */
xtimer_set64(&timeout->timer, ((uint64_t)delay_sec) * US_PER_SEC);
#endif
Comment on lines +953 to +960
Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe i would like the use of just msec (one ztimer to rule them all, this could also avoid some jitter but dhcpv6 seems to be tolerant to that) ❓

Main reason for also using ZTIMER_SEC is thes lines. I really do not want to cap of a potential overflow here. Since this function is used to set the T1 and T2 timeouts (both come over the wire) it is not guaranteed that they will be e.g. UINT32_MAX - 1.

Copy link
Copy Markdown
Contributor

@kfessel kfessel Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

good, But: This may still be problematic since gnrc_dhcp dhcpv6_client_conf_prefix and nib _nib_pl_add have a max addr lifetime of uint32_max-1 msecs or infite
which will make the addr last shorter than its T1 and T2 timeout

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 23, 2021

Choose a reason for hiding this comment

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

Sure, but nothing prevents us to port that to ZTIMER_SEC in the future as well ;-). Currently the same problem exists in master, since the xtimer of DHCPv6 uses 64-bit scale.

}

static void _clear_event_timeout(event_timeout_t *timeout)
{
event_timeout_clear(timeout);
}

/** @} */