From b8d45c67ea23f8676dc050a1350c876fed372ffc Mon Sep 17 00:00:00 2001 From: David Hankins Date: Wed, 22 Jul 2009 17:00:01 +0000 Subject: [PATCH] - Secondary servers in a failover pair will now perform ddns removals if they had performed ddns updates on a lease that is expiring, or was released through the primary. As part of the same fix, stale binding scopes will now be removed if a change in identity of a lease's active client is detected, rather than simply if a lease is noticed to have expired (which it may have expired without a failover server noticing in some situations). [ISC-Bugs #19826b] --- RELNOTES | 7 +++++++ server/failover.c | 51 ++++++++++++++++++++++++++++++++++++++++++++--- server/mdb.c | 15 ++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/RELNOTES b/RELNOTES index b7e516e8..d7d1d649 100644 --- a/RELNOTES +++ b/RELNOTES @@ -171,6 +171,13 @@ work on other platforms. Please report any problems and suggested fixes to - Don't look for IPv6 interfaces on Linux when running in DHCPv4 mode. Thanks to patches from Matthew Newton and David Cantrell. +- Secondary servers in a failover pair will now perform ddns removals if + they had performed ddns updates on a lease that is expiring, or was + released through the primary. As part of the same fix, stale binding scopes + will now be removed if a change in identity of a lease's active client is + detected, rather than simply if a lease is noticed to have expired (which it + may have expired without a failover server noticing in some situations). + Changes since 4.1.0b1 - A missing "else" in dhcrelay.c could have caused an interface not to diff --git a/server/failover.c b/server/failover.c index 78728c1c..3793fadd 100644 --- a/server/failover.c +++ b/server/failover.c @@ -5016,6 +5016,8 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state, int new_binding_state; int send_to_backup = 0; int required_options; + isc_boolean_t chaddr_changed = ISC_FALSE; + isc_boolean_t ident_changed = ISC_FALSE; /* Validate the binding update. */ required_options = FTB_ASSIGNED_IP_ADDRESS | FTB_BINDING_STATUS; @@ -5085,6 +5087,12 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state, message = "chaddr too long"; goto bad; } + + if ((lt->hardware_addr.hlen != msg->chaddr.count) || + (memcmp(lt->hardware_addr.hbuf, msg->chaddr.data, + msg->chaddr.count) != 0)) + chaddr_changed = ISC_TRUE; + lt -> hardware_addr.hlen = msg -> chaddr.count; memcpy (lt -> hardware_addr.hbuf, msg -> chaddr.data, msg -> chaddr.count); @@ -5095,6 +5103,7 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state, reason = FTR_MISSING_BINDINFO; goto bad; } else if (msg->binding_status == FTS_ABANDONED) { + chaddr_changed = ISC_TRUE; lt->hardware_addr.hlen = 0; if (lt->scope) binding_scope_dereference(<->scope, MDL); @@ -5110,6 +5119,12 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state, goto bad; } + if ((lt->uid_len != msg->client_identifier.count) || + (lt->uid == NULL) || /* Sanity; should never happen. */ + (memcmp(lt->uid, msg->client_identifier.data, + lt->uid_len) != 0)) + ident_changed = ISC_TRUE; + lt->uid_len = msg->client_identifier.count; /* Allocate the lt->uid buffer if we haven't already, or @@ -5138,15 +5153,45 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state, } else if (lt->uid && msg->binding_status != FTS_RESET && msg->binding_status != FTS_FREE && msg->binding_status != FTS_BACKUP) { + ident_changed = ISC_TRUE; if (lt->uid != lt->uid_buf) dfree (lt->uid, MDL); lt->uid = NULL; lt->uid_max = lt->uid_len = 0; } - /* If the lease was expired, also remove the stale binding scope. */ - if (lt->scope && lt->ends < cur_time) - binding_scope_dereference(<->scope, MDL); + /* + * A server's configuration can assign a 'binding scope'; + * + * set var = "value"; + * + * The problem with these binding scopes is that they are refreshed + * when the server processes a client's DHCP packet. A local binding + * scope is trash, then, when the lease has been assigned by the + * partner server. There is no real way to detect this, a peer may + * be updating us (as through potential conflict) with a binding we + * sent them, but we can trivially detect the /problematic/ case; + * + * lease is free. + * primary allocates lease to client A, assigns ddns name A. + * primary fails. + * secondary enters partner down. + * lease expires, and is set free. + * lease is allocated to client B and given ddns name B. + * primary recovers. + * + * The binding update in this case will be active->active, but the + * client identification on the lease will have changed. The ddns + * update on client A will have leaked if we just remove the binding + * scope blindly. + */ + if (msg->binding_status == FTS_ACTIVE && + (chaddr_changed || ident_changed)) { + ddns_removals(lease, NULL); + + if (lease->scope != NULL) + binding_scope_dereference(&lease->scope, MDL); + } /* XXX Times may need to be adjusted based on clock skew! */ if (msg -> options_present & FTB_STOS) { diff --git a/server/mdb.c b/server/mdb.c index 2f04aa3c..5e333555 100644 --- a/server/mdb.c +++ b/server/mdb.c @@ -1462,6 +1462,21 @@ void make_binding_state_transition (struct lease *lease) lease -> binding_state == FTS_ACTIVE && lease -> next_binding_state == FTS_RELEASED))) { #if defined (NSUPDATE) + /* + * Note: ddns_removals() is also iterated when the lease + * enters state 'released' in 'release_lease()'. The below + * is caught when a peer receives a BNDUPD from a failover + * peer; it may not have received the client's release (it + * may have been offline). + * + * We could remove the call from release_lease() because + * it will also catch here on the originating server after the + * peer acknowledges the state change. However, there could + * be many hours inbetween, and in this case we /know/ the + * client is no longer using the lease when we receive the + * release message. This is not true of expiry, where the + * peer may have extended the lease. + */ ddns_removals(lease, NULL); #endif if (lease -> on_release) {