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

DHCP6: IPv6 Ready Logo test fixes #397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

DHCP6: IPv6 Ready Logo test fixes #397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
154 changes: 86 additions & 68 deletions src/dhcp6.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ static const char * const dhcp6_statuses[] = {
};

static void dhcp6_bind(struct interface *, const char *, const char *);
static void dhcp6_failinform(void *);
static void dhcp6_startrebind(void *arg);
static void dhcp6_recvaddr(void *, unsigned short);
static void dhcp6_startdecline(struct interface *);
Expand Down Expand Up @@ -984,6 +983,15 @@ dhcp6_makemessage(struct interface *ifp)
ia_na.t2 = 0;
COPYIN(ifia->ia_type, &ia_na, ia_na_len);
TAILQ_FOREACH(ap, &state->addrs, next) {
/* A bit of a corner case, but tested by IPv6 Ready Logo:
* Don't attempt to renew expired leases. This can
* really only happen if the server is poorly configured and
* doesn't allow enough time for lease renewal per the
* T1 and valid lifetime parameters it sent.
*/
if ((state->state == DH6S_RENEW) &&
(ap->prefix_vltime <= state->renew))
continue;
Copy link
Member

Choose a reason for hiding this comment

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

The comment does not match the code. vltime and renew values don't actually change - when we add an address we adjust the vltime at that point based on the acquisition time vs now time.
This change makes the assumption that RENEW only happens when t1 expires, but that isn't always the case.
So the address could still be valid and therefore should be renewed.

When an address does expire it is removed from the array, unless it's been requested via config.

So what is this code really trying to achieve?

if (ap->flags & IPV6_AF_STALE)
continue;
if (!(ap->flags & IPV6_AF_REQUEST) &&
Expand Down Expand Up @@ -1412,7 +1420,7 @@ dhcp6_sendmessage(struct interface *ifp, void (*callback)(void *))
state->RT = RT * 2;
if (state->RT < RT) /* Check overflow */
state->RT = RT;
if (state->MRC == 0 || state->RTC < state->MRC)
if (state->MRC == 0 || state->RTC <= state->MRC)
eloop_timeout_add_msec(ctx->eloop,
RT, callback, ifp);
else if (state->MRC != 0 && state->MRCcallback)
Expand Down Expand Up @@ -1677,7 +1685,6 @@ dhcp6_startdiscover(void *arg)
state->IRT = SOL_TIMEOUT;
state->MRT = state->sol_max_rt;
state->MRC = SOL_MAX_RC;

/* If we fail to renew or confirm, our requested addreses will
* be marked as stale.
To re-request them, just mark them as not stale. */
Expand Down Expand Up @@ -1714,14 +1721,8 @@ dhcp6_startinform(void *arg)
logerr("%s: %s", __func__, ifp->name);
return;
}
dhcp6_sendinform(ifp);
/* RFC3315 18.1.2 says that if CONFIRM failed then the prior addresses
* SHOULD be used. The wording here is poor, because the addresses are
* merely one facet of the lease as a whole.
* This poor wording might explain the lack of similar text for INFORM
* in 18.1.5 because there are no addresses in the INFORM message. */
eloop_timeout_add_sec(ifp->ctx->eloop,
INF_MAX_RD, dhcp6_failinform, ifp);
else
dhcp6_sendinform(ifp);
Copy link
Member

Choose a reason for hiding this comment

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

OK, I see what you're trying to fix here. We want to drop through to failinform so we can use the lastlease code, but only on the first failure.

So I think what we need to do is after we have logged the message we want to guard the state timing changes like so

if (state->state == DH6S_INFORM) {
      dhcp6_sendinform(ifp);
      return;
}

Then we change failinform to do dhcp6_fail(ifp, false) - ie we don't force a drop.
Adjust the code there to handle lastlease for INFORM and we should be good yes?

}

static bool
Expand Down Expand Up @@ -1810,17 +1811,6 @@ dhcp6_failrequest(void *arg)
dhcp6_fail(ifp, true);
}

static void
dhcp6_failinform(void *arg)
{
struct interface *ifp = arg;
int llevel = dhcp6_failloglevel(ifp);

logmessage(llevel, "%s: failed to request DHCPv6 information",
ifp->name);
dhcp6_fail(ifp, true);
}

