* [PATCH pve-firewall 1/2] firewall: ebtables: resolve aliases in ipfilter ipsets
2026-10-12 9:38 [PATCH pve-firewall 0/2] ebtables: fix ARP filter for alias and nomatch ipfilter entries Arthur Bied-Charreton
@ 2026-10-12 9:38 ` Arthur Bied-Charreton
2026-10-12 9:38 ` [PATCH pve-firewall 2/2] firewall: ebtables: honor nomatch " Arthur Bied-Charreton
1 sibling, 0 replies; 3+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-12 9:38 UTC (permalink / raw)
To: pve-devel
Entries of an ipfilter-netX ipset were passed straight to
parse_ip_or_cidr() when building the ARP filter, so an alias entry or
one the parser already flagged as invalid, made it die.
The error dropped all ebtables rules for the guest, including the MAC and
ARP source filters, and was logged on every update cycle without saying
which guest was affected.
Resolve entries the same way generate_ipset_chains() does, skip
erroneous ones, and log resolution errors per entry with context.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall.pm | 70 +++++++++++++++---------
src/test/Makefile | 2 +-
src/test/test_ebtables_ipfilter.pl | 85 ++++++++++++++++++++++++++++++
3 files changed, 132 insertions(+), 25 deletions(-)
create mode 100755 src/test/test_ebtables_ipfilter.pl
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index da6976e..bb3350b 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -4074,6 +4074,16 @@ sub generate_std_chains {
}
}
+my sub resolve_ipset_entry {
+ my ($cluster_conf, $fw_conf, $cidr) = @_;
+
+ if ($cidr =~ m@^(dc/|guest/)?(${ip_alias_pattern})$@) {
+ return resolve_alias($cluster_conf, $fw_conf, $2, $1 // "");
+ }
+
+ return parse_ip_or_cidr($cidr);
+}
+
sub generate_ipset_chains {
my ($ipset_ruleset, $clusterfw_conf, $fw_conf, $device_ips, $ipsets) = @_;
@@ -4092,14 +4102,8 @@ sub generate_ipset_chains {
foreach my $entry (@$options) {
next if $entry->{errors}; # skip entries with errors
eval {
- my ($cidr, $ver);
- if ($entry->{cidr} =~ m@^(dc/|guest/)?(${ip_alias_pattern})$@) {
- my $scope = $1 // "";
- my $alias = $2;
- ($cidr, $ver) = resolve_alias($clusterfw_conf, $fw_conf, $alias, $scope);
- } else {
- ($cidr, $ver) = parse_ip_or_cidr($entry->{cidr});
- }
+ my ($cidr, $ver) =
+ resolve_ipset_entry($clusterfw_conf, $fw_conf, $entry->{cidr});
#http://backreference.org/2013/03/01/ipv6-address-normalization/
if ($ver == 6) {
# ip_compress_address takes an address only, no CIDR
@@ -4810,6 +4814,32 @@ sub compile_ipsets {
return $ipset_ruleset;
}
+my sub resolve_arpfilter {
+ my ($ipset, $cluster_conf, $vm_conf, $vmid, $netid) = @_;
+ return [] if !defined($ipset);
+
+ my $arpfilter = [];
+ for my $entry ($ipset->@*) {
+ next if $entry->{errors};
+
+ my ($ip, $ver) = eval { resolve_ipset_entry($cluster_conf, $vm_conf, $entry->{cidr}) };
+ if ($@) {
+ my $err = "$vmid: ipfilter-$netid: skipping '$entry->{cidr}': $@";
+ if ($verbose) {
+ warn $err;
+ } else {
+ syslog('warn', $err);
+ }
+ next;
+ }
+
+ next if $ver != 4;
+ push $arpfilter->@*, $ip;
+ }
+
+ return $arpfilter;
+}
+
sub compile_ebtables_filter {
my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata) = @_;
@@ -4842,14 +4872,10 @@ sub compile_ebtables_filter {
my $iface = "tap${vmid}i$1";
next if !valid_interface_name($iface);
my $macaddr = $net->{macaddr};
- my $arpfilter = [];
- if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) {
- foreach my $ipaddr (@$ipset) {
- my ($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr});
- next if !$ip || ($version && $version != 4);
- push(@$arpfilter, $ip);
- }
- }
+ my $arpfilter = resolve_arpfilter(
+ $ipsets->{"ipfilter-$netid"},
+ $cluster_conf, $vmfw_conf, $vmid, $netid,
+ );
generate_tap_layer2filter(
$ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter,
);
@@ -4881,14 +4907,10 @@ sub compile_ebtables_filter {
my $iface = "veth${vmid}i$1";
next if !valid_interface_name($iface);
my $macaddr = $net->{hwaddr};
- my $arpfilter = [];
- if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) {
- foreach my $ipaddr (@$ipset) {
- my ($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr});
- next if !$ip || ($version && $version != 4);
- push(@$arpfilter, $ip);
- }
- }
+ my $arpfilter = resolve_arpfilter(
+ $ipsets->{"ipfilter-$netid"},
+ $cluster_conf, $vmfw_conf, $vmid, $netid,
+ );
if (defined(my $ip = $net->{ip}) && $vmfw_conf->{options}->{ipfilter}) {
# ebtables changes this to a .0/MASK network but we just
# want the address here, no network - see #2193
diff --git a/src/test/Makefile b/src/test/Makefile
index 272a19a..74f9e35 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -1,4 +1,4 @@
-TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_simulator_protocols test_interface_names test_restore_errors
+TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_ebtables_ipfilter test_simulator_protocols test_interface_names test_restore_errors
.PHONY: all test $(TESTS) clean
all: test
diff --git a/src/test/test_ebtables_ipfilter.pl b/src/test/test_ebtables_ipfilter.pl
new file mode 100755
index 0000000..9716ac2
--- /dev/null
+++ b/src/test/test_ebtables_ipfilter.pl
@@ -0,0 +1,85 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::More;
+
+use PVE::Firewall;
+
+plan(skip_all => 'PVE::QemuServer::Network is not installed')
+ if !eval { require PVE::QemuServer::Network; 1 };
+
+PVE::Firewall::set_verbose(1);
+
+sub compile_arp_sources {
+ my ($entries) = @_;
+
+ my $cluster = {
+ options => {},
+ aliases => { dcalias => { name => 'dcalias', cidr => '192.0.2.2', ipversion => 4 } },
+ };
+ my $vmfw_conf = {
+ vmid => 100,
+ options => { enable => 1 },
+ aliases =>
+ { guestalias => { name => 'guestalias', cidr => '192.0.2.1', ipversion => 4 } },
+ ipset => { 'ipfilter-net0' => $entries },
+ };
+ my $vmdata = {
+ qemu => { 100 => { net0 => 'virtio=02:00:00:00:00:01,bridge=vmbr0,firewall=1' } },
+ lxc => {},
+ };
+
+ my @warnings;
+ local $SIG{__WARN__} = sub { push @warnings, @_ };
+
+ my $ruleset =
+ PVE::Firewall::compile_ebtables_filter($cluster, {}, { 100 => $vmfw_conf }, $vmdata);
+
+ ok(exists($ruleset->{'tap100i0-OUT'}), 'guest keeps its layer-2 filter');
+ my @sources = map { /--arp-ip-src (\S+)/ ? $1 : () } ($ruleset->{'tap100i0-OUT-ARP'} // [])->@*;
+
+ return (\@sources, \@warnings);
+}
+
+subtest 'aliases are resolved' => sub {
+ my ($sources, $warnings) = compile_arp_sources([
+ { cidr => '192.0.2.10' },
+ { cidr => 'guestalias' },
+ { cidr => 'guest/guestalias' },
+ { cidr => 'dc/dcalias' },
+ { cidr => 'dcalias' },
+ ]);
+ is_deeply(
+ $sources,
+ ['192.0.2.10', '192.0.2.1', '192.0.2.1', '192.0.2.2', '192.0.2.2'],
+ 'ARP filter contains resolved addresses',
+ );
+ is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'entries flagged by the parser are skipped' => sub {
+ my ($sources, $warnings) = compile_arp_sources([
+ { cidr => 'c', errors => { cidr => 'invalid' } }, { cidr => '192.0.2.10' },
+ ]);
+ is_deeply($sources, ['192.0.2.10'], 'valid entry is kept');
+ is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'unresolvable entries are skipped with context' => sub {
+ my ($sources, $warnings) = compile_arp_sources([
+ { cidr => 'missing' }, { cidr => '192.0.2.10' },
+ ]);
+ is_deeply($sources, ['192.0.2.10'], 'valid entry is kept');
+ is(scalar($warnings->@*), 1, 'one warning');
+ like(
+ $warnings->[0] // '',
+ qr/^100: ipfilter-net0: skipping 'missing': /,
+ 'warning names guest, ipset and entry',
+ );
+};
+
+done_testing();
--
2.47.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH pve-firewall 2/2] firewall: ebtables: honor nomatch in ipfilter ipsets
2026-10-12 9:38 [PATCH pve-firewall 0/2] ebtables: fix ARP filter for alias and nomatch ipfilter entries Arthur Bied-Charreton
2026-10-12 9:38 ` [PATCH pve-firewall 1/2] firewall: ebtables: resolve aliases in ipfilter ipsets Arthur Bied-Charreton
@ 2026-10-12 9:38 ` Arthur Bied-Charreton
1 sibling, 0 replies; 3+ messages in thread
From: Arthur Bied-Charreton @ 2026-10-12 9:38 UTC (permalink / raw)
To: pve-devel
The ARP filter turnes every ipfilter-netX entry into a RETURN rule,
including nomatch entries. An address excluded from an allowed network
with "!" is therefore still accepted as an ARP source.
Emit DROP for nomatch entries and order the rules by prefix length. The
rules are sorted most-specific first, and equal prefix lengths keep
their original order.
Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
src/PVE/Firewall.pm | 18 ++++++++++---
src/test/test_ebtables_ipfilter.pl | 42 ++++++++++++++++++++++++++++--
2 files changed, 54 insertions(+), 6 deletions(-)
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index bb3350b..9630d58 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -4834,7 +4834,7 @@ my sub resolve_arpfilter {
}
next if $ver != 4;
- push $arpfilter->@*, $ip;
+ push $arpfilter->@*, { cidr => $ip, nomatch => $entry->{nomatch} };
}
return $arpfilter;
@@ -4916,7 +4916,7 @@ sub compile_ebtables_filter {
# want the address here, no network - see #2193
$ip =~ s|/(\d+)$||;
if ($ip ne 'dhcp') {
- push @$arpfilter, $ip;
+ push $arpfilter->@*, { cidr => $ip };
}
}
generate_tap_layer2filter(
@@ -4958,8 +4958,18 @@ sub generate_tap_layer2filter {
ruleset_addrule($ruleset, $tapchain, "-p ARP", "-j $arpchain");
ruleset_create_chain($ruleset, $arpchain);
- foreach my $ip (@{$arpfilter}) {
- ruleset_addrule($ruleset, $arpchain, "-p ARP --arp-ip-src $ip", '-j RETURN');
+ my $prefix_len = sub { $arpfilter->[$_[0]]->{cidr} =~ m|/(\d+)$| ? $1 : 32 };
+ my @order =
+ sort { $prefix_len->($b) <=> $prefix_len->($a) || $a <=> $b } 0 .. $arpfilter->$#*;
+
+ for my $entry ($arpfilter->@[@order]) {
+ my $target = $entry->{nomatch} ? 'DROP' : 'RETURN';
+ ruleset_addrule(
+ $ruleset,
+ $arpchain,
+ "-p ARP --arp-ip-src $entry->{cidr}",
+ "-j $target",
+ );
}
ruleset_addrule($ruleset, $arpchain, '', '-j DROP');
}
diff --git a/src/test/test_ebtables_ipfilter.pl b/src/test/test_ebtables_ipfilter.pl
index 9716ac2..069ee6c 100755
--- a/src/test/test_ebtables_ipfilter.pl
+++ b/src/test/test_ebtables_ipfilter.pl
@@ -40,9 +40,11 @@ sub compile_arp_sources {
PVE::Firewall::compile_ebtables_filter($cluster, {}, { 100 => $vmfw_conf }, $vmdata);
ok(exists($ruleset->{'tap100i0-OUT'}), 'guest keeps its layer-2 filter');
- my @sources = map { /--arp-ip-src (\S+)/ ? $1 : () } ($ruleset->{'tap100i0-OUT-ARP'} // [])->@*;
+ my $arp_rules = $ruleset->{'tap100i0-OUT-ARP'} // [];
+ my @sources = map { /--arp-ip-src (\S+) -j RETURN/ ? $1 : () } $arp_rules->@*;
+ my @rules = map { /--arp-ip-src (\S+) -j (\S+)/ ? "$2 $1" : () } $arp_rules->@*;
- return (\@sources, \@warnings);
+ return (\@sources, \@warnings, \@rules);
}
subtest 'aliases are resolved' => sub {
@@ -82,4 +84,40 @@ subtest 'unresolvable entries are skipped with context' => sub {
);
};
+subtest 'nomatch entries are excluded' => sub {
+ my (undef, $warnings, $rules) = compile_arp_sources([
+ { cidr => '192.0.2.0/24' }, { cidr => '192.0.2.5', nomatch => 1 },
+ ]);
+ is_deeply(
+ $rules,
+ ['DROP 192.0.2.5', 'RETURN 192.0.2.0/24'],
+ 'excluded address is dropped before the allowed network',
+ );
+ is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'more specific entries take precedence over nomatch networks' => sub {
+ my (undef, $warnings, $rules) = compile_arp_sources([
+ { cidr => '192.0.2.0/24', nomatch => 1 }, { cidr => '192.0.2.5' },
+ ]);
+ is_deeply(
+ $rules,
+ ['RETURN 192.0.2.5', 'DROP 192.0.2.0/24'],
+ 'allowed address is returned before the excluded network',
+ );
+ is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'entries with the same prefix length keep their order' => sub {
+ my (undef, $warnings, $rules) = compile_arp_sources([
+ { cidr => '192.0.2.20' }, { cidr => '192.0.2.0/24' }, { cidr => '192.0.2.3' },
+ ]);
+ is_deeply(
+ $rules,
+ ['RETURN 192.0.2.20', 'RETURN 192.0.2.3', 'RETURN 192.0.2.0/24'],
+ 'ties are not reordered',
+ );
+ is_deeply($warnings, [], 'no warnings');
+};
+
done_testing();
--
2.47.3
^ permalink raw reply related [flat|nested] 3+ messages in thread