From 4116fa10448d1083612b97911a4671ccf3dc6775 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 14:10:41 +0200 Subject: [PATCH 01/14] tools/run-nm-test.sh: add -d option to set NMTST_DEBUG=d For convenience, by passing "-d" to tools/run-nm-test.sh, we set NMTST_DEBUG=d (if $NMTST_DEBUG is unset previously). --- tools/run-nm-test.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tools/run-nm-test.sh b/tools/run-nm-test.sh index 5848d0a061..2c23e7175f 100755 --- a/tools/run-nm-test.sh +++ b/tools/run-nm-test.sh @@ -121,6 +121,10 @@ else NMTST_USE_VALGRIND=0 shift; ;; + "-d") + NMTST_SET_DEBUG=1 + shift; + ;; "--test"|-t) shift TEST="$1" @@ -146,6 +150,10 @@ else fi +if [ "$NMTST_SET_DEBUG" == 1 -a -z "${NMTST_DEBUG+x}" ]; then + export NMTST_DEBUG=d +fi + if _is_true "$NMTST_MAKE_FIRST" 0; then git_dir="$(readlink -f "$(git rev-parse --show-toplevel)")" rel_path="$(realpath --relative-to="$git_dir" -m "$TEST" 2>/dev/null)" || die "cannot resolve test-name \"$TEST\". Did you call the script properly?" From 84b303217e0e357872155982d5756feb6f075fce Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 14:02:03 +0200 Subject: [PATCH 02/14] nmtst: add nmtst_get_rand_bool() util --- shared/nm-utils/nm-test-utils.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index d7233de20f..15671643da 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -808,6 +808,12 @@ nmtst_get_rand_int (void) return g_rand_int (nmtst_get_rand ()); } +static inline gboolean +nmtst_get_rand_bool (void) +{ + return nmtst_get_rand_int () % 2; +} + static inline gpointer nmtst_rand_buf (GRand *rand, gpointer buffer, gsize buffer_length) { From 530b5755ad9d06a53f007a462e59c08cf19698e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 11:16:32 +0200 Subject: [PATCH 03/14] platform: fix double whitespace for tun device in nm_platform_lnk_tun_to_string() --- src/platform/nm-platform.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index a645da2ee6..f380447015 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -5120,7 +5120,7 @@ nm_platform_lnk_tun_to_string (const NMPlatformLnkTun *lnk, char *buf, gsize len type = nm_sprintf_buf (str_type, "tun type %u", (guint) lnk->type); g_snprintf (buf, len, - "%s " /* type */ + "%s" /* type */ " pi %s" /* pi */ " vnet_hdr %s" /* vnet_hdr */ "%s" /* multi_queue */ From 01ad4f33ed489d2ffa4ab55ec72a17609694a0cf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 14:51:34 +0200 Subject: [PATCH 04/14] platform: adjust format for nm_platform_lnk_tun_to_string() and print "persist" Switch from "pi on|off" to optinally printing "pi" to indicate whether the flag is set. That follows ip-tuntap syntax and is more familiar: $ ip tuntap help Usage: ip tuntap { add | del | show | list | lst | help } [ dev PHYS_DEV ] [ mode { tun | tap } ] [ user USER ] [ group GROUP ] [ one_queue ] [ pi ] [ vnet_hdr ] [ multi_queue ] [ name NAME ] Where: USER := { STRING | NUMBER } GROUP := { STRING | NUMBER } Also, print the "persist" flag. --- src/platform/nm-platform.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index f380447015..e264324aa0 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -5121,16 +5121,18 @@ nm_platform_lnk_tun_to_string (const NMPlatformLnkTun *lnk, char *buf, gsize len g_snprintf (buf, len, "%s" /* type */ - " pi %s" /* pi */ - " vnet_hdr %s" /* vnet_hdr */ + "%s" /* pi */ + "%s" /* vnet_hdr */ "%s" /* multi_queue */ + "%s" /* persist */ "%s" /* owner */ "%s" /* group */ "", type, - lnk->pi ? "on" : "off", - lnk->vnet_hdr ? "on" : "off", + lnk->pi ? " pi" : "", + lnk->vnet_hdr ? " vnet_hdr" : "", lnk->multi_queue ? " multi_queue" : "", + lnk->persist ? " persist" : "", lnk->owner_valid ? nm_sprintf_buf (str_owner, " owner %u", (guint) lnk->owner) : "", lnk->group_valid ? nm_sprintf_buf (str_group, " group %u", (guint) lnk->group) : ""); return buf; From 28b5118ad2f35c984ea53e3df817b6cd0a090ab9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 17:33:21 +0200 Subject: [PATCH 05/14] platform: assert in nm_platform_link_tun_add() for unsupported options It doesn't make sense that NetworkManager adds non-persist tun devices, likewise, only the type IFF_TUN or IFF_TAP is supported. Assert that the values are as expected. --- src/platform/nm-linux-platform.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index b8d252070e..495d765b12 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -5597,8 +5597,9 @@ link_tun_add (NMPlatform *platform, struct ifreq ifr = { }; nm_auto_close int fd = -1; - if (!NM_IN_SET (props->type, IFF_TAP, IFF_TUN)) - return FALSE; + if ( !NM_IN_SET (props->type, IFF_TAP, IFF_TUN) + || !props->persist) + g_return_val_if_reached (FALSE); fd = open ("/dev/net/tun", O_RDWR | O_CLOEXEC); if (fd < 0) From e8a9bffdb088e0fb07d674ba6daae219d050d094 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 15:10:39 +0200 Subject: [PATCH 06/14] platform: refactor fetching links in cache_on_change() Rework the code to if-else-if, to not schedule the same DELAYED_ACTION_TYPE_REFRESH_LINK instance multiple times. Note that delayed_action_schedule() already would check that no duplicates are scheduled, but we can avoid that. --- src/platform/nm-linux-platform.c | 46 ++++++++++++++------------------ 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 495d765b12..787fdb19bd 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3934,40 +3934,31 @@ cache_on_change (NMPlatform *platform, && (obj_new && obj_new->_link.netlink.is_in_netlink) && (!obj_old || !obj_old->_link.netlink.is_in_netlink)) { - if (!obj_new->_link.netlink.lnk) { + gboolean re_request_link = FALSE; + + if ( !obj_new->_link.netlink.lnk + && NM_IN_SET (obj_new->link.type, NM_LINK_TYPE_GRE, + NM_LINK_TYPE_IP6TNL, + NM_LINK_TYPE_INFINIBAND, + NM_LINK_TYPE_MACVLAN, + NM_LINK_TYPE_MACVLAN, + NM_LINK_TYPE_SIT, + NM_LINK_TYPE_VLAN, + NM_LINK_TYPE_VXLAN)) { /* certain link-types also come with a IFLA_INFO_DATA/lnk_data. It may happen that * kernel didn't send this notification, thus when we first learn about a link * that lacks an lnk_data we re-request it again. * * For example https://bugzilla.redhat.com/show_bug.cgi?id=1284001 */ - switch (obj_new->link.type) { - case NM_LINK_TYPE_GRE: - case NM_LINK_TYPE_IP6TNL: - case NM_LINK_TYPE_INFINIBAND: - case NM_LINK_TYPE_MACVLAN: - case NM_LINK_TYPE_MACVTAP: - case NM_LINK_TYPE_SIT: - case NM_LINK_TYPE_VLAN: - case NM_LINK_TYPE_VXLAN: - delayed_action_schedule (platform, - DELAYED_ACTION_TYPE_REFRESH_LINK, - GINT_TO_POINTER (obj_new->link.ifindex)); - break; - default: - break; - } - } - if ( obj_new->link.type == NM_LINK_TYPE_VETH - && obj_new->link.parent == 0) { + re_request_link = TRUE; + } else if ( obj_new->link.type == NM_LINK_TYPE_VETH + && obj_new->link.parent == 0) { /* the initial notification when adding a veth pair can lack the parent/IFLA_LINK * (https://bugzilla.redhat.com/show_bug.cgi?id=1285827). * Request it again. */ - delayed_action_schedule (platform, - DELAYED_ACTION_TYPE_REFRESH_LINK, - GINT_TO_POINTER (obj_new->link.ifindex)); - } - if ( obj_new->link.type == NM_LINK_TYPE_ETHERNET - && obj_new->link.addr.len == 0) { + re_request_link = TRUE; + } else if ( obj_new->link.type == NM_LINK_TYPE_ETHERNET + && obj_new->link.addr.len == 0) { /* Due to a kernel bug, we sometimes receive spurious NEWLINK * messages after a wifi interface has disappeared. Since the * link is not present anymore we can't determine its type and @@ -3975,6 +3966,9 @@ cache_on_change (NMPlatform *platform, * specified. Request the link again to check if it really * exists. https://bugzilla.redhat.com/show_bug.cgi?id=1302037 */ + re_request_link = TRUE; + } + if (re_request_link) { delayed_action_schedule (platform, DELAYED_ACTION_TYPE_REFRESH_LINK, GINT_TO_POINTER (obj_new->link.ifindex)); From 031e58e1cf41347321b6310830ce053314f076e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 11:21:59 +0200 Subject: [PATCH 07/14] platform: enable parsing tun/tap properties from netlink Now that the kernel patches are merged to mainline (rc), enable accepting tun/tap link properties from netlink. https://bugzilla.redhat.com/show_bug.cgi?id=1547213 --- src/platform/nm-linux-platform.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 787fdb19bd..96d8367ecb 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -1396,10 +1396,6 @@ _parse_lnk_tun (const char *kind, struct nlattr *info_data) NMPObject *obj; NMPlatformLnkTun *props; - /* FIXME: the netlink API is not yet part of a released kernel version - * Disable it for now, until we are sure it is stable. */ - return NULL; - if (!info_data || !nm_streq0 (kind, "tun")) return NULL; From f76a94668ddd4b5e1385a0daf6c8925674f9ef18 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 11:35:55 +0200 Subject: [PATCH 08/14] platform: refetch TUN link when no type-specific lnk data was received Now that kernel supports providing information about tun/tap devices via netlink, make use of it. Also, enable the hack that: - when we first see a link that has no lnk data, we refetch it on the assumption, that kernel just didn't send it the first time. For old kernels that do not yet support tun properties on netlink, this means that we will always refetch the link once, the first time we see it. I think that is acceptable, and the more correct behavior for newer kernels that do support it. --- src/platform/nm-linux-platform.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 96d8367ecb..3c01d3d36e 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3939,6 +3939,7 @@ cache_on_change (NMPlatform *platform, NM_LINK_TYPE_MACVLAN, NM_LINK_TYPE_MACVLAN, NM_LINK_TYPE_SIT, + NM_LINK_TYPE_TUN, NM_LINK_TYPE_VLAN, NM_LINK_TYPE_VXLAN)) { /* certain link-types also come with a IFLA_INFO_DATA/lnk_data. It may happen that From 722f79c9c563192a46dddb921bfc00c24687f2ff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 15:14:10 +0200 Subject: [PATCH 09/14] platform: workaround kernel issue for tun device for first RTM_NETLINK event Due to a bug, the current rc-kernel will emit the first netlink notification about tun devices before the device is initialized. Hence, the content of the message is bogus. If the message looks like to be this case, re-request it right away. --- src/platform/nm-linux-platform.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 3c01d3d36e..8923708657 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3931,6 +3931,7 @@ cache_on_change (NMPlatform *platform, && (!obj_old || !obj_old->_link.netlink.is_in_netlink)) { gboolean re_request_link = FALSE; + const NMPlatformLnkTun *lnk_tun; if ( !obj_new->_link.netlink.lnk && NM_IN_SET (obj_new->link.type, NM_LINK_TYPE_GRE, @@ -3948,6 +3949,19 @@ cache_on_change (NMPlatform *platform, * * For example https://bugzilla.redhat.com/show_bug.cgi?id=1284001 */ re_request_link = TRUE; + } else if ( obj_new->link.type == NM_LINK_TYPE_TUN + && obj_new->_link.netlink.lnk + && (lnk_tun = &(obj_new->_link.netlink.lnk)->lnk_tun) + && !lnk_tun->persist + && lnk_tun->pi + && !lnk_tun->vnet_hdr + && !lnk_tun->multi_queue + && !lnk_tun->owner_valid + && !lnk_tun->group_valid) { + /* kernel has/had a know issue that the first notification for TUN device would + * be sent with invalid parameters. The message looks like that kind, so refetch + * it. */ + re_request_link = TRUE; } else if ( obj_new->link.type == NM_LINK_TYPE_VETH && obj_new->link.parent == 0) { /* the initial notification when adding a veth pair can lack the parent/IFLA_LINK From bd8ab54b8e2f66a53971430d66db766ca30b18ad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 14:02:22 +0200 Subject: [PATCH 10/14] platform/tests: add tests for TUN/TAP handling --- src/platform/tests/test-common.c | 65 ++++++++++++++++++++++++++++++++ src/platform/tests/test-common.h | 4 ++ src/platform/tests/test-link.c | 61 +++++++++++++++++++++++++++++- 3 files changed, 129 insertions(+), 1 deletion(-) diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index 67e74c13f2..4310ef39fb 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -23,6 +23,7 @@ #include #include #include +#include #include "test-common.h" @@ -1468,6 +1469,70 @@ nmtstp_link_sit_add (NMPlatform *platform, return pllink; } +const NMPlatformLink * +nmtstp_link_tun_add (NMPlatform *platform, + gboolean external_command, + const char *name, + const NMPlatformLnkTun *lnk) +{ + const NMPlatformLink *pllink = NULL; + NMPlatformError plerr; + int err; + + g_assert (nm_utils_is_valid_iface_name (name, NULL)); + g_assert (lnk); + g_assert (NM_IN_SET (lnk->type, IFF_TUN, IFF_TAP)); + + if (!lnk->persist) { + /* ip tuntap does not support non-persistent devices. + * + * Add this device only via NMPlatform. */ + if (external_command == -1) + external_command = FALSE; + } + + external_command = nmtstp_run_command_check_external (external_command); + + _init_platform (&platform, external_command); + + if (external_command) { + gs_free char *dev = NULL; + gs_free char *local = NULL, *remote = NULL; + + g_assert (lnk->persist); + + err = nmtstp_run_command ("ip tuntap add" + " mode %s" + "%s" /* user */ + "%s" /* group */ + "%s" /* pi */ + "%s" /* vnet_hdr */ + "%s" /* multi_queue */ + " name %s", + lnk->type == IFF_TUN ? "tun" : "tap", + lnk->owner_valid ? nm_sprintf_bufa (100, " user %u", (guint) lnk->owner) : "", + lnk->group_valid ? nm_sprintf_bufa (100, " group %u", (guint) lnk->group) : "", + lnk->pi ? " pi" : "", + lnk->vnet_hdr ? " vnet_hdr" : "", + lnk->multi_queue ? " multi_queue" : "", + name); + /* Older versions of iproute2 don't support adding devices. + * On failure, fallback to using platform code. */ + if (err == 0) + pllink = nmtstp_assert_wait_for_link (platform, name, NM_LINK_TYPE_TUN, 100); + else + g_error ("failure to add tun/tap device via ip-route"); + } else { + plerr = nm_platform_link_tun_add (platform, name, lnk, &pllink); + g_assert_cmpint (plerr, ==, NM_PLATFORM_ERROR_SUCCESS); + } + + g_assert (pllink); + g_assert_cmpint (pllink->type, ==, NM_LINK_TYPE_TUN); + g_assert_cmpstr (pllink->name, ==, name); + return pllink; +} + const NMPlatformLink * nmtstp_link_vxlan_add (NMPlatform *platform, gboolean external_command, diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h index 2b830cf196..a8e126f40b 100644 --- a/src/platform/tests/test-common.h +++ b/src/platform/tests/test-common.h @@ -307,6 +307,10 @@ const NMPlatformLink *nmtstp_link_sit_add (NMPlatform *platform, gboolean external_command, const char *name, const NMPlatformLnkSit *lnk); +const NMPlatformLink *nmtstp_link_tun_add (NMPlatform *platform, + gboolean external_command, + const char *name, + const NMPlatformLnkTun *lnk); const NMPlatformLink *nmtstp_link_vxlan_add (NMPlatform *platform, gboolean external_command, const char *name, diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index 90bb823ddb..a44936e5dc 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -23,6 +23,7 @@ #include #include #include +#include #include "platform/nmp-object.h" #include "platform/nmp-netns.h" @@ -697,6 +698,7 @@ test_software_detect (gconstpointer user_data) const NMPObject *lnk; guint i_step; const gboolean ext = test_data->external_command; + NMPlatformLnkTun lnk_tun; nmtstp_run_command_check ("ip link add %s type dummy", PARENT_NAME); ifindex_parent = nmtstp_assert_wait_for_link (NM_PLATFORM_GET, PARENT_NAME, NM_LINK_TYPE_DUMMY, 100)->ifindex; @@ -892,6 +894,35 @@ test_software_detect (gconstpointer user_data) g_assert (nmtstp_link_vxlan_add (NULL, ext, DEVICE_NAME, &lnk_vxlan)); break; } + case NM_LINK_TYPE_TUN: { + gboolean owner_valid = nmtst_get_rand_bool (); + gboolean group_valid = nmtst_get_rand_bool (); + + switch (test_data->test_mode) { + case 0: + lnk_tun = (NMPlatformLnkTun) { + .type = nmtst_get_rand_bool () ? IFF_TUN : IFF_TAP, + .owner = owner_valid ? getuid () : 0, + .owner_valid = owner_valid, + .group = group_valid ? getgid () : 0, + .group_valid = group_valid, + .pi = nmtst_get_rand_bool (), + .vnet_hdr = nmtst_get_rand_bool (), + .multi_queue = nmtst_get_rand_bool (), + + /* arguably, adding non-persistant devices from within NetworkManager makes + * no sense. */ + .persist = TRUE, + }; + break; + default: + g_assert_not_reached (); + break; + } + + g_assert (nmtstp_link_tun_add (NULL, ext, DEVICE_NAME, &lnk_tun)); + break; + } default: g_assert_not_reached (); } @@ -922,7 +953,13 @@ test_software_detect (gconstpointer user_data) lnk = nm_platform_link_get_lnk (NM_PLATFORM_GET, ifindex, test_data->link_type, &plink); g_assert (plink); g_assert_cmpint (plink->ifindex, ==, ifindex); - g_assert (lnk); + + if ( !lnk + && test_data->link_type == NM_LINK_TYPE_TUN) { + /* this is ok. Kernel apparently does not support tun properties via netlink. We + * fetch them from sysfs below. */ + } else + g_assert (lnk); switch (test_data->link_type) { case NM_LINK_TYPE_GRE: { @@ -1011,6 +1048,27 @@ test_software_detect (gconstpointer user_data) g_assert_cmpint (plnk->path_mtu_discovery, ==, FALSE); break; } + case NM_LINK_TYPE_TUN: { + const NMPlatformLnkTun *plnk; + NMPlatformLnkTun lnk_tun2; + + g_assert ((lnk ? &lnk->lnk_tun : NULL) == nm_platform_link_get_lnk_tun (NM_PLATFORM_GET, ifindex, NULL)); + + /* kernel might not expose tun options via netlink. Either way, try + * to read them (either from platform cache, or fallback to sysfs). + * See also: rh#1547213. */ + if (!nm_platform_link_tun_get_properties (NM_PLATFORM_GET, + ifindex, + &lnk_tun2)) + g_assert_not_reached (); + + plnk = lnk ? &lnk->lnk_tun : &lnk_tun2; + if (lnk) + g_assert (memcmp (plnk, &lnk_tun2, sizeof (NMPlatformLnkTun)) == 0); + + g_assert (nm_platform_lnk_tun_cmp (plnk, &lnk_tun) == 0); + break; + } case NM_LINK_TYPE_VLAN: { const NMPlatformLnkVlan *plnk = &lnk->lnk_vlan; @@ -2581,6 +2639,7 @@ _nmtstp_setup_tests (void) test_software_detect_add ("/link/software/detect/macvlan", NM_LINK_TYPE_MACVLAN, 0); test_software_detect_add ("/link/software/detect/macvtap", NM_LINK_TYPE_MACVTAP, 0); test_software_detect_add ("/link/software/detect/sit", NM_LINK_TYPE_SIT, 0); + test_software_detect_add ("/link/software/detect/tun", NM_LINK_TYPE_TUN, 0); test_software_detect_add ("/link/software/detect/vlan", NM_LINK_TYPE_VLAN, 0); test_software_detect_add ("/link/software/detect/vxlan/0", NM_LINK_TYPE_VXLAN, 0); test_software_detect_add ("/link/software/detect/vxlan/1", NM_LINK_TYPE_VXLAN, 1); From 770015f512d4798f7d2e060db613586ee382ba65 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 19:02:18 +0200 Subject: [PATCH 11/14] shared/tests: add nmtst_assert_nonnull() macro There is g_assert_nonnull(), however that doesn't return the pointer. Returning the pointer can be convenient... --- shared/nm-utils/nm-test-utils.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 15671643da..f44f49d835 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -162,6 +162,14 @@ g_assert_not_reached (); \ } G_STMT_END +#define nmtst_assert_nonnull(command) \ + ({ \ + typeof (*(command)) *_ptr = (command); \ + \ + g_assert (_ptr && (TRUE || (command))); \ + _ptr; \ + }) + #define nmtst_assert_success(success, error) \ G_STMT_START { \ g_assert_no_error (error); \ From dd2f5cf3d40a0fe2b293c0404ef0577f2e0b1a86 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 19:07:10 +0200 Subject: [PATCH 12/14] platform/tests: implement nmtstp_assert_wait_for_link() as macro Implement nmtstp_assert_wait_for_link() and nmtstp_assert_wait_for_link_until() as macros, based on nmtst_assert_nonnull(). This way, the assertion will report a more helpful file:line location, instead of being somewhere nested inside test-common.c. --- src/platform/tests/test-common.c | 16 ---------------- src/platform/tests/test-common.h | 7 +++++-- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index 4310ef39fb..c480fb74a2 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -654,22 +654,6 @@ nmtstp_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType } } -const NMPlatformLink * -nmtstp_assert_wait_for_link (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, guint timeout_ms) -{ - return nmtstp_assert_wait_for_link_until (platform, ifname, expected_link_type, nm_utils_get_monotonic_timestamp_ms () + timeout_ms); -} - -const NMPlatformLink * -nmtstp_assert_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, gint64 until_ms) -{ - const NMPlatformLink *plink; - - plink = nmtstp_wait_for_link_until (platform, ifname, expected_link_type, until_ms); - g_assert (plink); - return plink; -} - /*****************************************************************************/ int diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h index a8e126f40b..e7e8b127b5 100644 --- a/src/platform/tests/test-common.h +++ b/src/platform/tests/test-common.h @@ -129,8 +129,11 @@ const NMPlatformLink *nmtstp_wait_for_link_until (NMPlatform *platform, const ch g_assert_not_reached (); \ } G_STMT_END -const NMPlatformLink *nmtstp_assert_wait_for_link (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, guint timeout_ms); -const NMPlatformLink *nmtstp_assert_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, gint64 until_ms); +#define nmtstp_assert_wait_for_link(platform, ifname, expected_link_type, timeout_ms) \ + nmtst_assert_nonnull (nmtstp_wait_for_link (platform, ifname, expected_link_type, timeout_ms)) + +#define nmtstp_assert_wait_for_link_until(platform, ifname, expected_link_type, until_ms) \ + nmtst_assert_nonnull (nmtstp_wait_for_link_until (platform, ifname, expected_link_type, until_ms)) /*****************************************************************************/ From f21ff48a8495a07bd04de76d395d7512f3cb7941 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 18:56:09 +0200 Subject: [PATCH 13/14] platform/tests: extend nmtstp_wait_for_link*() to never wait Previously, it was not (reliably) possible to use nmtstp_wait_for_link*() to only look into the platform cache, without trying to poll the netlink socket for events. Add this option. Now, if the timeout is specified as zero, we never actually read the netlink socket. Currently, there are no callers who make use of this (by passing a zero timeout). So, this is no change in existing behavior. --- src/platform/tests/test-common.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index c480fb74a2..f54c0dd4c1 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -628,7 +628,10 @@ nmtstp_wait_for_signal_until (NMPlatform *platform, gint64 until_ms) const NMPlatformLink * nmtstp_wait_for_link (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, gint64 timeout_ms) { - return nmtstp_wait_for_link_until (platform, ifname, expected_link_type, nm_utils_get_monotonic_timestamp_ms () + timeout_ms); + return nmtstp_wait_for_link_until (platform, ifname, expected_link_type, + timeout_ms + ? nm_utils_get_monotonic_timestamp_ms () + timeout_ms + : 0); } const NMPlatformLink * @@ -636,6 +639,7 @@ nmtstp_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType { const NMPlatformLink *plink; gint64 now; + gboolean waited_once = FALSE; _init_platform (&platform, FALSE); @@ -647,9 +651,20 @@ nmtstp_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType && (expected_link_type == NM_LINK_TYPE_NONE || plink->type == expected_link_type)) return plink; - if (until_ms < now) + if (until_ms == 0) { + /* don't wait, don't even poll the socket. */ return NULL; + } + if ( waited_once + && until_ms < now) { + /* timeout reached (+ we already waited for a signal at least once). */ + return NULL; + } + + waited_once = TRUE; + /* regardless of whether timeout is already reached, we poll the netlink + * socket a bit. */ nmtstp_wait_for_signal (platform, until_ms - now); } } From ef93f6caad0552b8bd73057caed52147dac5eda0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 Apr 2018 18:23:43 +0200 Subject: [PATCH 14/14] platform: support creating non-persistant TUN/TAP devices For completeness, extend the API to support non-persistant device. That requires that nm_platform_link_tun_add() returns the file descriptor. While NetworkManager doesn't create such devices itself, it recognizes the IFLA_TUN_PERSIST / IFF_PERSIST flag. Since ip-tuntap (obviously) cannot create such devices, we cannot add a test for how non-persistent devices look in the platform cache. Well, we could instead add them with ioctl directly, but instead, just extend the platform API to allow for that. Also, use the function from test-lldp.c to (optionally) use nm_platform_link_tun_add() to create the tap device. --- src/devices/nm-device-tun.c | 3 +- src/devices/tests/test-lldp.c | 50 +++++++++++++++++++++++--------- src/platform/nm-linux-platform.c | 23 +++++++++------ src/platform/nm-platform.c | 17 +++++++++-- src/platform/nm-platform.h | 6 ++-- src/platform/tests/test-common.c | 7 +++-- src/platform/tests/test-common.h | 3 +- src/platform/tests/test-link.c | 12 +++++--- 8 files changed, 87 insertions(+), 34 deletions(-) diff --git a/src/devices/nm-device-tun.c b/src/devices/nm-device-tun.c index 123455f16d..c3ce4b7357 100644 --- a/src/devices/nm-device-tun.c +++ b/src/devices/nm-device-tun.c @@ -264,7 +264,8 @@ create_and_realize (NMDevice *device, plerr = nm_platform_link_tun_add (nm_device_get_platform (device), iface, &props, - out_plink); + out_plink, + NULL); if (plerr != NM_PLATFORM_ERROR_SUCCESS) { g_set_error (error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_CREATION_FAILED, "Failed to create TUN/TAP interface '%s' for '%s': %s", diff --git a/src/devices/tests/test-lldp.c b/src/devices/tests/test-lldp.c index 655f26ecad..5d0625f80a 100644 --- a/src/devices/tests/test-lldp.c +++ b/src/devices/tests/test-lldp.c @@ -347,8 +347,7 @@ static void _test_recv_fixture_setup (TestRecvFixture *fixture, gconstpointer user_data) { const NMPlatformLink *link; - struct ifreq ifr = { }; - int fd, s; + nm_auto_close int fd = -1; fd = open ("/dev/net/tun", O_RDWR | O_CLOEXEC); if (fd == -1) { @@ -357,20 +356,45 @@ _test_recv_fixture_setup (TestRecvFixture *fixture, gconstpointer user_data) return; } - ifr.ifr_flags = IFF_TAP | IFF_NO_PI; - nm_utils_ifname_cpy (ifr.ifr_name, TEST_IFNAME); - g_assert (ioctl (fd, TUNSETIFF, &ifr) >= 0); + if (nmtst_get_rand_bool ()) { + const NMPlatformLnkTun lnk = { + .type = IFF_TAP, + .pi = FALSE, + .vnet_hdr = FALSE, + .multi_queue = FALSE, + .persist = FALSE, + }; - /* Bring the interface up */ - s = socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); - g_assert (s >= 0); - ifr.ifr_flags |= IFF_UP; - g_assert (ioctl (s, SIOCSIFFLAGS, &ifr) >= 0); - nm_close (s); + nm_close (nm_steal_fd (&fd)); + + link = nmtstp_link_tun_add (NM_PLATFORM_GET, + FALSE, + TEST_IFNAME, + &lnk, + &fd); + g_assert (link); + nmtstp_link_set_updown (NM_PLATFORM_GET, -1, link->ifindex, TRUE); + link = nmtstp_assert_wait_for_link (NM_PLATFORM_GET, TEST_IFNAME, NM_LINK_TYPE_TUN, 0); + } else { + int s; + struct ifreq ifr = { }; + + ifr.ifr_flags = IFF_TAP | IFF_NO_PI; + nm_utils_ifname_cpy (ifr.ifr_name, TEST_IFNAME); + g_assert (ioctl (fd, TUNSETIFF, &ifr) >= 0); + + /* Bring the interface up */ + s = socket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); + g_assert (s >= 0); + ifr.ifr_flags |= IFF_UP; + g_assert (ioctl (s, SIOCSIFFLAGS, &ifr) >= 0); + nm_close (s); + + link = nmtstp_assert_wait_for_link (NM_PLATFORM_GET, TEST_IFNAME, NM_LINK_TYPE_TUN, 100); + } - link = nmtstp_assert_wait_for_link (NM_PLATFORM_GET, TEST_IFNAME, NM_LINK_TYPE_TUN, 100); fixture->ifindex = link->ifindex; - fixture->fd = fd; + fixture->fd = nm_steal_fd (&fd); memcpy (fixture->mac, link->addr.data, ETH_ALEN); } diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 8923708657..0ed8fa06fb 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -5596,15 +5596,15 @@ static gboolean link_tun_add (NMPlatform *platform, const char *name, const NMPlatformLnkTun *props, - const NMPlatformLink **out_link) + const NMPlatformLink **out_link, + int *out_fd) { const NMPObject *obj; struct ifreq ifr = { }; nm_auto_close int fd = -1; - if ( !NM_IN_SET (props->type, IFF_TAP, IFF_TUN) - || !props->persist) - g_return_val_if_reached (FALSE); + nm_assert (NM_IN_SET (props->type, IFF_TAP, IFF_TUN)); + nm_assert (props->persist || out_fd); fd = open ("/dev/net/tun", O_RDWR | O_CLOEXEC); if (fd < 0) @@ -5629,18 +5629,23 @@ link_tun_add (NMPlatform *platform, return FALSE; } - if (ioctl (fd, TUNSETPERSIST, (int) !!props->persist)) - return FALSE; + if (props->persist) { + if (ioctl (fd, TUNSETPERSIST, 1)) + return FALSE; + } do_request_link (platform, 0, name); obj = nmp_cache_lookup_link_full (nm_platform_get_cache (platform), 0, name, FALSE, NM_LINK_TYPE_TUN, NULL, NULL); - if (out_link) - *out_link = obj ? &obj->link : NULL; - return !!obj; + if (!obj) + return FALSE; + + NM_SET_OUT (out_link, &obj->link); + NM_SET_OUT (out_fd, nm_steal_fd (&fd)); + return TRUE; } static gboolean diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index e264324aa0..976039def4 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -2046,6 +2046,11 @@ nm_platform_link_vxlan_add (NMPlatform *self, * @vnet_hdr: whether to set the IFF_VNET_HDR flag * @multi_queue: whether to set the IFF_MULTI_QUEUE flag * @out_link: on success, the link object + * @out_fd: (allow-none): if give, return the file descriptor for the + * created device. Note that when creating a non-persistent device, + * this argument is mandatory, otherwise it makes no sense + * to create such an interface. + * The caller is responsible for closing this file descriptor. * * Create a TUN or TAP interface. */ @@ -2053,7 +2058,8 @@ NMPlatformError nm_platform_link_tun_add (NMPlatform *self, const char *name, const NMPlatformLnkTun *props, - const NMPlatformLink **out_link) + const NMPlatformLink **out_link, + int *out_fd) { char b[255]; NMPlatformError plerr; @@ -2062,6 +2068,13 @@ nm_platform_link_tun_add (NMPlatform *self, g_return_val_if_fail (name, NM_PLATFORM_ERROR_BUG); g_return_val_if_fail (props, NM_PLATFORM_ERROR_BUG); + g_return_val_if_fail (NM_IN_SET (props->type, IFF_TUN, IFF_TAP), NM_PLATFORM_ERROR_BUG); + + /* creating a non-persistant device requires that the caller handles + * the file descriptor. */ + g_return_val_if_fail (props->persist || out_fd, NM_PLATFORM_ERROR_BUG); + + NM_SET_OUT (out_fd, -1); plerr = _link_add_check_existing (self, name, NM_LINK_TYPE_TUN, out_link); if (plerr != NM_PLATFORM_ERROR_SUCCESS) @@ -2069,7 +2082,7 @@ nm_platform_link_tun_add (NMPlatform *self, _LOGD ("link: adding tun '%s' %s", name, nm_platform_lnk_tun_to_string (props, b, sizeof (b))); - if (!klass->link_tun_add (self, name, props, out_link)) + if (!klass->link_tun_add (self, name, props, out_link, out_fd)) return NM_PLATFORM_ERROR_UNSPECIFIED; return NM_PLATFORM_ERROR_SUCCESS; } diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 824735083a..e6cef63bae 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -853,7 +853,8 @@ typedef struct { gboolean (*link_tun_add) (NMPlatform *platform, const char *name, const NMPlatformLnkTun *props, - const NMPlatformLink **out_link); + const NMPlatformLink **out_link, + int *out_fd); gboolean (*infiniband_partition_add) (NMPlatform *, int parent, int p_key, const NMPlatformLink **out_link); gboolean (*infiniband_partition_delete) (NMPlatform *, int parent, int p_key); @@ -1286,7 +1287,8 @@ NMPlatformError nm_platform_link_sit_add (NMPlatform *self, NMPlatformError nm_platform_link_tun_add (NMPlatform *self, const char *name, const NMPlatformLnkTun *props, - const NMPlatformLink **out_link); + const NMPlatformLink **out_link, + int *out_fd); const NMPlatformIP6Address *nm_platform_ip6_address_get (NMPlatform *self, int ifindex, struct in6_addr address); diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index f54c0dd4c1..8724fedc86 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -1472,7 +1472,8 @@ const NMPlatformLink * nmtstp_link_tun_add (NMPlatform *platform, gboolean external_command, const char *name, - const NMPlatformLnkTun *lnk) + const NMPlatformLnkTun *lnk, + int *out_fd) { const NMPlatformLink *pllink = NULL; NMPlatformError plerr; @@ -1481,6 +1482,7 @@ nmtstp_link_tun_add (NMPlatform *platform, g_assert (nm_utils_is_valid_iface_name (name, NULL)); g_assert (lnk); g_assert (NM_IN_SET (lnk->type, IFF_TUN, IFF_TAP)); + g_assert (!out_fd || *out_fd == -1); if (!lnk->persist) { /* ip tuntap does not support non-persistent devices. @@ -1522,7 +1524,8 @@ nmtstp_link_tun_add (NMPlatform *platform, else g_error ("failure to add tun/tap device via ip-route"); } else { - plerr = nm_platform_link_tun_add (platform, name, lnk, &pllink); + g_assert (lnk->persist || out_fd); + plerr = nm_platform_link_tun_add (platform, name, lnk, &pllink, out_fd); g_assert_cmpint (plerr, ==, NM_PLATFORM_ERROR_SUCCESS); } diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h index e7e8b127b5..bd02b0d7d3 100644 --- a/src/platform/tests/test-common.h +++ b/src/platform/tests/test-common.h @@ -313,7 +313,8 @@ const NMPlatformLink *nmtstp_link_sit_add (NMPlatform *platform, const NMPlatformLink *nmtstp_link_tun_add (NMPlatform *platform, gboolean external_command, const char *name, - const NMPlatformLnkTun *lnk); + const NMPlatformLnkTun *lnk, + int *out_fd); const NMPlatformLink *nmtstp_link_vxlan_add (NMPlatform *platform, gboolean external_command, const char *name, diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index a44936e5dc..dcd600ee64 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -699,6 +699,7 @@ test_software_detect (gconstpointer user_data) guint i_step; const gboolean ext = test_data->external_command; NMPlatformLnkTun lnk_tun; + nm_auto_close int tun_fd = -1; nmtstp_run_command_check ("ip link add %s type dummy", PARENT_NAME); ifindex_parent = nmtstp_assert_wait_for_link (NM_PLATFORM_GET, PARENT_NAME, NM_LINK_TYPE_DUMMY, 100)->ifindex; @@ -910,9 +911,9 @@ test_software_detect (gconstpointer user_data) .vnet_hdr = nmtst_get_rand_bool (), .multi_queue = nmtst_get_rand_bool (), - /* arguably, adding non-persistant devices from within NetworkManager makes - * no sense. */ - .persist = TRUE, + /* if we add the device via iproute2 (external), we can only + * create persistent devices. */ + .persist = (ext == 1) ? TRUE : nmtst_get_rand_bool (), }; break; default: @@ -920,7 +921,10 @@ test_software_detect (gconstpointer user_data) break; } - g_assert (nmtstp_link_tun_add (NULL, ext, DEVICE_NAME, &lnk_tun)); + g_assert (nmtstp_link_tun_add (NULL, ext, DEVICE_NAME, &lnk_tun, + (!lnk_tun.persist || nmtst_get_rand_bool ()) + ? &tun_fd + : NULL)); break; } default: