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 D97BD1FF0C0 for ; Wed, 21 Oct 2026 10:46:00 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id C7A3220493; Wed, 21 Oct 2026 10:45:48 +0200 (CEST) From: Hannes Laimer To: pve-devel@lists.proxmox.com Subject: [PATCH pve-network v4 06/20] sdn: ipam: key the MAC cache by lower-case MAC Date: Wed, 21 Oct 2026 10:44:31 +0200 Message-ID: <20261021084445.462195-7-h.laimer@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261021084445.462195-1-h.laimer@proxmox.com> References: <20261021084445.462195-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: 1792572307613 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.373 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: EUZMTRGXORAJZ22O3WYJMNTMWRAAU4PB X-Message-ID-Hash: EUZMTRGXORAJZ22O3WYJMNTMWRAAU4PB 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. The cache is now read with every MAC in lower case and written under the lower-case MAC. An entry cached before this change under another spelling is read under the lower-case key and leaves the file with the next write. Two spellings that disagree on an address are both dropped on the read, since one of them is stale and the cache cannot tell which. The next lookup then asks the IPAM. The dnsmasq reservation file is written and read with its MACs in lower case as well, since its lines from before keep their writer's spelling. Signed-off-by: Hannes Laimer --- Notes: v4: * fold the spelling handling into the cache read * split the miss and lock fixes into their own commits src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 3 ++ src/PVE/Network/SDN/Ipams.pm | 39 ++++++++++---- src/test/run_test_vnets_blackbox.pl | 82 +++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 11 deletions(-) diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm index 477b700f..d3b78fe9 100644 --- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm +++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm @@ -53,6 +53,7 @@ sub update_lease { sub add_ip_mapping { my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_; + $mac = lc($mac); my $ethers_file = ethers_file($dhcpid); my $ethers_tmp_file = "$ethers_file.tmp"; @@ -76,6 +77,8 @@ sub add_ip_mapping { warn "failed to parse MAC from $dhcpid ethers file on line $line_no: '$line'\n"; next; } + # the cache is keyed in lower case, a line from before keeps its writer's spelling + $parsed_mac = lc($parsed_mac); my ($parsed_ip4, $parsed_ip6); if ($parsed_ip2) { diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm index 3d849d79..0e795a93 100644 --- a/src/PVE/Network/SDN/Ipams.pm +++ b/src/PVE/Network/SDN/Ipams.pm @@ -23,17 +23,31 @@ PVE::Network::SDN::Ipams::Plugin->init(); my $macdb_filename = "sdn/mac-cache.json"; cfs_register_file($macdb_filename, \&json_reader, \&json_writer); -# an entry without an address was left behind by a lookup that missed, and -# says nothing about its MAC +# the file may hold a MAC under more than one spelling, and an entry without +# an address says nothing about its MAC. Two spellings that disagree on an +# address are both dropped, since one of them is stale and nothing here says +# which. The next lookup then asks the IPAM +my sub fold_macdb { + my ($db) = @_; + + my $macs = {}; + my %clash; + for my $key (sort keys %{ $db->{macs} // {} }) { + my $entry = $db->{macs}->{$key}; + for my $family (grep { defined($entry->{$_}) } qw(ip4 ip6)) { + my $seen = $macs->{ lc($key) }->{$family}; + $clash{ lc($key) } = 1 if defined($seen) && lc($seen) ne lc($entry->{$family}); + $macs->{ lc($key) }->{$family} //= $entry->{$family}; + } + } + delete $macs->{$_} for keys %clash; + return { %$db, macs => $macs }; +} + sub json_reader { my ($filename, $data) = @_; - my $db = defined($data) && length($data) > 0 ? decode_json($data) : {}; - for my $mac (keys %{ $db->{macs} // {} }) { - my $entry = $db->{macs}->{$mac}; - delete $db->{macs}->{$mac} if !defined($entry->{ip4}) && !defined($entry->{ip6}); - } - return $db; + return fold_macdb(defined($data) && length($data) > 0 ? decode_json($data) : {}); } sub json_writer { @@ -54,8 +68,10 @@ sub write_macdb { cfs_write_file($macdb_filename, $data); } +# 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, @@ -75,6 +91,7 @@ sub add_cache_mac_ip { sub del_cache_mac_ip { my ($mac, $ip) = @_; + $mac = lc($mac); cfs_lock_file( $macdb_filename, @@ -142,7 +159,7 @@ sub complete_sdn_vnet { sub get_ips_from_mac { my ($mac, $zoneid, $zone) = @_; - my $cached = read_macdb()->{macs}->{$mac}; + my $cached = read_macdb()->{macs}->{ lc($mac) }; return ($cached->{ip4}, $cached->{ip6}) if $cached; my $plugin_config = get_plugin_config($zone); @@ -157,8 +174,8 @@ sub get_ips_from_mac { undef, sub { my $db = read_macdb(); - $db->{macs}->{$mac}->{ip4} = $ip4 if defined($ip4); - $db->{macs}->{$mac}->{ip6} = $ip6 if defined($ip6); + $db->{macs}->{ lc($mac) }->{ip4} = $ip4 if defined($ip4); + $db->{macs}->{ lc($mac) }->{ip6} = $ip6 if defined($ip6); write_macdb($db); }, ); diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl index d8e35231..6b89778f 100755 --- a/src/test/run_test_vnets_blackbox.pl +++ b/src/test/run_test_vnets_blackbox.pl @@ -471,6 +471,88 @@ sub test_ipam_cache_misses { 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", + ); + + # two spellings that disagree are not trusted, the lookup asks the IPAM + $test_state->{macdb}->{macs} = { + uc($mac) => { ip4 => '10.0.0.150' }, + $mac => { ip4 => '10.0.0.151' }, + }; + @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); + is($ips[0], '10.0.0.160', "$test_name: a clash of spellings counts as a miss"); + is_deeply( + $test_state->{macdb}->{macs}, + { $mac => { ip4 => '10.0.0.160' } }, + "$test_name: and its write leaves the IPAM's answer alone", + ); +} + +run_test(\&test_ipam_cache_case); + sub test_nic_join { my ($test_name, $subnets) = @_; -- 2.47.3