Head bugfix, dueling BNDUPD/ACKs [ISC-Bugs #16346b]

diff --git a/server/failover.c b/server/failover.c
index cc8349b3a4d6219a53325920ce6d3f74d87c53ee..85c5571a41b988091e8e365aeb6950471a17c017 100644 (file)
--- a/server/failover.c
+++ b/server/failover.c
@@ -34,7 +34,7 @@
 
 #ifndef lint
 static char copyright[] =
-"$Id: failover.c,v 1.64 2006/08/24 14:55:51 dhankins Exp $ Copyright (c) 2004-2006 Internet Systems Consortium.  All rights reserved.\n";
+"$Id: failover.c,v 1.65 2006/08/28 21:35:03 dhankins Exp $ Copyright (c) 2004-2006 Internet Systems Consortium.  All rights reserved.\n";
 #endif /* not lint */
 
 #include "dhcpd.h"
@@ -1739,6 +1739,10 @@ isc_result_t dhcp_failover_set_state (dhcp_failover_state_t *state,
 
     switch (new_state) {
          case normal:
+           /* Upon entering normal state, the server is expected to retransmit
+            * all pending binding updates.
+            */
+           dhcp_failover_generate_update_queue(state, 0);
            if (state -> partner.state == normal)
                    dhcp_failover_state_pool_check (state);
            break;
@@ -2720,6 +2724,12 @@ void dhcp_failover_ack_queue_remove (dhcp_failover_state_t *state,
        }
 
        lease -> flags &= ~ON_ACK_QUEUE;
+       /* Multiple acks on one XID is an error and may cause badness. */
+       lease->last_xid = 0;
+       /* XXX: this violates draft-failover.  We can't send another
+        * update just because we forgot about an old one that hasn't
+        * been acked yet.
+        */
        state -> cur_unacked_updates--;
 
        /*
@@ -4931,15 +4941,15 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
        /* If it is probably wise, assign lease to backup state if the peer
         * is not already hoarding leases.
         */
-       if (send_to_backup && secondary_not_hoarding(state, lt->pool)) {
-               lt->next_binding_state = FTS_BACKUP;
-               lt->tstp = cur_time;
-               lt->starts = cur_time;
+       if (send_to_backup && secondary_not_hoarding(state, lease->pool)) {
+               lease->next_binding_state = FTS_BACKUP;
+               lease->tstp = cur_time;
+               lease->starts = cur_time;
 
-               if (!supersede_lease(lt, NULL, 0, 1, 0) ||
-                   !write_lease(lt))
+               if (!supersede_lease(lease, NULL, 0, 1, 0) ||
+                   !write_lease(lease))
                        log_error("can't commit lease %s for mac addr "
-                                 "affinity", piaddr(lt->ip_addr));
+                                 "affinity", piaddr(lease->ip_addr));
 
                dhcp_failover_send_updates(state);
        }
@@ -4989,6 +4999,7 @@ isc_result_t dhcp_failover_process_bind_ack (dhcp_failover_state_t *state,
        struct iaddr ia;
        const char *message = "no memory";
        u_int32_t pot_expire;
+       int send_to_backup = ISC_FALSE;
 
        ia.len = sizeof msg -> assigned_addr;
        memcpy (ia.iabuf, &msg -> assigned_addr, ia.len);
@@ -5013,28 +5024,49 @@ isc_result_t dhcp_failover_process_bind_ack (dhcp_failover_state_t *state,
                goto unqueue;
        }
 
+       /* Silently discard acks for leases we did not update (or multiple
+        * acks).
+        */
+       if (!lease->last_xid)
+               goto unqueue;
+
+       if (lease->last_xid != msg->xid) {
+               message = "xid mismatch";
+               goto bad;
+       }
+
        /* XXX Times may need to be adjusted based on clock skew! */
        if (msg->options_present & FTO_POTENTIAL_EXPIRY)
                pot_expire = msg->potential_expiry;
-       else if(lease->last_xid && (msg->xid == lease->last_xid))
+       else
                pot_expire = lease->tstp;
-       else {
-               message = "peer sent no potential-expiry and mismatched XID.";
-               goto bad;
-       }
 
-       /* XXX it could be a problem to do this directly if the
-          XXX lease is sorted by tsfp. */
-       if (lease->binding_state == FTS_EXPIRED ||
-           lease->binding_state == FTS_RESET ||
-           lease->binding_state == FTS_RELEASED)
+       /* If the lease was desired to enter a binding state, we set
+        * such a value upon transmitting a bndupd.  We do not clear it
+        * if we receive a bndupd in the meantime (or change the state
+        * of the lease again ourselves), but we do set binding_state
+        * if we get a bndupd.
+        *
+        * So desired_binding_state tells us what we sent a bndupd for,
+        * and binding_state tells us what we have since determined in
+        * the meantime.
+        */
+       if (lease->desired_binding_state == FTS_EXPIRED ||
+           lease->desired_binding_state == FTS_RESET ||
+           lease->desired_binding_state == FTS_RELEASED)
        {
+               /* It is not a problem to do this directly as we call
+                * supersede_lease immediately after: the lease is requeued
+                * even if its sort order (tsfp) has changed.
+                */
                lease->atsfp = lease->tsfp = pot_expire;
                if ((state->i_am == secondary) &&
                    (lease->flags & RESERVED_LEASE))
                        lease->next_binding_state = FTS_BACKUP;
                else
                        lease->next_binding_state = FTS_FREE;
+               /* Clear this condition for the next go-round. */
+               lease->desired_binding_state = lease->next_binding_state;
                supersede_lease(lease, (struct lease *)0, 0, 0, 0);
                write_lease(lease);
 
@@ -5042,25 +5074,20 @@ isc_result_t dhcp_failover_process_bind_ack (dhcp_failover_state_t *state,
                 * transitional states.  If the lease 'belongs'
                 * to a client that would be served by the
                 * peer, process a binding update now to send
-                * the lease to backup state.
+                * the lease to backup state.  But not if we
+                * think we already have.
                 */
                if (state->i_am == primary &&
                    !(lease->flags & (RESERVED_LEASE | BOOTP_LEASE)) &&
-                   peer_wants_lease(lease)) {
-                       lease->next_binding_state = FTS_BACKUP;
-                       lease->tstp = cur_time;
-                       lease->starts = cur_time;
-
-                       if (!supersede_lease(lease, NULL, 0, 1, 0) ||
-                           !write_lease(lease))
-                               log_error("can't commit lease %s for "
-                                         "client affinity",
-                                         piaddr(lease->ip_addr));
-               }
+                   peer_wants_lease(lease))
+                       send_to_backup = ISC_TRUE;
 
-               if (state->me.state == normal)
-                       commit_leases ();
+               if (!send_to_backup && state->me.state == normal)
+                       commit_leases();
        } else {
+               /* XXX It could be a problem to do this directly if the lease
+                * XXX is sorted by tsfp.
+                */
                lease->atsfp = lease->tsfp = pot_expire;
                if (lease->desired_binding_state != lease->binding_state) {
                        lease->next_binding_state =
@@ -5088,6 +5115,22 @@ isc_result_t dhcp_failover_process_bind_ack (dhcp_failover_state_t *state,
                dhcp_failover_send_update_done (state);
        }
 
+       /* Now that the least is off the ack queue, consider putting it
+        * back on the update queue for mac address affinity.
+        */
+       if (send_to_backup && secondary_not_hoarding(state, lease->pool)) {
+               lease->next_binding_state = FTS_BACKUP;
+               lease->tstp = lease->starts = cur_time;
+
+               if (!supersede_lease(lease, NULL, 0, 1, 0) ||
+                   !write_lease(lease))
+                       log_error("can't commit lease %s for "
+                                 "client affinity", piaddr(lease->ip_addr));
+
+               if (state->me.state == normal)
+                       commit_leases();
+       }
+
        /* If there are updates pending, we've created space to send at
           least one. */
        dhcp_failover_send_updates (state);
@@ -5100,7 +5143,7 @@ isc_result_t dhcp_failover_process_bind_ack (dhcp_failover_state_t *state,
        return ISC_R_SUCCESS;
 
       bad:
-       log_info ("bind update on %s from %s: %s.",
+       log_info ("bind update on %s got ack from %s: %s.",
                  piaddr (ia), state -> name, message);
        goto out;
 }

diff --git a/server/mdb.c b/server/mdb.c
index 1986d608cf84cd428201ce38986f89cef2567cb4..0372e625062c6c626c35bd170c9c50f09c9d6b4d 100644 (file)
--- a/server/mdb.c
+++ b/server/mdb.c
@@ -34,7 +34,7 @@
 
 #ifndef lint
 static char copyright[] =
-"$Id: mdb.c,v 1.83 2006/07/25 13:26:00 shane Exp $ Copyright (c) 2004-2006 Internet Systems Consortium.  All rights reserved.\n";
+"$Id: mdb.c,v 1.84 2006/08/28 21:35:03 dhankins Exp $ Copyright (c) 2004-2006 Internet Systems Consortium.  All rights reserved.\n";
 #endif /* not lint */
 
 #include "dhcpd.h"
@@ -1482,7 +1482,14 @@ void release_lease (lease, packet)
                /* Blow away any bindings. */
                if (lease -> scope)
                        binding_scope_dereference (&lease -> scope, MDL);
+
+               /* Set sort times to the present. */
                lease -> ends = cur_time;
+               /* Lower layers of muckery set tstp to ->ends.  But we send
+                * protocol messages before this.  So it is best to set
+                * tstp now anyway.
+                */
+               lease->tstp = cur_time;
 #if defined (FAILOVER_PROTOCOL)
                if (lease -> pool && lease -> pool -> failover_peer) {
                        lease -> next_binding_state = FTS_RELEASED;