From d6826b15e6a2d1fde19fff520df6b9878e0fc7a2 Mon Sep 17 00:00:00 2001 From: Franco Fichtner Date: Thu, 30 Jun 2022 10:11:22 +0200 Subject: [PATCH] interfaces: stop last internal use of /var/run/booting #5637 At last, we seem to be free... To be precise here move staticarp configure before reload block in interface_configure() to avoid passing a stale ifconfig cache as that would trigger a transition twice. Pass ifconfig cache from where it is available or read it on the fly (e.g. rc.linkup). With that cache we can figure out if a transition is required and so can avoid most of the boot stalling except when staticarp is enabled on a lot of interfaces, but that was always slow(er) later on. It should even be faster now avoiding the ifconfig in the common case. There is a side effect that dhcp wants to populate the ARP table and that is still unconditional because we do not know whether we have new entries added or others removed. Having them removed might leave them in the ARP table for longer than necessary, however. It's not that the current implementation is particularly bad, but it relies heavily on implied regular flushing of ARP entries just to keep a consistent functionality which is a big design flaw. As a stopgap measure remove an ARP entry when we delete the static mapping for it to keep the entries in sync. /var/run/booting remains in backend scripts that should not interfere with boot but we will clean these up later as they do not need removal but rather a transition to a safer way than checking for a file (that might not get deleted for one reason or another.. it has been known to happen). --- src/etc/inc/interfaces.inc | 26 +++++++++++++------ src/etc/inc/plugins.inc.d/dhcpd.inc | 3 ++- .../conf/actions.d/actions_interface.conf | 6 +++++ src/www/services_dhcp.php | 5 ++++ 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/etc/inc/interfaces.inc b/src/etc/inc/interfaces.inc index 401fe993a..f2c064940 100644 --- a/src/etc/inc/interfaces.inc +++ b/src/etc/inc/interfaces.inc @@ -2363,6 +2363,9 @@ function interface_configure($verbose = false, $interface = 'wan', $reload = fal /* XXX bridges can only load if all interfaces are there */ } + /* XXX may be called twice since also tied to dhcp configure */ + interfaces_staticarp_configure($interface, $ifconfig_details); + if ($verbose) { echo "done.\n"; } @@ -2377,8 +2380,6 @@ function interface_configure($verbose = false, $interface = 'wan', $reload = fal configdp_run('rfc2136 reload', array($interface)); } - interfaces_staticarp_configure($interface); - return $loaded; } @@ -3807,7 +3808,7 @@ function get_vip_descr($ipaddress) return ''; } -function interfaces_staticarp_configure($if) +function interfaces_staticarp_configure($if, $ifconfig_details = null) { global $config; @@ -3816,22 +3817,31 @@ function interfaces_staticarp_configure($if) return; } - mwexecf('/sbin/ifconfig %s %sstaticarp', [$ifcfg['if'], isset($config['dhcpd'][$if]['staticarp']) ? '' : '-']); + if (empty($ifconfig_details)) { + $ifconfig_details = legacy_interfaces_details($ifcfg['if']); + } - if (!file_exists('/var/run/booting')) { - // XXX: arp is really slow when there are many interfaces, while booting -d -a shouldn't be needed anyway + if (empty($ifconfig_details[$ifcfg['if']])) { + return; + } + + $have = in_array('staticarp', $ifconfig_details[$ifcfg['if']]['flags']); + $want = isset($config['dhcpd'][$if]['staticarp']); + + if ($have !== $want) { + mwexecf('/sbin/ifconfig %s %sstaticarp', [$ifcfg['if'], $want ? '' : '-']); mwexecf('/usr/sbin/arp -d -i %s -a', [$ifcfg['if']]); } if (isset($config['dhcpd'][$if]['staticmap'])) { foreach ($config['dhcpd'][$if]['staticmap'] as $arpent) { - if (!isset($config['dhcpd'][$if]['staticarp']) && !isset($arpent['arp_table_static_entry'])) { + if (!$want && !isset($arpent['arp_table_static_entry'])) { continue; } if (!isset($arpent['ipaddr'])) { continue; } - mwexecf( '/usr/sbin/arp -s %s %s', [$arpent['ipaddr'], $arpent['mac']]); + mwexecf('/usr/sbin/arp -s %s %s', [$arpent['ipaddr'], $arpent['mac']]); } } } diff --git a/src/etc/inc/plugins.inc.d/dhcpd.inc b/src/etc/inc/plugins.inc.d/dhcpd.inc index f9fb2787b..2681dfdfe 100644 --- a/src/etc/inc/plugins.inc.d/dhcpd.inc +++ b/src/etc/inc/plugins.inc.d/dhcpd.inc @@ -633,7 +633,8 @@ EOD; * to setup failover peer "bleh" entries */ foreach ($config['dhcpd'] as $dhcpif => $dhcpifconf) { - interfaces_staticarp_configure($dhcpif); + /* certainly odd but documented and side effect populates ARP table */ + interfaces_staticarp_configure($dhcpif, $ifconfig_details); if (!isset($dhcpifconf['enable'])) { continue; diff --git a/src/opnsense/service/conf/actions.d/actions_interface.conf b/src/opnsense/service/conf/actions.d/actions_interface.conf index 62cf22c04..96fe80e6c 100644 --- a/src/opnsense/service/conf/actions.d/actions_interface.conf +++ b/src/opnsense/service/conf/actions.d/actions_interface.conf @@ -47,6 +47,12 @@ parameters: %s type:script_output message:request arp table +[remove.arp] +command:/usr/sbin/arp -d +parameters:%s 2> /dev/null; exit 0 +type:script +message:remove arp entry for %s + [flush.arp] command:arp -da parameters: diff --git a/src/www/services_dhcp.php b/src/www/services_dhcp.php index 39de82b70..a456c0fb4 100644 --- a/src/www/services_dhcp.php +++ b/src/www/services_dhcp.php @@ -435,6 +435,11 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { exit; } elseif ($act == "del") { if (!empty($config['dhcpd'][$if]['staticmap'][$_POST['id']])) { + if (isset($config['dhcpd'][$if]['staticmap'][$_POST['id']]['ipaddr'])) { + configdp_run('interface remove arp', [ + $config['dhcpd'][$if]['staticmap'][$_POST['id']]['ipaddr'] + ]); + } unset($config['dhcpd'][$if]['staticmap'][$_POST['id']]); write_config(); if (isset($config['dhcpd'][$if]['enable'])) {