Re: [PATCH pve-network v3 06/17] sdn: ipam: key the MAC cache by lower-case MAC, stop caching misses

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 10/20/26 10:01 AM, Hannes Laimer wrote:
> 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 <h.laimer@proxmox.com>
> ---
>  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;

would it make sense to move all of this case handling into read_macdb itself? Currently
we're doing it at every call site of read_macdb in Ipams.pm

> 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";