From 2a8181bcd78d055b7cb9e6c0e026bc3b08231b5a Mon Sep 17 00:00:00 2001
From: Beniamino Galvani <bgalvani@redhat.com>
Date: Tue, 25 May 2021 16:58:28 +0200
Subject: [PATCH] core,libnm: don't touch device TC configuration by default

NetworkManager supports a very limited set of qdiscs. If users want to
configure a unsupported qdisc, they need to do it outside of
NetworkManager using tc.

The problem is that NM also removes all qdiscs and filters during
activation if the connection doesn't contain a TC setting. Therefore,
setting TC configuration outside of NM is hard because users need to
do it *after* the connection is up (for example through a dispatcher
script).

Let NM consider the presence (or absence) of a TC setting in the
connection to determine whether NM should configure (or not) qdiscs
and filters on the interface. We already do something similar for
SR-IOV configuration.

Since new connections don't have the TC setting, the new behavior
(ignore existing configuration) will be the default. The impact of
this change in different scenarios is:

 - the user previously configured TC settings via NM. This continues
   to work as before;

 - the user didn't set any qdiscs or filters in the connection, and
   expected NM to clear them from the interface during activation.
   Here there is a change in behavior, but it seems unlikely that
   anybody relied on the old one;

 - the user didn't care about qdiscs and filters; NM removed all
   qdiscs upon activation, and so the default qdisc from kernel was
   used. After this change, NM will not touch qdiscs and the default
   qdisc will be used, as before;

 - the user set a different qdisc via tc and NM cleared it during
   activation. Now this will work as expected.

So, the new default behavior seems better than the previous one.

https://bugzilla.redhat.com/show_bug.cgi?id=1928078
(cherry picked from commit a48edd0410c878d65fc5adcd5192b116ab6f8afc)
---
 .../generate-docs-nm-settings-nmcli.xml.in    |  4 +--
 clients/common/settings-docs.h.in             |  4 +--
 libnm-core/nm-setting-tc-config.c             | 16 ++++++++++++
 src/core/devices/nm-device.c                  | 26 +++++++++----------
 4 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/clients/cli/generate-docs-nm-settings-nmcli.xml.in b/clients/cli/generate-docs-nm-settings-nmcli.xml.in
index 1044ae0d38..0a75a0e681 100644
--- a/clients/cli/generate-docs-nm-settings-nmcli.xml.in
+++ b/clients/cli/generate-docs-nm-settings-nmcli.xml.in
@@ -914,9 +914,9 @@
     </setting>
     <setting name="tc" >
         <property name="qdiscs"
-                  description="Array of TC queueing disciplines." />
+                  description="Array of TC queueing disciplines. When the &quot;tc&quot; setting is present, qdiscs from this property are applied upon activation. If the property is empty, all qdiscs are removed and the device will only have the default qdisc assigned by kernel according to the &quot;net.core.default_qdisc&quot; sysctl. If the &quot;tc&quot; setting is not present, NetworkManager doesn&apos;t touch the qdiscs present on the interface." />
         <property name="tfilters"
-                  description="Array of TC traffic filters." />
+                  description="Array of TC traffic filters. When the &quot;tc&quot; setting is present, filters from this property are applied upon activation. If the property is empty, NetworkManager removes all the filters. If the &quot;tc&quot; setting is not present, NetworkManager doesn&apos;t touch the filters present on the interface." />
     </setting>
     <setting name="team" >
         <property name="config"
diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in
index 2c275a99c8..7c3ff25fe3 100644
--- a/clients/common/settings-docs.h.in
+++ b/clients/common/settings-docs.h.in
@@ -342,8 +342,8 @@
 #define DESCRIBE_DOC_NM_SETTING_SRIOV_AUTOPROBE_DRIVERS N_("Whether to autoprobe virtual functions by a compatible driver. If set to NM_TERNARY_TRUE (1), the kernel will try to bind VFs to a compatible driver and if this succeeds a new network interface will be instantiated for each VF. If set to NM_TERNARY_FALSE (0), VFs will not be claimed and no network interfaces will be created for them. When set to NM_TERNARY_DEFAULT (-1), the global default is used; in case the global default is unspecified it is assumed to be NM_TERNARY_TRUE (1).")
 #define DESCRIBE_DOC_NM_SETTING_SRIOV_TOTAL_VFS N_("The total number of virtual functions to create. Note that when the sriov setting is present NetworkManager enforces the number of virtual functions on the interface (also when it is zero) during activation and resets it upon deactivation. To prevent any changes to SR-IOV parameters don't add a sriov setting to the connection.")
 #define DESCRIBE_DOC_NM_SETTING_SRIOV_VFS N_("Array of virtual function descriptors. Each VF descriptor is a dictionary mapping attribute names to GVariant values. The 'index' entry is mandatory for each VF. When represented as string a VF is in the form: \"INDEX [ATTR=VALUE[ ATTR=VALUE]...]\". for example: \"2 mac=00:11:22:33:44:55 spoof-check=true\". Multiple VFs can be specified using a comma as separator. Currently, the following attributes are supported: mac, spoof-check, trust, min-tx-rate, max-tx-rate, vlans. The \"vlans\" attribute is represented as a semicolon-separated list of VLAN descriptors, where each descriptor has the form \"ID[.PRIORITY[.PROTO]]\". PROTO can be either 'q' for 802.1Q (the default) or 'ad' for 802.1ad.")
-#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_QDISCS N_("Array of TC queueing disciplines.")
-#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_TFILTERS N_("Array of TC traffic filters.")
+#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_QDISCS N_("Array of TC queueing disciplines. When the \"tc\" setting is present, qdiscs from this property are applied upon activation. If the property is empty, all qdiscs are removed and the device will only have the default qdisc assigned by kernel according to the \"net.core.default_qdisc\" sysctl. If the \"tc\" setting is not present, NetworkManager doesn't touch the qdiscs present on the interface.")
+#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_TFILTERS N_("Array of TC traffic filters. When the \"tc\" setting is present, filters from this property are applied upon activation. If the property is empty, NetworkManager removes all the filters. If the \"tc\" setting is not present, NetworkManager doesn't touch the filters present on the interface.")
 #define DESCRIBE_DOC_NM_SETTING_TEAM_CONFIG N_("The JSON configuration for the team network interface.  The property should contain raw JSON configuration data suitable for teamd, because the value is passed directly to teamd. If not specified, the default configuration is used.  See man teamd.conf for the format details.")
 #define DESCRIBE_DOC_NM_SETTING_TEAM_LINK_WATCHERS N_("Link watchers configuration for the connection: each link watcher is defined by a dictionary, whose keys depend upon the selected link watcher. Available link watchers are 'ethtool', 'nsna_ping' and 'arp_ping' and it is specified in the dictionary with the key 'name'. Available keys are:   ethtool: 'delay-up', 'delay-down', 'init-wait'; nsna_ping: 'init-wait', 'interval', 'missed-max', 'target-host'; arp_ping: all the ones in nsna_ping and 'source-host', 'validate-active', 'validate-inactive', 'send-always'. See teamd.conf man for more details.")
 #define DESCRIBE_DOC_NM_SETTING_TEAM_MCAST_REJOIN_COUNT N_("Corresponds to the teamd mcast_rejoin.count.")
diff --git a/libnm-core/nm-setting-tc-config.c b/libnm-core/nm-setting-tc-config.c
index 33df6d34e5..2fad98b1e8 100644
--- a/libnm-core/nm-setting-tc-config.c
+++ b/libnm-core/nm-setting-tc-config.c
@@ -1810,6 +1810,15 @@ nm_setting_tc_config_class_init(NMSettingTCConfigClass *klass)
      * NMSettingTCConfig:qdiscs: (type GPtrArray(NMTCQdisc))
      *
      * Array of TC queueing disciplines.
