From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 03F361FF17C for ; Wed, 23 Jul 2025 16:20:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 174BC14B6B; Wed, 23 Jul 2025 16:21:41 +0200 (CEST) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Wed, 23 Jul 2025 16:21:06 +0200 Message-Id: <20250723142106.235104-1-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.39.5 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.196 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RDNS_NONE 0.793 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [list-interfaces.pl, base-auto-allow-hotplug.pl, keep-option-order.pl, unhandled-interfaces-to-manual.pl, runtest.pl, inotify.pm] Subject: [pve-devel] [PATCH pve-common 1/1] inotify/interfaces: use 'ip link' instead of /proc/net/dev X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" The function reading /etc/network/interfaces used /proc/net/dev to determine pre-existing physical interfaces. Since the introduction of altnames, /proc/net/dev returns insufficient information for determining if an interface is already contained in /e/n/i, since it does not include altnames. The interfaces parser added all interfaces from /proc/net/dev as configurable network devices. If altnames were used in the configuration, then the same interface would be listed twice: once with its 'real' name (from /proc/net/dev) and once with its altname (from the interfaces file). Adapt the interfaces file parser to utilize 'ip link' instead of /proc/net/dev as its source of truth regarding configured network devices. The parser now only adds a physical interface if neither its 'real' name nor any of its altnames are contained in /etc/network/interfaces. This patch makes the assumption that a interface is always referred to by either its name or one of its altnames, but not mixed (e.g. the stanza uses the 'real' name but bridge_ports uses the altname). In its current state this will already lead to problems with validation, since we do not consider altnames when looking up the availability of bridge ports or VLAN parent devices for instance. As long as our tooling is used, this assumption should always hold, it can only be broken by manually editing the interfaces file. There is one special case still where two entries exist for a single network interface: If a pinning is generated by proxmox-network-interface-pinning, but not yet applied. In this case, the UI will show both network interfaces (nicX and ethX). It will also show the pending changes that replace ethX with nicX. A possible solution for improving this case could look at the existing .link files, check if any not-yet-applied pin exists there and use the pinned name as the 'real' name when parsing the interfaces file. Signed-off-by: Stefan Hanreich --- src/PVE/INotify.pm | 31 ++++++++++++------- test/etc_network_interfaces/ip_link_details | 20 ++++++++++++ test/etc_network_interfaces/proc_net_dev | 5 --- test/etc_network_interfaces/runtest.pl | 7 ++--- .../t.base-auto-allow-hotplug.pl | 14 ++++++--- .../t.create_network.pl | 27 +++++++++------- .../t.keep-option-order.pl | 21 ++++++++----- .../t.list-interfaces.pl | 27 ++++++++++------ .../t.ovs_bridge_allow.pl | 25 +++++++++------ .../t.unhandled-interfaces-to-manual.pl | 28 ++++++++--------- .../etc_network_interfaces/t.unknown_order.pl | 14 ++++++++- 11 files changed, 138 insertions(+), 81 deletions(-) create mode 100644 test/etc_network_interfaces/ip_link_details delete mode 100644 test/etc_network_interfaces/proc_net_dev diff --git a/src/PVE/INotify.pm b/src/PVE/INotify.pm index dbad08c..93f8f2d 100644 --- a/src/PVE/INotify.pm +++ b/src/PVE/INotify.pm @@ -866,13 +866,13 @@ my $check_mtu = sub { # } sub read_etc_network_interfaces { my ($filename, $fh) = @_; - my $proc_net_dev = IO::File->new('/proc/net/dev', 'r'); + my $ip_links = PVE::Network::ip_link_details(); my $active = PVE::ProcFSTools::get_active_network_interfaces(); - return __read_etc_network_interfaces($fh, $proc_net_dev, $active); + return __read_etc_network_interfaces($fh, $ip_links, $active); } sub __read_etc_network_interfaces { - my ($fh, $proc_net_dev, $active_ifaces) = @_; + my ($fh, $ip_links, $active_ifaces) = @_; my $config = {}; my $ifaces = $config->{ifaces} = {}; @@ -894,15 +894,6 @@ sub __read_etc_network_interfaces { my $line; - if ($proc_net_dev) { - while (defined($line = <$proc_net_dev>)) { - if ($line =~ m/^\s*($PVE::Network::PHYSICAL_NIC_RE):.*/) { - $ifaces->{$1}->{exists} = 1; - } - } - close($proc_net_dev); - } - # we try to keep order inside the file my $priority = 2; # 1 is reserved for lo @@ -1047,6 +1038,22 @@ SECTION: while (defined($line = <$fh>)) { } } + OUTER: + for my $iface_name (keys $ip_links->%*) { + my $ip_link = $ip_links->{$iface_name}; + + next if $iface_name !~ m/^$PVE::Network::PHYSICAL_NIC_RE$/; + + for my $altname ($ip_link->{altnames}->@*) { + if ($ifaces->{$altname}) { + $ifaces->{$altname}->{exists} = 1; + next OUTER; + } + } + + $ifaces->{$iface_name}->{exists} = 1; + } + foreach my $ifname (@$active_ifaces) { if (my $iface = $ifaces->{$ifname}) { $iface->{active} = 1; diff --git a/test/etc_network_interfaces/ip_link_details b/test/etc_network_interfaces/ip_link_details new file mode 100644 index 0000000..93b5bf6 --- /dev/null +++ b/test/etc_network_interfaces/ip_link_details @@ -0,0 +1,20 @@ +{ + "lo": { + "ifindex": 1, + "ifname": "lo", + "link_type": "loopback" + }, + "eth0": { + "ifindex": 2, + "ifname": "eth0", + "link_type": "ether" + }, + "vmbr0": { + "ifindex": 3, + "ifname": "vmbr0", + "link_type": "ether", + "linkinfo": { + "info_kind": "bridge" + } + } +} diff --git a/test/etc_network_interfaces/proc_net_dev b/test/etc_network_interfaces/proc_net_dev deleted file mode 100644 index 01df936..0000000 --- a/test/etc_network_interfaces/proc_net_dev +++ /dev/null @@ -1,5 +0,0 @@ -Inter-| Receive | Transmit - face |bytes packets errs drop fifo frame compressed multicast|bytes packets errs drop fifo colls carrier compressed - vmbr0: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 - eth0: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 - lo: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 diff --git a/test/etc_network_interfaces/runtest.pl b/test/etc_network_interfaces/runtest.pl index 814b331..b5ef2fd 100755 --- a/test/etc_network_interfaces/runtest.pl +++ b/test/etc_network_interfaces/runtest.pl @@ -65,12 +65,11 @@ sub flush_files() { # Read an interfaces file with optional /proc/net/dev file content string and # the list of active interfaces, which otherwise default sub r($;$$) { - my ($ifaces, $proc_net_dev, $active) = @_; - $proc_net_dev //= load('proc_net_dev'); + my ($ifaces, $ip_links, $active) = @_; + $ip_links //= decode_json(load('ip_link_details')); $active //= [split(/\s+/, load('active_interfaces'))]; open my $fh1, '<', \$ifaces; - open my $fh2, '<', \$proc_net_dev; - $config = PVE::INotify::__read_etc_network_interfaces($fh1, $fh2, $active); + $config = PVE::INotify::__read_etc_network_interfaces($fh1, $ip_links, $active); close $fh1; } diff --git a/test/etc_network_interfaces/t.base-auto-allow-hotplug.pl b/test/etc_network_interfaces/t.base-auto-allow-hotplug.pl index 772da83..628146c 100644 --- a/test/etc_network_interfaces/t.base-auto-allow-hotplug.pl +++ b/test/etc_network_interfaces/t.base-auto-allow-hotplug.pl @@ -1,23 +1,27 @@ +use JSON; + my $active_ifaces = ['lo', 'ens18', 'ens']; -my $proc_net = load('proc_net_dev'); -$proc_net =~ s/eth0/ens18/; + +my $ip_links = decode_json(load('ip_link_details')); +$ip_links->{ens18} = delete $ip_links->{eth0}; +$ip_links->{ens18}->{ifname} = ens18; my $wanted = load('base-allow-hotplug'); # parse the config -r($wanted, $proc_net, $active_ifaces); +r($wanted, $ip_links, $active_ifaces); $wanted =~ s/allow-hotplug ens18/auto ens18/; # FIXME: hack! rather we need to keep allow-hotplug! expect $wanted; # idempotency (save, re-parse, and re-check) -r(w(), $proc_net, $active_ifaces); +r(w(), $ip_links, $active_ifaces); expect $wanted; # parse one with both, "auto" and "allow-hotplug" my $bad = load('base-auto-allow-hotplug'); -r($bad, $proc_net, $active_ifaces); +r($bad, $ip_links, $active_ifaces); # should drop the first occuring one of the conflicting options ("auto" currently) expect $wanted; diff --git a/test/etc_network_interfaces/t.create_network.pl b/test/etc_network_interfaces/t.create_network.pl index 7b5a12f..ef63d0f 100644 --- a/test/etc_network_interfaces/t.create_network.pl +++ b/test/etc_network_interfaces/t.create_network.pl @@ -1,13 +1,16 @@ -save('proc_net_dev', <<'/proc/net/dev'); -eth0: -eth1: -eth2: -eth3: -eth4: -eth5: -/proc/net/dev +use JSON; +use Storable qw(dclone); -r(load('brbase')); +my $ip_links = decode_json(load('ip_link_details')); + +for my $idx (1..5) { + my $entry = dclone($ip_links->{eth0}); + $entry->{ifname} = "eth$idx"; + + $ip_links->{"eth$idx"} = $entry; +} + +r(load('brbase'), $ip_links); # # Variables used for the various interfaces: @@ -483,14 +486,14 @@ CHECK # save('if', w()); -r(load('if')); +r(load('if'), $ip_links); expect load('if'); # # Check a brbase with an ipv6 address on eth1 # -r(load('brbase')); +r(load('brbase'), $ip_links); my $ip = 'fc05::2'; my $nm = '112'; @@ -535,7 +538,7 @@ iface vmbr0 inet static CHECK save('if', w()); -r(load('if')); +r(load('if'), $ip_links); expect load('if'); 1; diff --git a/test/etc_network_interfaces/t.keep-option-order.pl b/test/etc_network_interfaces/t.keep-option-order.pl index 6651871..8b506c1 100644 --- a/test/etc_network_interfaces/t.keep-option-order.pl +++ b/test/etc_network_interfaces/t.keep-option-order.pl @@ -1,3 +1,15 @@ +use JSON; +use Storable qw(dclone); + +my $ip_links = decode_json(load('ip_link_details')); + +for my $idx (1..3) { + my $entry = dclone($ip_links->{eth0}); + $entry->{ifname} = "eth$idx"; + + $ip_links->{"eth$idx"} = $entry; +} + # # Order of option lines between interfaces should be preserved: # eth0 is unconfigured and will thus end up at the end as 'manual' @@ -15,14 +27,7 @@ iface eth3 inet manual ORDERED -r( - "$ordered", <<'/proc/net/dev', -eth0: -eth1: -eth2: -eth3: -/proc/net/dev -); +r($ordered, $ip_links); expect(load('loopback') . $ordered . "iface eth0 inet manual\n\n"); diff --git a/test/etc_network_interfaces/t.list-interfaces.pl b/test/etc_network_interfaces/t.list-interfaces.pl index 638bc42..d8d1450 100644 --- a/test/etc_network_interfaces/t.list-interfaces.pl +++ b/test/etc_network_interfaces/t.list-interfaces.pl @@ -7,13 +7,22 @@ # The expected behavior is to notice their existance and treat them as manually # configured interfaces. # Saving the file after reading would add the corresponding 'manual' lines. -save('proc_net_dev', <<'/proc/net/dev'); -eth0: -eth1: -eth2: -eth3: -eth100: -/proc/net/dev + +use JSON; +use Storable qw(dclone); + +my $ip_links = decode_json(load('ip_link_details')); + +for my $idx (1..3) { + my $entry = dclone($ip_links->{eth0}); + $entry->{ifname} = "eth$idx"; + + $ip_links->{"eth$idx"} = $entry; +} + +my $entry = dclone($ip_links->{eth0}); +$entry->{ifname} = "eth100"; +$ip_links->{"eth100"} = $entry; my %wanted = ( vmbr0 => { @@ -85,7 +94,7 @@ source after-ovs /etc/network/interfaces -r(load('interfaces')); +r(load('interfaces'), $ip_links); save('2', w()); my $ifaces = $config->{ifaces}; @@ -125,7 +134,7 @@ die "invalid families defined for vmbr0" if (scalar(@f100) != 2) || ($f100[0] ne 'inet') || ($f100[1] ne 'inet6'); # idempotency -r(w()); +r(w(), $ip_links); expect load('2'); 1; diff --git a/test/etc_network_interfaces/t.ovs_bridge_allow.pl b/test/etc_network_interfaces/t.ovs_bridge_allow.pl index fabaa8b..f4fbf4c 100644 --- a/test/etc_network_interfaces/t.ovs_bridge_allow.pl +++ b/test/etc_network_interfaces/t.ovs_bridge_allow.pl @@ -1,17 +1,22 @@ use strict; +use JSON; +use Storable qw(dclone); + +my $ip_links = decode_json(load('ip_link_details')); + +for my $idx (1..3) { + my $entry = dclone($ip_links->{eth0}); + $entry->{ifname} = "eth$idx"; + + $ip_links->{"eth$idx"} = $entry; +} + + my $ip = '192.168.0.100/24'; my $gw = '192.168.0.1'; -# replace proc_net_dev with one with a bunch of interfaces -save('proc_net_dev', <<'/proc/net/dev'); -eth0: -eth1: -eth2: -eth3: -/proc/net/dev - -r(''); +r('', $ip_links); new_iface( 'vmbr0', @@ -82,7 +87,7 @@ iface vmbr0 inet static # they're stripped from $config->{options} at load-time since they're # auto-generated when writing OVSPorts. save('idem', w()); -r(load('idem')); +r(load('idem'), $ip_links); expect load('idem'); # Removing an ovs_port also has to remove the corresponding allow- line! diff --git a/test/etc_network_interfaces/t.unhandled-interfaces-to-manual.pl b/test/etc_network_interfaces/t.unhandled-interfaces-to-manual.pl index fd79251..c2dc602 100644 --- a/test/etc_network_interfaces/t.unhandled-interfaces-to-manual.pl +++ b/test/etc_network_interfaces/t.unhandled-interfaces-to-manual.pl @@ -1,18 +1,16 @@ -r( - '', <<'/proc/net/dev', -Inter-| Receive | Transmit - face |bytes packets errs drop fifo frame compressed multicast|bytes packets errs drop fifo colls carrier compressed -These eth interfaces show up: - eth0: -eth1: - eth2: - eth3: - lo: -All other stuff is being ignored eth99: -eth100 is not actually available: - ethBAD: this one's now allowed either -/proc/net/dev -); +use JSON; +use Storable qw(dclone); + +my $ip_links = decode_json(load('ip_link_details')); + +for my $idx (1..3) { + my $entry = dclone($ip_links->{eth0}); + $entry->{ifname} = "eth$idx"; + + $ip_links->{"eth$idx"} = $entry; +} + +r('', $ip_links); expect load('base') . <<'IFACES'; iface eth1 inet manual diff --git a/test/etc_network_interfaces/t.unknown_order.pl b/test/etc_network_interfaces/t.unknown_order.pl index 291b858..dd73069 100644 --- a/test/etc_network_interfaces/t.unknown_order.pl +++ b/test/etc_network_interfaces/t.unknown_order.pl @@ -1,3 +1,15 @@ +use JSON; +use Storable qw(dclone); + +my $ip_links = decode_json(load('ip_link_details')); + +for my $idx (1..5) { + my $entry = dclone($ip_links->{eth0}); + $entry->{ifname} = "eth$idx"; + + $ip_links->{"eth$idx"} = $entry; +} + my $base = load('loopback'); sub wanted($) { @@ -91,7 +103,7 @@ iface vmbr5 inet manual IFACES } -r(wanted(13)); +r(wanted(13), $ip_links); update_iface('bond1', [{ family => 'inet', address => '10.10.10.11/24' }]); expect wanted(11); -- 2.39.5 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel