From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 94E841FF0AA for ; Tue, 20 Oct 2026 10:01:16 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 568F7216DB; Tue, 20 Oct 2026 10:00:27 +0200 (CEST) From: Hannes Laimer To: pve-devel@lists.proxmox.com Subject: [PATCH pve-network v3 06/17] sdn: ipam: key the MAC cache by lower-case MAC, stop caching misses Date: Tue, 20 Oct 2026 09:59:54 +0200 Message-ID: <20261020080005.470757-7-h.laimer@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261020080005.470757-1-h.laimer@proxmox.com> References: <20261020080005.470757-1-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792483215743 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.421 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 3OM63QETR5JYHSELBMUXZAAYJOBGHQ6O X-Message-ID-Hash: 3OM63QETR5JYHSELBMUXZAAYJOBGHQ6O X-MailFrom: h.laimer@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Generated guest MACs are upper case, but the API accepts either spelling. The cache was keyed by the MAC as its writer spelled it. A deletion in the other spelling missed the entry, so the cache kept answering with the deleted address. Entries cached before this change keep their spelling. A lookup and a deletion still find them, and the next write for their MAC moves them to the lower-case key. A lookup that missed the cache stored the IPAM's empty answer, and any entry counted as a hit. So a record created later directly in the IPAM stayed hidden, and a guest start allocated another address. An empty answer is no longer stored, and an entry without an address counts as a miss. The lookup also wrote the whole cache back without the cluster lock the other writers take. A change made between its read and its write was lost. The lookup now takes the lock and reads the cache again before it stores its answer. Signed-off-by: Hannes Laimer --- src/PVE/Network/SDN/Ipams.pm | 61 +++++++++++--- src/test/run_test_vnets_blackbox.pl | 118 ++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 11 deletions(-) diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm index 179bdf76..d8051d8d 100644 --- a/src/PVE/Network/SDN/Ipams.pm +++ b/src/PVE/Network/SDN/Ipams.pm @@ -47,14 +47,27 @@ sub write_macdb { cfs_write_file($macdb_filename, $data); } +# an entry under another spelling moves to the lower-case key +my sub fold_spellings { + my ($db, $mac) = @_; + + for my $key (grep { $_ ne $mac && lc($_) eq $mac } reverse sort keys $db->{macs}->%*) { + my $old = delete $db->{macs}->{$key}; + $db->{macs}->{$mac}->{$_} //= $old->{$_} for grep { defined($old->{$_}) } qw(ip4 ip6); + } +} + +# the cache is keyed by the MAC in lower case, whatever spelling its writers use sub add_cache_mac_ip { my ($mac, $ip) = @_; + $mac = lc($mac); cfs_lock_file( $macdb_filename, undef, sub { my $db = read_macdb(); + fold_spellings($db, $mac); if (Net::IP::ip_is_ipv4($ip)) { $db->{macs}->{$mac}->{ip4} = $ip; } else { @@ -74,13 +87,17 @@ sub del_cache_mac_ip { undef, sub { my $db = read_macdb(); - if (Net::IP::ip_is_ipv4($ip)) { - delete $db->{macs}->{$mac}->{ip4}; - } else { - delete $db->{macs}->{$mac}->{ip6}; + # entries from before the cache was keyed in lower case carry their writer's spelling + for my $key (grep { lc($_) eq lc($mac) } keys $db->{macs}->%*) { + if (Net::IP::ip_is_ipv4($ip)) { + delete $db->{macs}->{$key}->{ip4}; + } else { + delete $db->{macs}->{$key}->{ip6}; + } + delete $db->{macs}->{$key} + if !defined($db->{macs}->{$key}->{ip4}) + && !defined($db->{macs}->{$key}->{ip6}); } - delete $db->{macs}->{$mac} - if !defined($db->{macs}->{$mac}->{ip4}) && !defined($db->{macs}->{$mac}->{ip6}); write_macdb($db); }, ); @@ -136,16 +153,38 @@ sub get_ips_from_mac { my ($mac, $zoneid, $zone) = @_; my $macdb = read_macdb(); - return ($macdb->{macs}->{$mac}->{ip4}, $macdb->{macs}->{$mac}->{ip6}) if $macdb->{macs}->{$mac}; + # entries from before the cache was keyed in lower case carry their writer's + # spelling, and the lower-case one wins where two hold the same family + my %cached; + for my $key (grep { lc($_) eq lc($mac) } sort keys $macdb->{macs}->%*) { + my $entry = $macdb->{macs}->{$key}; + $cached{$_} = $entry->{$_} for grep { defined($entry->{$_}) } qw(ip4 ip6); + } + + # an entry holding no address says nothing about the MAC, so ask the IPAM + return ($cached{ip4}, $cached{ip6}) if defined($cached{ip4}) || defined($cached{ip6}); my $plugin_config = get_plugin_config($zone); my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type}); - ($macdb->{macs}->{$mac}->{ip4}, $macdb->{macs}->{$mac}->{ip6}) = - $plugin->get_ips_from_mac($plugin_config, $mac, $zoneid); + my ($ip4, $ip6) = $plugin->get_ips_from_mac($plugin_config, $mac, $zoneid); + + # an empty answer is not cached, since the record may simply not exist yet + return if !defined($ip4) && !defined($ip6); - write_macdb($macdb); + cfs_lock_file( + $macdb_filename, + undef, + sub { + my $db = read_macdb(); + fold_spellings($db, lc($mac)); + $db->{macs}->{ lc($mac) }->{ip4} = $ip4 if defined($ip4); + $db->{macs}->{ lc($mac) }->{ip6} = $ip6 if defined($ip6); + write_macdb($db); + }, + ); + warn "$@" if $@; - return ($macdb->{macs}->{$mac}->{ip4}, $macdb->{macs}->{$mac}->{ip6}); + return ($ip4, $ip6); } 1; diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl index d324538e..476e7f8d 100755 --- a/src/test/run_test_vnets_blackbox.pl +++ b/src/test/run_test_vnets_blackbox.pl @@ -1118,6 +1118,124 @@ sub test_dnsmasq_mapping_push { run_test(\&test_dnsmasq_mapping_push); +sub test_ipam_cache_misses { + my $test_name = (split(/::/, (caller(0))[3]))[-1]; + my $zoneid = "TESTZONE"; + my $vnetid = "testvnet"; + my $mac = "da:65:8f:18:9b:6f"; + + create_zone({ + type => "simple", + dhcp => "dnsmasq", + ipam => "pve", + zone => $zoneid, + }); + create_vnet({ + type => "vnet", + zone => $zoneid, + vnet => $vnetid, + }); + create_subnet({ + type => "subnet", + vnet => $vnetid, + subnet => "10.0.0.0/24", + gateway => "10.0.0.1", + 'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"], + }); + + my @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); + is(scalar(grep { defined } @ips), 0, "$test_name: an unknown MAC has no answer"); + ok(!exists $test_state->{macdb}->{macs}->{$mac}, "$test_name: the miss is not cached"); + + # an entry holding no address, left behind by an earlier miss, does not + # hide a record created since + create_ip({ + zone => $zoneid, + vnet => $vnetid, + mac => $mac, + ip => "10.0.0.50", + }); + $test_state->{macdb}->{macs}->{$mac} = { ip4 => undef, ip6 => undef }; + @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); + is($ips[0], "10.0.0.50", "$test_name: an address-less entry counts as a miss"); + is( + $test_state->{macdb}->{macs}->{$mac}->{ip4}, + "10.0.0.50", + "$test_name: the answer is cached", + ); +} + +run_test(\&test_ipam_cache_misses); + +sub test_ipam_cache_case { + my $test_name = (split(/::/, (caller(0))[3]))[-1]; + my $zoneid = "TESTZONE"; + my $vnetid = "testvnet"; + my $mac = "da:65:8f:18:9b:6f"; + + # an entry written under the config's spelling before the cache was keyed in + # lower case is read and released like any other + create_zone({ + type => "simple", + dhcp => "dnsmasq", + ipam => "pve", + zone => $zoneid, + }); + create_vnet({ + type => "vnet", + zone => $zoneid, + vnet => $vnetid, + }); + create_subnet({ + type => "subnet", + vnet => $vnetid, + subnet => "10.0.0.0/24", + gateway => "10.0.0.1", + 'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"], + }); + $test_state->{macdb}->{macs}->{ uc($mac) } = { ip4 => '10.0.0.150' }; + my ($ip4) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); + is($ip4, '10.0.0.150', "$test_name: an old entry is found however it is spelled"); + PVE::Network::SDN::Ipams::del_cache_mac_ip($mac, '10.0.0.150'); + ok( + !(grep { lc($_) eq $mac } keys $test_state->{macdb}->{macs}->%*), + "$test_name: and released", + ); + + # a write spelled either way moves such an entry to the lower-case key + # and keeps what it held + $test_state->{macdb}->{macs}->{ uc($mac) } = { ip4 => '10.0.0.150' }; + PVE::Network::SDN::Ipams::add_cache_mac_ip(uc($mac), 'fd00::150'); + is_deeply( + [grep { lc($_) eq $mac } keys $test_state->{macdb}->{macs}->%*], + [$mac], + "$test_name: a write leaves one entry for the MAC", + ); + is_deeply( + $test_state->{macdb}->{macs}->{$mac}, + { ip4 => '10.0.0.150', ip6 => 'fd00::150' }, + "$test_name: holding both addresses", + ); + + # the lookup's write moves an address-less entry under another spelling as well + create_ip({ + zone => $zoneid, + vnet => $vnetid, + mac => $mac, + ip => "10.0.0.160", + }); + $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => undef, ip6 => undef } }; + my @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); + is($ips[0], '10.0.0.160', "$test_name: a lookup past an address-less entry asks the IPAM"); + is_deeply( + [sort keys $test_state->{macdb}->{macs}->%*], + [$mac], + "$test_name: and its write leaves one entry for the MAC", + ); +} + +run_test(\&test_ipam_cache_case); + sub test_dnsmasq_dual_stack_and_sweep { my $test_name = (split(/::/, (caller(0))[3]))[-1]; my $zoneid = "TESTZONE"; -- 2.47.3