+     *
+     * When the #NMSettingTCConfig setting is present, qdiscs from this
+     * property are applied upon activation. If the property is empty,
+     * all qdiscs are removed and the device will only
+     * have the default qdisc assigned by kernel according to the
+     * "net.core.default_qdisc" sysctl.
+     *
+     * If the #NMSettingTCConfig setting is not present, NetworkManager
+     * doesn't touch the qdiscs present on the interface.
      **/
     /* ---ifcfg-rh---
      * property: qdiscs
@@ -1834,6 +1843,13 @@ nm_setting_tc_config_class_init(NMSettingTCConfigClass *klass)
      * NMSettingTCConfig:tfilters: (type GPtrArray(NMTCTfilter))
      *
      * Array of TC traffic filters.
+     *
+     * When the #NMSettingTCConfig setting is present, filters from this
+     * property are applied upon activation. If the property is empty,
+     * NetworkManager removes all the filters.
+     *
+     * If the #NMSettingTCConfig setting is not present, NetworkManager
+     * doesn't touch the filters present on the interface.
      **/
     /* ---ifcfg-rh---
      * property: qdiscs
diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c
index 086fd4ddfb..73a94c7281 100644
--- a/src/core/devices/nm-device.c
+++ b/src/core/devices/nm-device.c
@@ -8361,26 +8361,23 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode)
 static gboolean
 tc_commit(NMDevice *self)
 {
-    NMConnection *    connection          = NULL;
     gs_unref_ptrarray GPtrArray *qdiscs   = NULL;
     gs_unref_ptrarray GPtrArray *tfilters = NULL;
-    NMSettingTCConfig *          s_tc     = NULL;
+    NMSettingTCConfig *          s_tc;
     NMPlatform *                 platform;
     int                          ip_ifindex;
 
-    platform   = nm_device_get_platform(self);
-    connection = nm_device_get_applied_connection(self);
-    if (connection)
-        s_tc = nm_connection_get_setting_tc_config(connection);
+    s_tc = nm_device_get_applied_setting(self, NM_TYPE_SETTING_TC_CONFIG);
+    if (!s_tc)
+        return TRUE;
 
     ip_ifindex = nm_device_get_ip_ifindex(self);
     if (!ip_ifindex)
-        return s_tc == NULL;
+        return FALSE;
 
-    if (s_tc) {
-        qdiscs   = nm_utils_qdiscs_from_tc_setting(platform, s_tc, ip_ifindex);
-        tfilters = nm_utils_tfilters_from_tc_setting(platform, s_tc, ip_ifindex);
-    }
+    platform = nm_device_get_platform(self);
+    qdiscs   = nm_utils_qdiscs_from_tc_setting(platform, s_tc, ip_ifindex);
+    tfilters = nm_utils_tfilters_from_tc_setting(platform, s_tc, ip_ifindex);
 
     if (!nm_platform_qdisc_sync(platform, ip_ifindex, qdiscs))
         return FALSE;
@@ -15898,9 +15895,12 @@ nm_device_cleanup(NMDevice *self, NMDeviceStateReason reason, CleanupType cleanu
 
             nm_platform_ip_route_flush(platform, AF_UNSPEC, ifindex);
             nm_platform_ip_address_flush(platform, AF_UNSPEC, ifindex);
-            nm_platform_tfilter_sync(platform, ifindex, NULL);
-            nm_platform_qdisc_sync(platform, ifindex, NULL);
             set_ipv6_token(self, iid, "::");
+
+            if (nm_device_get_applied_setting(self, NM_TYPE_SETTING_TC_CONFIG)) {
+                nm_platform_tfilter_sync(platform, ifindex, NULL);
+                nm_platform_qdisc_sync(platform, ifindex, NULL);
+            }
         }
     }