From 9a4578c8f9c8e098b6e9816c39841b60e1e62a26 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 22 May 2020 15:26:43 +0200 Subject: platform: use ECHO flag for qdisc and filter requests By default the kernel sends back events notification to all other process except the one that requested the change, unless the ECHO flag is used. See [1], [2]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sched/sch_api.c?h=v5.6#n979 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/core/rtnetlink.c?h=v5.6#n706 --- src/platform/nm-linux-platform.c | 4 ++-- src/platform/nm-platform.c | 1 + src/platform/nm-platform.h | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index a721fc7cfa..78e33748c1 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -4644,7 +4644,7 @@ _nl_msg_new_qdisc (int nlmsg_type, .tcm_info = qdisc->info, }; - msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags); + msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags | NMP_NLM_FLAG_F_ECHO); if (nlmsg_append_struct (msg, &tcm) < 0) goto nla_put_failure; @@ -4697,7 +4697,7 @@ _nl_msg_new_tfilter (int nlmsg_type, .tcm_info = tfilter->info, }; - msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags); + msg = nlmsg_alloc_simple (nlmsg_type, nlmsg_flags | NMP_NLM_FLAG_F_ECHO); if (nlmsg_append_struct (msg, &tcm) < 0) goto nla_put_failure; diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 8d826a109f..33aa8db69e 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -310,6 +310,7 @@ NM_UTILS_LOOKUP_STR_DEFINE (_nmp_nlm_flag_to_string_lookup, NMPNlmFlags, NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_F_APPEND), NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_FMASK), NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_SUPPRESS_NETLINK_FAILURE), + NM_UTILS_LOOKUP_ITEM_IGNORE (NMP_NLM_FLAG_F_ECHO), ); #define _nmp_nlm_flag_to_string(flags) \ diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index c21190e803..4bfbfea432 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -69,6 +69,8 @@ typedef gboolean (*NMPObjectPredicateFunc) (const NMPObject *obj, #define NM_GRE_KEY 0x2000 typedef enum { + NMP_NLM_FLAG_F_ECHO = 0x08, /* NLM_F_ECHO, Echo this request */ + /* use our own platform enum for the nlmsg-flags. Otherwise, we'd have * to include */ NMP_NLM_FLAG_F_REPLACE = 0x100, /* NLM_F_REPLACE, Override existing */ -- cgit v1.2.1 From 9064502834a650d8cb1a40f11b9833162679b109 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 22 May 2020 17:49:39 +0200 Subject: platform: rework qdisc synchronization Rework qdisc synchronization. The previous implementation added all known qdiscs and removed unneeded ones from platform; this had some problems: - kernel doesn't allow to add (with exclusive flag) a qdisc if one with the same parent already exists; - if we use the replace flag instead of add, then it becomes possible to add a new qdisc with the same parent of an existing one. However if the existing qdisc is of the same kind, kernel will try to to change() it, which fails for some qdiscs (e.g. sfq). - kernel doesn't allow to delete a qdisc with handle of zero because that is the default qdisc and can only be replaced; Fix that. --- src/platform/nm-platform.c | 55 +++++++++++++++++++++++++++++++++++++--------- src/platform/nm-platform.h | 3 +++ 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 33aa8db69e..5ac788f73f 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -5064,12 +5064,15 @@ nm_platform_qdisc_sync (NMPlatform *self, known_qdiscs_idx = g_hash_table_new ((GHashFunc) nmp_object_id_hash, (GEqualFunc) nmp_object_id_equal); - if (known_qdiscs) { for (i = 0; i < known_qdiscs->len; i++) { const NMPObject *q = g_ptr_array_index (known_qdiscs, i); - g_hash_table_insert (known_qdiscs_idx, (gpointer) q, (gpointer) q); + if (!g_hash_table_insert (known_qdiscs_idx, (gpointer) q, (gpointer) q)) { + _LOGW ("duplicate qdisc %s", nm_platform_qdisc_to_string (&q->qdisc, NULL, 0)); + return FALSE; + } + } } @@ -5078,13 +5081,34 @@ nm_platform_qdisc_sync (NMPlatform *self, NMP_OBJECT_TYPE_QDISC, ifindex), NULL, NULL); - if (plat_qdiscs) { for (i = 0; i < plat_qdiscs->len; i++) { - const NMPObject *q = g_ptr_array_index (plat_qdiscs, i); + const NMPObject *p = g_ptr_array_index (plat_qdiscs, i); + const NMPObject *k; - if (!g_hash_table_lookup (known_qdiscs_idx, q)) - success &= nm_platform_object_delete (self, q); + /* look up known qdisc with same parent */ + k = g_hash_table_lookup (known_qdiscs_idx, p); + + if (k) { + const NMPlatformQdisc *qdisc_k = NMP_OBJECT_CAST_QDISC (k); + const NMPlatformQdisc *qdisc_p = NMP_OBJECT_CAST_QDISC (p); + + /* check other fields */ + if ( nm_platform_qdisc_cmp_full (qdisc_k, qdisc_p, FALSE) != 0 + || ( qdisc_k->handle != qdisc_p->handle + && qdisc_k != 0)) { + k = NULL; + } + } + + if (k) { + g_hash_table_remove (known_qdiscs_idx, k); + } else { + /* can't delete qdisc with zero handle */ + if (TC_H_MAJ (p->qdisc.handle) != 0) { + success &= nm_platform_object_delete (self, p); + } + } } } @@ -5092,8 +5116,10 @@ nm_platform_qdisc_sync (NMPlatform *self, for (i = 0; i < known_qdiscs->len; i++) { const NMPObject *q = g_ptr_array_index (known_qdiscs, i); - success &= (nm_platform_qdisc_add (self, NMP_NLM_FLAG_ADD, - NMP_OBJECT_CAST_QDISC (q)) >= 0); + if (g_hash_table_contains (known_qdiscs_idx, q)) { + success &= (nm_platform_qdisc_add (self, NMP_NLM_FLAG_ADD, + NMP_OBJECT_CAST_QDISC (q)) >= 0); + } } } @@ -6518,14 +6544,17 @@ nm_platform_qdisc_hash_update (const NMPlatformQdisc *obj, NMHashState *h) } int -nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b) +nm_platform_qdisc_cmp_full (const NMPlatformQdisc *a, + const NMPlatformQdisc *b, + gboolean compare_handle) { NM_CMP_SELF (a, b); NM_CMP_FIELD (a, b, ifindex); NM_CMP_FIELD (a, b, parent); NM_CMP_FIELD_STR_INTERNED (a, b, kind); NM_CMP_FIELD (a, b, addr_family); - NM_CMP_FIELD (a, b, handle); + if (compare_handle) + NM_CMP_FIELD (a, b, handle); NM_CMP_FIELD (a, b, info); if (nm_streq0 (a->kind, "fq_codel")) { @@ -6542,6 +6571,12 @@ nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b) return 0; } +int +nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b) +{ + return nm_platform_qdisc_cmp_full (a, b, TRUE); +} + const char * nm_platform_tfilter_to_string (const NMPlatformTfilter *tfilter, char *buf, gsize len) { diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 4bfbfea432..fb82262979 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -1909,6 +1909,9 @@ nm_platform_routing_rule_cmp_full (const NMPlatformRoutingRule *a, const NMPlatf } int nm_platform_qdisc_cmp (const NMPlatformQdisc *a, const NMPlatformQdisc *b); +int nm_platform_qdisc_cmp_full (const NMPlatformQdisc *a, + const NMPlatformQdisc *b, + gboolean compare_handle); int nm_platform_tfilter_cmp (const NMPlatformTfilter *a, const NMPlatformTfilter *b); void nm_platform_link_hash_update (const NMPlatformLink *obj, NMHashState *h); -- cgit v1.2.1 From 53aa5bd2070f0edf6b7180b2ebef75e6d6c9d2a0 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 25 May 2020 15:17:09 +0200 Subject: platform: add tc tests --- .gitignore | 2 + Makefile.am | 15 ++++ contrib/fedora/REQUIRED_PACKAGES | 1 + contrib/fedora/rpm/NetworkManager.spec | 4 + src/platform/tests/meson.build | 2 + src/platform/tests/test-common.c | 5 ++ src/platform/tests/test-tc.c | 133 +++++++++++++++++++++++++++++++++ 7 files changed, 162 insertions(+) create mode 100644 src/platform/tests/test-tc.c diff --git a/.gitignore b/.gitignore index 78aca2baef..cfb77444e8 100644 --- a/.gitignore +++ b/.gitignore @@ -239,6 +239,8 @@ test-*.trs /src/platform/tests/test-platform-general /src/platform/tests/test-route-fake /src/platform/tests/test-route-linux +/src/platform/tests/test-tc-fake +/src/platform/tests/test-tc-linux /src/settings/plugins/ifcfg-rh/nmdbus-ifcfg-rh.[ch] /src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh /src/settings/plugins/ifupdown/tests/test-ifupdown diff --git a/Makefile.am b/Makefile.am index ae3f1fc006..f1a06dbf33 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3848,6 +3848,8 @@ check_programs += \ src/platform/tests/test-platform-general \ src/platform/tests/test-route-fake \ src/platform/tests/test-route-linux \ + src/platform/tests/test-tc-fake \ + src/platform/tests/test-tc-linux \ $(NULL) src_platform_tests_monitor_CPPFLAGS = $(src_cppflags_test) @@ -3902,6 +3904,17 @@ src_platform_tests_test_route_linux_CPPFLAGS = $(src_tests_cppflags_linux) src_platform_tests_test_route_linux_LDFLAGS = $(src_platform_tests_ldflags) src_platform_tests_test_route_linux_LDADD = $(src_platform_tests_libadd) +src_platform_tests_test_tc_fake_SOURCES = src/platform/tests/test-tc.c +src_platform_tests_test_tc_fake_CPPFLAGS = $(src_tests_cppflags_fake) +src_platform_tests_test_tc_fake_LDFLAGS = $(src_platform_tests_ldflags) +src_platform_tests_test_tc_fake_LDADD = $(src_platform_tests_libadd) + +src_platform_tests_test_tc_linux_SOURCES = src/platform/tests/test-tc.c +src_platform_tests_test_tc_linux_CPPFLAGS = $(src_tests_cppflags_linux) +src_platform_tests_test_tc_linux_LDFLAGS = $(src_platform_tests_ldflags) +src_platform_tests_test_tc_linux_LDADD = $(src_platform_tests_libadd) + + $(src_platform_tests_monitor_OBJECTS): $(libnm_core_lib_h_pub_mkenums) $(src_platform_tests_test_address_fake_OBJECTS): $(libnm_core_lib_h_pub_mkenums) $(src_platform_tests_test_address_linux_OBJECTS): $(libnm_core_lib_h_pub_mkenums) @@ -3913,6 +3926,8 @@ $(src_platform_tests_test_nmp_object_OBJECTS): $(libnm_core_lib_h_pub_mken $(src_platform_tests_test_platform_general_OBJECTS): $(libnm_core_lib_h_pub_mkenums) $(src_platform_tests_test_route_fake_OBJECTS): $(libnm_core_lib_h_pub_mkenums) $(src_platform_tests_test_route_linux_OBJECTS): $(libnm_core_lib_h_pub_mkenums) +$(src_platform_tests_test_tc_fake_OBJECTS): $(libnm_core_lib_h_pub_mkenums) +$(src_platform_tests_test_tc_linux_OBJECTS): $(libnm_core_lib_h_pub_mkenums) EXTRA_DIST += \ src/platform/tests/meson.build \ diff --git a/contrib/fedora/REQUIRED_PACKAGES b/contrib/fedora/REQUIRED_PACKAGES index f03eebfb00..18ba364199 100755 --- a/contrib/fedora/REQUIRED_PACKAGES +++ b/contrib/fedora/REQUIRED_PACKAGES @@ -86,6 +86,7 @@ install \ # some packages don't exist in certain distributions. Install them one-by-one, and ignore errors. install_ignore_missing \ dbus-python \ + iproute-tc \ libasan \ libpsl-devel \ libubsan \ diff --git a/contrib/fedora/rpm/NetworkManager.spec b/contrib/fedora/rpm/NetworkManager.spec index 59406e5b23..81567ab58a 100644 --- a/contrib/fedora/rpm/NetworkManager.spec +++ b/contrib/fedora/rpm/NetworkManager.spec @@ -250,6 +250,10 @@ BuildRequires: libubsan %if %{with firewalld_zone} BuildRequires: firewalld-filesystem %endif +BuildRequires: iproute +%if 0%{?fedora} || 0%{?rhel} > 7 +BuildRequires: iproute-tc +%endif Provides: %{name}-dispatcher%{?_isa} = %{epoch}:%{version}-%{release} diff --git a/src/platform/tests/meson.build b/src/platform/tests/meson.build index 4a50bca978..f96850cc26 100644 --- a/src/platform/tests/meson.build +++ b/src/platform/tests/meson.build @@ -14,6 +14,8 @@ test_units = [ ['test-platform-general', 'test-platform-general.c', test_c_flags, default_test_timeout], ['test-route-fake', 'test-route.c', test_fake_c_flags, default_test_timeout], ['test-route-linux', 'test-route.c', test_linux_c_flags, default_test_timeout], + ['test-tc-fake', 'test-tc.c', test_fake_c_flags, default_test_timeout], + ['test-tc-linux', 'test-tc.c', test_linux_c_flags, default_test_timeout], ] foreach test_unit: test_units diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index f28dfa3a72..9f6a29bc99 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -557,6 +557,7 @@ nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_msec) { WaitForSignalData data = { 0 }; gulong id_link, id_ip4_address, id_ip6_address, id_ip4_route, id_ip6_route; + gulong id_qdisc, id_tfilter; _init_platform (&platform, FALSE); @@ -567,6 +568,8 @@ nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_msec) id_ip6_address = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ADDRESS_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); id_ip4_route = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); id_ip6_route = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); + id_qdisc = g_signal_connect (platform, NM_PLATFORM_SIGNAL_QDISC_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); + id_tfilter = g_signal_connect (platform, NM_PLATFORM_SIGNAL_TFILTER_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); /* if timeout_msec is negative, it means the wait-time already expired. * Maybe, we should do nothing and return right away, without even @@ -589,6 +592,8 @@ nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_msec) g_assert (nm_clear_g_signal_handler (platform, &id_ip6_address)); g_assert (nm_clear_g_signal_handler (platform, &id_ip4_route)); g_assert (nm_clear_g_signal_handler (platform, &id_ip6_route)); + g_assert (nm_clear_g_signal_handler (platform, &id_tfilter)); + g_assert (nm_clear_g_signal_handler (platform, &id_qdisc)); nm_clear_pointer (&data.loop, g_main_loop_unref); diff --git a/src/platform/tests/test-tc.c b/src/platform/tests/test-tc.c new file mode 100644 index 0000000000..dc2bf9f214 --- /dev/null +++ b/src/platform/tests/test-tc.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: LGPL-2.1+ + +#include "nm-default.h" + +#include + +#include "nm-test-utils-core.h" +#include "platform/nmp-object.h" +#include "platform/nmp-netns.h" +#include "platform/nm-platform-utils.h" +#include "test-common.h" + +static NMPObject * +qdisc_new (int ifindex, const char *kind, guint32 parent) +{ + NMPObject *obj; + + obj = nmp_object_new (NMP_OBJECT_TYPE_QDISC, NULL); + obj->qdisc = (NMPlatformQdisc) { + .ifindex = ifindex, + .kind = kind, + .parent = parent, + }; + + return obj; +} + +static GPtrArray * +qdiscs_lookup (int ifindex) +{ + NMPLookup lookup; + + return nm_platform_lookup_clone (NM_PLATFORM_GET, + nmp_lookup_init_object (&lookup, + NMP_OBJECT_TYPE_QDISC, + ifindex), + NULL, NULL); +} + +static void +test_qdisc1 (void) +{ + int ifindex; + gs_unref_ptrarray GPtrArray *known = NULL; + gs_unref_ptrarray GPtrArray *plat = NULL; + NMPObject *obj; + NMPlatformQdisc *qdisc; + + ifindex = nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME); + g_assert_cmpint (ifindex, >, 0); + + nmtstp_run_command ("tc qdisc del dev %s root", DEVICE_NAME); + nmtstp_run_command_check ("tc qdisc add dev %s root sfq", DEVICE_NAME); + + nmtstp_wait_for_signal (NM_PLATFORM_GET, 0); + + known = g_ptr_array_new_with_free_func ((GDestroyNotify) nmp_object_unref); + g_ptr_array_add (known, qdisc_new (ifindex, "fq_codel", TC_H_ROOT)); + g_ptr_array_add (known, qdisc_new (ifindex, "ingress", TC_H_INGRESS)); + + g_assert (nm_platform_qdisc_sync (NM_PLATFORM_GET, ifindex, known)); + plat = qdiscs_lookup (ifindex); + g_assert (plat); + g_assert_cmpint (plat->len, ==, 2); + + obj = plat->pdata[0]; + qdisc = NMP_OBJECT_CAST_QDISC (obj); + g_assert_cmpint (qdisc->parent, ==, TC_H_ROOT); + g_assert_cmpstr (qdisc->kind, ==, "fq_codel"); + + obj = plat->pdata[1]; + qdisc = NMP_OBJECT_CAST_QDISC (obj); + g_assert_cmpint (qdisc->parent, ==, TC_H_INGRESS); + g_assert_cmpstr (qdisc->kind, ==, "ingress"); +} + +static void +test_qdisc2 (void) +{ + int ifindex; + gs_unref_ptrarray GPtrArray *known = NULL; + gs_unref_ptrarray GPtrArray *plat = NULL; + NMPObject *obj; + NMPlatformQdisc *qdisc; + + ifindex = nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME); + g_assert_cmpint (ifindex, >, 0); + + nmtstp_run_command ("tc qdisc del dev %s root", DEVICE_NAME); + + nmtstp_wait_for_signal (NM_PLATFORM_GET, 0); + + known = g_ptr_array_new_with_free_func ((GDestroyNotify) nmp_object_unref); + obj = qdisc_new (ifindex, "fq_codel", TC_H_ROOT); + obj->qdisc.handle = TC_H_MAKE (0x8142 << 16, 0); + obj->qdisc.fq_codel.limit = 2048; + obj->qdisc.fq_codel.flows = 64; + obj->qdisc.fq_codel.quantum = 1000; + g_ptr_array_add (known, obj); + + g_assert (nm_platform_qdisc_sync (NM_PLATFORM_GET, ifindex, known)); + plat = qdiscs_lookup (ifindex); + g_assert (plat); + g_assert_cmpint (plat->len, ==, 1); + + obj = plat->pdata[0]; + qdisc = NMP_OBJECT_CAST_QDISC (obj); + g_assert_cmpstr (qdisc->kind, ==, "fq_codel"); + g_assert_cmpint (qdisc->handle, ==, TC_H_MAKE (0x8142 << 16, 0)); + g_assert_cmpint (qdisc->parent, ==, TC_H_ROOT); + g_assert_cmpint (qdisc->fq_codel.limit, ==, 2048); + g_assert_cmpint (qdisc->fq_codel.flows, ==, 64); + g_assert_cmpint (qdisc->fq_codel.quantum, ==, 1000); +} + +/*****************************************************************************/ + +NMTstpSetupFunc const _nmtstp_setup_platform_func = SETUP; + +void +_nmtstp_init_tests (int *argc, char ***argv) +{ + nmtst_init_with_logging (argc, argv, NULL, "ALL"); +} + +void +_nmtstp_setup_tests (void) +{ + if (nmtstp_is_root_test ()) { + nmtstp_env1_add_test_func ("/link/qdisc/1", test_qdisc1, TRUE); + nmtstp_env1_add_test_func ("/link/qdisc/2", test_qdisc2, TRUE); + } +} -- cgit v1.2.1