From b78a4b21b187555d4a17b2d5c08127d962f1997f Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Mon, 11 Jul 2022 19:31:29 +0300 Subject: [PATCH] [#2485] wrong subnet-id with shared network - backport #2409 to 2.0.3 --- ChangeLog | 16 ++++ .../dhcp4/tests/shared_network_unittest.cc | 91 ++++++++++++++++++- src/lib/dhcpsrv/alloc_engine.cc | 8 +- 3 files changed, 111 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5c8759b649..de0814a7e5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,19 @@ +1960. [bug] marcin + A bug in the allocation engine, which caused it to write an + allocated lease under the wrong subnet ID within a shared + network, has been corrected. This was occurring when multiple + clients matched the same fixed address reservation. The first + client is now assigned the fixed address, while a subsequent + client is then given a dynamically allocated address from a + different subnet in the shared network. + (Gitlab #2409, #2485) + +1959. [bug] razvan, marcin + Fixed a crash in HA+MT scenario caused by a race condition + between resetting the CalloutHandle state and accessing hook + point parameters from different threads when unparking packets. + (Gitlab #2473, #2477) + 1958. [build] andrei Froze sphinx dependency versions used to build documentation. Added the update-python-dependencies Makefile rule to bump the diff --git a/src/bin/dhcp4/tests/shared_network_unittest.cc b/src/bin/dhcp4/tests/shared_network_unittest.cc index 60eb4550e7..cc1f908163 100644 --- a/src/bin/dhcp4/tests/shared_network_unittest.cc +++ b/src/bin/dhcp4/tests/shared_network_unittest.cc @@ -1013,8 +1013,47 @@ const char* NETWORKS_CONFIG[] = { " ]" " }" " ]" - "}" + "}", +// Configuration #20 +// - a shared network with two subnets +// - first subnet has a dynamic address pool +// - second subnet has no address pool but it has a host reservation + "{" + " \"interfaces-config\": {" + " \"interfaces\": [ \"*\" ]" + " }," + " \"valid-lifetime\": 600," + " \"shared-networks\": [" + " {" + " \"name\": \"frog\"," + " \"relay\": {" + " \"ip-address\": \"192.3.5.6\"" + " }," + " \"subnet4\": [" + " {" + " \"subnet\": \"10.0.0.0/24\"," + " \"id\": 100," + " \"pools\": [" + " {" + " \"pool\": \"10.0.0.1 - 10.0.0.1\"" + " }" + " ]" + " }," + " {" + " \"subnet\": \"192.0.2.0/26\"," + " \"id\": 10," + " \"reservations\": [" + " {" + " \"circuit-id\": \"'charter950'\"," + " \"ip-address\": \"192.0.2.28\"" + " }" + " ]" + " }" + " ]" + " }" + " ]" + "}" }; /// @Brief Test fixture class for DHCPv4 server using shared networks. @@ -1681,6 +1720,56 @@ TEST_F(Dhcpv4SharedNetworkTest, reservationInSharedNetwork) { }); } +// Two clients use the same circuit ID and this circuit ID is used to assign a +// host reservation. The clients compete for the reservation, and one of them +// gets it and the other one gets an address from the dynamic pool. This test +// checks that the assigned leases have appropriate subnet IDs. Previously, the +// client getting the lease from the dynamic pool had a wrong subnet ID (not +// matching the address from the dynamic pool). +TEST_F(Dhcpv4SharedNetworkTest, reservationInSharedNetworkTwoClientsSameIdentifier) { + // Create client #1 which uses a circuit ID as a host identifier. + Dhcp4Client client1(Dhcp4Client::SELECTING); + client1.useRelay(true, IOAddress("192.3.5.6")); + client1.includeClientId("01:02:03:04"); + client1.setCircuitId("charter950"); + + // Create server configuration with a shared network including two subnets. + // One of the subnets includes a reservation identified by the client's + // circuit ID. + configure(NETWORKS_CONFIG[20], *client1.getServer()); + + // Client #1 should get the reserved address. + testAssigned([this, &client1] { + doDORA(client1, "192.0.2.28", ""); + }); + + // Create client #2 with the same circuit ID. + Dhcp4Client client2(client1.getServer(), Dhcp4Client::SELECTING); + client2.useRelay(true, IOAddress("192.3.5.6")); + client2.includeClientId("02:03:04:05"); + client2.setCircuitId("charter950"); + + // Client #2 presents the same circuit ID but the reserved address has been + // already allocated. The client should get an address from the dynamic pool. + testAssigned([this, &client2] { + doDORA(client2, "10.0.0.1", "192.0.2.28"); + }); + + // Client #1 renews the lease. + client1.setState(Dhcp4Client::RENEWING); + testAssigned([this, &client1]() { + doRequest(client1, "192.0.2.28"); + }); + + // Client #2 renews the lease. + client2.setState(Dhcp4Client::RENEWING); + // Check if the client successfully renewed its address and that the + // subnet id in the renewed lease is not messed up. + testAssigned([this, &client2]() { + doRequest(client2, "10.0.0.1"); + }); +} + // Reserved address can't be assigned as long as access to a subnet is // restricted by classification. TEST_F(Dhcpv4SharedNetworkTest, reservationAccessRestrictedByClass) { diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 085da6c697..ff68d37b5b 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -3816,12 +3816,14 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { // If the client is requesting an address which is assigned to the client // let's just renew this address. Also, renew this address if the client // doesn't request any specific address. - // Added extra checks: the address is reserved or belongs to the dynamic - // pool for the case the pool class has changed before the request. + // Added extra checks: the address is reserved for this client or belongs + // to the dynamic pool for the case the pool class has changed before the + // request. if (client_lease) { if (((client_lease->addr_ == ctx.requested_address_) || ctx.requested_address_.isV4Zero()) && - (hasAddressReservation(ctx) || + ((hasAddressReservation(ctx) && + (ctx.currentHost()->getIPv4Reservation() == ctx.requested_address_)) || inAllowedPool(ctx, client_lease->addr_))) { LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE, -- 2.47.2