#ifndef SMALL
static void
dhcp6_failrebindpd(void *arg)
Expand Down Expand Up @@ -2104,6 +2094,17 @@ dhcp6_checkstatusok(const struct interface *ifp,
free(sbuf);
state->lerror = code;
errno = 0;
if (code == D6_STATUS_USEMULTICAST) {
/* RFC 8415 18.2.10
* If the client receives a Reply message with a status code of
* UseMulticast, the client records the receipt of the message and sends
* subsequent messages to the server through the interface on which the
* message was received using multicast. The client resends the
* original message using multicast.
*/
logdebugx("%s: server sent UseMulticast", ifp->name);
state->unicast = in6addr_any;
}

/* code cannot be D6_STATUS_OK, so there is a failure */
if (ifp->ctx->options & DHCPCD_TEST)
Expand Down Expand Up @@ -3301,6 +3302,51 @@ dhcp6_bind(struct interface *ifp, const char *op, const char *sfrom)
}
}

static void
dhcp6_adjust_max_rt(struct interface *ifp, const char *sfrom,
struct dhcp6_message *r, size_t len)
{
struct dhcp6_state *state;
uint8_t *o;
uint16_t ol;

state = D6_STATE(ifp);
/* RFC8415 */
o = dhcp6_findmoption(r, len, D6_OPTION_SOL_MAX_RT, &ol);
if (o && ol == sizeof(uint32_t)) {
uint32_t max_rt;

memcpy(&max_rt, o, sizeof(max_rt));
max_rt = ntohl(max_rt);
if (max_rt >= 60 && max_rt <= 86400) {
logdebugx("%s: SOL_MAX_RT %llu -> %u",
ifp->name,
(unsigned long long)state->sol_max_rt,
max_rt);
state->sol_max_rt = max_rt;
state->MRT = max_rt;
} else
logerr("%s: invalid SOL_MAX_RT %u",
ifp->name, max_rt);
}
o = dhcp6_findmoption(r, len, D6_OPTION_INF_MAX_RT, &ol);
if (o && ol == sizeof(uint32_t)) {
uint32_t max_rt;

memcpy(&max_rt, o, sizeof(max_rt));
max_rt = ntohl(max_rt);
if (max_rt >= 60 && max_rt <= 86400) {
logdebugx("%s: INF_MAX_RT %llu -> %u",
ifp->name,
(unsigned long long)state->inf_max_rt,
max_rt);
state->inf_max_rt = max_rt;
} else
logerrx("%s: invalid INF_MAX_RT %u",
ifp->name, max_rt);
}
}

static void
dhcp6_recvif(struct interface *ifp, const char *sfrom,
struct dhcp6_message *r, size_t len)
Expand Down Expand Up @@ -3419,6 +3465,7 @@ dhcp6_recvif(struct interface *ifp, const char *sfrom,
/* Validate lease before setting state to REQUEST. */
/* FALLTHROUGH */
case DH6S_REQUEST: /* FALLTHROUGH */
dhcp6_adjust_max_rt(ifp, sfrom, r, len);
case DH6S_RENEW: /* FALLTHROUGH */
case DH6S_REBIND:
if (dhcp6_validatelease(ifp, r, len, sfrom, NULL) == -1)
Expand All @@ -3427,7 +3474,7 @@ dhcp6_recvif(struct interface *ifp, const char *sfrom,
* If we can't use the lease, fallback to
* DISCOVER and try and get a new one.
*
* This is needed become some servers
* This is needed because some servers
* renumber the prefix or address
* and deny the current one before it expires
* rather than sending it back with a zero
Expand All @@ -3439,16 +3486,20 @@ dhcp6_recvif(struct interface *ifp, const char *sfrom,
* The currently held lease is still valid
* until a new one is found.
*/
if (state->state != DH6S_DISCOVER)
dhcp6_startdiscoinform(ifp);
return;
}
/* RFC8415 18.2.10.1 */
if ((state->state == DH6S_RENEW ||
state->state == DH6S_REBIND) &&
state->has_no_binding)
{
dhcp6_startrequest(ifp);
/* RFC 8415 18.2.10 calls out various behaviors
* for some of the status codes a client might receive:
* UnspecFail, and NotOnLink. Handle those here.
*/
if ((state->state == DH6S_RENEW ||
state->state == DH6S_REBIND) &&
state->has_no_binding) {
dhcp6_startrequest(ifp);
}
else if (state->state == DH6S_REQUEST &&
state->lerror == D6_STATUS_NOTONLINK) {
eloop_timeout_delete(ifp->ctx->eloop, NULL, ifp);
dhcp6_startdiscover(ifp);
}
Copy link
Member

Choose a reason for hiding this comment

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

We are no longer handling the state where we got a reply without an error but it didn't contain any addresses with error codes either.

return;
}
if (state->state == DH6S_DISCOVER)
Expand Down Expand Up @@ -3488,40 +3539,7 @@ dhcp6_recvif(struct interface *ifp, const char *sfrom,
return;
}
}

/* RFC7083 */
o = dhcp6_findmoption(r, len, D6_OPTION_SOL_MAX_RT, &ol);
if (o && ol == sizeof(uint32_t)) {
uint32_t max_rt;

memcpy(&max_rt, o, sizeof(max_rt));
max_rt = ntohl(max_rt);
if (max_rt >= 60 && max_rt <= 86400) {
logdebugx("%s: SOL_MAX_RT %llu -> %u",
ifp->name,
(unsigned long long)state->sol_max_rt,
max_rt);
state->sol_max_rt = max_rt;
} else
logerr("%s: invalid SOL_MAX_RT %u",
ifp->name, max_rt);
}
o = dhcp6_findmoption(r, len, D6_OPTION_INF_MAX_RT, &ol);
if (o && ol == sizeof(uint32_t)) {
uint32_t max_rt;

memcpy(&max_rt, o, sizeof(max_rt));
max_rt = ntohl(max_rt);
if (max_rt >= 60 && max_rt <= 86400) {
logdebugx("%s: INF_MAX_RT %llu -> %u",
ifp->name,
(unsigned long long)state->inf_max_rt,
max_rt);
state->inf_max_rt = max_rt;
} else
logerrx("%s: invalid INF_MAX_RT %u",
ifp->name, max_rt);
}
dhcp6_adjust_max_rt(ifp, sfrom, r, len);
if (dhcp6_validatelease(ifp, r, len, sfrom, NULL) == -1)
return;
break;
Expand Down Expand Up @@ -3614,7 +3632,7 @@ dhcp6_recvif(struct interface *ifp, const char *sfrom,
ifp->name, ia->saddr, sfrom, preference);

/*
* RFC 8415 18.2.1 says we must collect until ADVERTISEMENTs
* RFC 8415 18.2.1 says we must collect ADVERTISEMENTs
* until we get one with a preference of 255 or
* the initial RT has elpased.
*/
Expand Down
9 changes: 4 additions & 5 deletions src/dhcp6.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@

#define SOL_MAX_DELAY 1
#define SOL_TIMEOUT 1
#define SOL_MAX_RT 3600 /* RFC7083 */
#define SOL_MAX_RT 3600 /* RFC8415 */
#define SOL_MAX_RC 0
#define REQ_MAX_DELAY 0
#define REQ_TIMEOUT 1
Expand All @@ -134,15 +134,14 @@
#define REB_MAX_RT 600
#define INF_MAX_DELAY 1
#define INF_TIMEOUT 1
#define INF_MAX_RD CNF_MAX_RD /* NOT RFC defined */
#define INF_MAX_RT 3600 /* RFC7083 */
#define INF_MAX_RT 3600 /* RFC8415 */
#define REL_MAX_DELAY 0
#define REL_TIMEOUT 1
#define REL_MAX_RT 0
#define REL_MAX_RC 5
#define REL_MAX_RC 4 /* RFC8415 */
#define DEC_MAX_DELAY 0
#define DEC_TIMEOUT 1
#define DEC_MAX_RC 5
#define DEC_MAX_RC 4 /* RFC8415 */
#define REC_MAX_DELAY 0
#define REC_TIMEOUT 2
#define REC_MAX_RC 8
Expand Down
Loading