For routes and routing rules, kernel uses a certain (not stictly defined) set
of attributes to decide whether to routes/rules are identical.
That is a problem, as different kernel versions disagree on whether
two routes/rules are the same (EEXIST) or not.
Note that when NetworkManager tries to add a rule with protocol set to
anything but RTPROT_UNSPEC, then kernel will ignore the attribute if it
doesn't have support for it. Meaning: the added rule will have a
different protocol setting then intended.
Note that NMPRulesManager will add a rule if it doesn't find it in the
platform cache so far. That means, when looking into the platform cache
we must ignore or honor the protocol like kernel does.
This does not only affect FRA_PROTOCOL, but all attributes where kernel
and NetworkManager disagrees. But the protocol is the most prominent
one, because the rules tracked by nmp_rules_manager_track_default()
specify the protocol.
(cherry picked from commit ef4f8ccf6d)
Next we will need to detect more kernel features. First refactor the
handling of these to require less code changes and be more efficient.
A plain nm_platform_kernel_support_get() only reqiures to access an
array in the common case.
The other important change is that the function no longer requires a
NMPlatform instance. This allows us to check kernel support from
anywhere. The only thing is that we require kernel support to be
initialized before calling this function. That means, an NMPlatform
instance must have detected support before.
(cherry picked from commit ee269b318e)
- if there is only one vlan in the list, then we can return success
early. That is, because one NMBridgeVlan instance is always valid
due to the way how users must use the API to construct the element.
- the implementation for check_normalizable is only correct, if there
are no duplicate or overlapping ranges. Assert for that. In fact,
all callers first check for errors and then for normalizable errors.
- avoid duplicate calls to nm_bridge_vlan_get_vid_range(). There are
duplicate assertions that we don't need.
- only check for pvid once per range.
- combine calls to g_hash_table_contains() and g_hash_table_add().
(cherry picked from commit a358da096f)
We don't need GPtrArray to construct an array of fixed side.
Actually, we also don't need to malloc each NMPlatformBridgeVlan
element individually. Just allocate one buffer and append them
to the end.
(cherry picked from commit 6bc8ee87af)
In some cases it is convenient to specify ranges of bridge vlans, as
already supported by iproute2 and natively by kernel. With this commit
it becomes possible to add a range in this way:
nmcli connection modify eth0-slave +bridge-port.vlans "100-200 untagged"
vlan ranges can't be PVIDs because only one PVID vlan can exist.
https://bugzilla.redhat.com/show_bug.cgi?id=1652910
(cherry picked from commit 7093515777)
CC libnm-core/tests/libnm_core_tests_test_general-test-general.o
In file included from ../shared/nm-default.h:280:0,
from ../libnm-core/tests/test-general.c:24:
../libnm-core/tests/test-general.c: In function _sock_addr_endpoint:
../libnm-core/tests/test-general.c:5911:18: error: logical not is only applied to the left hand side of comparison [-Werror=logical-not-parentheses]
g_assert (!host == (port == -1));
^
../shared/nm-utils/nm-macros-internal.h:1793:7: note: in definition of macro __NM_G_BOOLEAN_EXPR_IMPL
if (expr) \
^
/usr/include/glib-2.0/glib/gmacros.h:376:43: note: in expansion of macro _G_BOOLEAN_EXPR
#define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR((expr)), 1))
^
/usr/include/glib-2.0/glib/gtestutils.h:116:49: note: in expansion of macro G_LIKELY
if G_LIKELY (expr) ; else \
^
../libnm-core/tests/test-general.c:5911:2: note: in expansion of macro g_assert
g_assert (!host == (port == -1));
^
Fixes: 713e879d76 ('libnm: add NMSockAddrEndpoint API')
(cherry picked from commit 1e8c08730f)
CC src/settings/plugins/ifcfg-rh/src_settings_plugins_ifcfg_rh_libnms_ifcfg_rh_core_la-nms-ifcfg-rh-reader.lo
In file included from ../shared/nm-default.h:280:0,
from ../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:21:
../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c: In function read_routing_rules_parse:
../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:4309:27: error: logical not is only applied to the left hand side of comparison [-Werror=logical-not-parentheses]
nm_assert (!key_is_ipv4 == NM_STR_HAS_PREFIX (key, "ROUTING_RULE6_"));
^
../shared/nm-utils/nm-macros-internal.h:1793:7: note: in definition of macro __NM_G_BOOLEAN_EXPR_IMPL
if (expr) \
^
/usr/include/glib-2.0/glib/gmacros.h:376:43: note: in expansion of macro _G_BOOLEAN_EXPR
#define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR((expr)), 1))
^
/usr/include/glib-2.0/glib/gtestutils.h:116:49: note: in expansion of macro G_LIKELY
if G_LIKELY (expr) ; else \
^
../shared/nm-utils/nm-macros-internal.h:973:40: note: in expansion of macro g_assert
#define nm_assert(cond) G_STMT_START { g_assert (cond); } G_STMT_END
^
../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:4309:3: note: in expansion of macro nm_assert
nm_assert (!key_is_ipv4 == NM_STR_HAS_PREFIX (key, "ROUTING_RULE6_"));
^
Fixes: 4d46804437
(cherry picked from commit c6e6dcae70)
This can be replaced by nm_utils_escaped_tokens_split().
Note that nm_utils_escaped_tokens_split() does not behave exactly
the same. For example, nm_utils_str_simpletokens_extract_next() would
remove all backslashes and leave only the following character.
nm_utils_escaped_tokens_split() instead only strips backslashes
that preceed a delimiter, whitespace or another backslash.
But we should have one preferred way of tokenizing, and I find this
preferable, because it allows for most backslashes to appear verbatim.
(cherry picked from commit ced7dbe8bf)
Optimally, all list types properties in nmcli support proper escaping.
For that, we should use nm_utils_escaped_tokens_*() API.
For now, just change that for policy routes. They were just added recently,
so no change in behavior of released API.
(cherry picked from commit d59f046bb6)
Replace nm_utils_str_simpletokens_extract_next() by
nm_utils_escaped_tokens_split().
nm_utils_escaped_tokens_split() should become our first choice for
parsing and tokenizing.
Note that both nm_utils_str_simpletokens_extract_next() and
nm_utils_escaped_tokens_split() need to strdup the string once,
and tokenizing takes O(n). So, they are roughtly the same performance
wise. The only difference is, that as we iterate through the tokens,
we might abort early on error with nm_utils_str_simpletokens_extract_next()
and not parse the entire string. But that is a small benefit, since we
anyway always strdup() the string (being O(n) already).
Note that to-string will no longer escape ',' and ';'. This is a change
in behavior, of unreleased API. Also note, that escaping these is no
longer necessary, because nmcli soon will also use nm_utils_escaped_tokens_*().
Another change in behavior is that nm_utils_str_simpletokens_extract_next()
treated invalid escape sequences (backslashes followed by an arbitrary
character), buy stripping the backslash. nm_utils_escaped_tokens_*()
leaves such backslashes as is, and only honors them if they are followed
by a whitespace (the delimiter) or another backslash. The disadvantage
of the new approach is that backslashes are treated differently
depending on the following character. The benefit is, that most
backslashes can now be written verbatim, not requiring them to escape
them with a double-backslash.
Yes, there is a problem with these nested escape schemes:
- the caller may already need to escape backslash in shell.
- then nmcli will use backslash escaping to split the rules at ','.
- then nm_ip_routing_rule_from_string() will honor backslash escaping
for spaces.
- then iifname and oifname use backslash escaping for nm_utils_buf_utf8safe_escape()
to express non-UTF-8 characters (because interface names are not
necessarily UTF-8).
This is only redeamed because escaping is really only necessary for very
unusual cases, if you want to embed a backslash, a space, a comma, or a
non-UTF-8 character. But if you have to, now you will be able to express
that.
The other upside of these layers of escaping is that they become all
indendent from each other:
- shell can accept quoted/escaped arguments and will unescape them.
- nmcli can do the tokenizing for ',' (and escape the content
unconditionally when converting to string).
- nm_ip_routing_rule_from_string() can do its tokenizing without
special consideration of utf8safe escaping.
- NMIPRoutingRule takes iifname/oifname as-is and is not concerned
about nm_utils_buf_utf8safe_escape(). However, before configuring
the rule in kernel, this utf8safe escape will be unescaped to get
the interface name (which is non-UTF8 binary).
(cherry picked from commit b6d0be2d3b)
nmcli supports list options (optlist and multilist properties).
These commonly are individual items, concatenated by a delimiter.
It should be generally possibly to express every value. That means, we
need some for of escaping mechanism for delimiters.
Currently this is all inconsistent or no escaping is supported. I intend
to fix that (which will be a change in behavior).
For now, just add yet another style of tokenzing/concatenating list
items in nmcli. This is the style to replace all other styles.
(cherry picked from commit ba956bd499)
This escapes strings so that they can be concatenated with a delimiter
and without loss tokenized with nm_utils_escaped_tokens_split().
Note that this is similar to _nm_utils_escape_plain() and
_nm_utils_escape_spaces(). The difference is that
nm_utils_escaped_tokens_escape() also escapes the last trailing
whitespace. That means, if delimiters contains all NM_ASCII_SPACES, then
it is identical to _nm_utils_escape_spaces(). Otherwise, the trailing
space is treated specially. That is, because
nm_utils_escaped_tokens_split() uses NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP,
to strip leading and trailing whitespace. To still express a trailing
whitespace, the last whitespace must be escaped. Note that
NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP also honors escaping any whitespace
(not only at the last position), but when escaping we don't need to
escape them, because unescaped (non-trailing) whitespace are taken just
fine.
The pair nm_utils_escaped_tokens_split() and
nm_utils_escaped_tokens_escape() are proposed as default way of
tokenizing a list of items. For example, with
$ nmcli connection modify "$PROFILE" +ipv4.routing-rules 'priority 5 from 192.168.7.5/32 table 5, priority 6 iif a\, from 192.168.7.5/32 table 6'
Here we implement a to/from string function to handle one item
(nm_ip_routing_rule_{from,to}_string()). When such elements are combined with ',',
then we need to support an additional layer of escaping on top of that.
The advantage is that the indvidual to/from string functions are agnostic
to this second layer of escaping/tokenizing that nmcli employs to handle
a list of these items.
The disadvantage is that we possibly get multiple layers of backslash
escapings. That is only mitigated by the fact that nm_utils_escaped_tokens_*()
supports a syntax for which *most* characters don't need any special escaping.
Only delimiters, backslash, and the trailing space needs escaping, and
these are cases are expected to be few.
(cherry picked from commit e206e3d4d8)
Add a new flag that will remove escape characters after splitting
the string.
This implements a special kind of backslash escaping. It's not C escape
sequences (like '\n' or '\020'), but simply to take the special character
following the backslash verbatim. Note that the backslash is only
considered special, if it's followed by a delimiter, another backslash,
or a whitespace (in combination with %NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP).
The main purpose of this form of escaping is nmcli's list options, like
$ nmcli connection modify "$PROFILE" +ipv4.routing-rules 'priority 5 from 192.168.7.5/32 table 5, priority 6 iif a\, from 192.168.7.5/32 table 6'
It's a contrieved example, but the list options are a list of IP
addresses, rules, etc. They implement their own syntax for one element,
and are concatenated by ','. To support that one element may have
arbitrary characters (including the delimiter and whitespaces), nmcli
employs a tokenization with this special kind of escaping.
(cherry picked from commit 9522aaf226)
This will essentially call g_strstrip() on each token.
There are some specialties:
- if the resulting word is empty after stripping, then according to
%NM_UTILS_STRSPLIT_SET_FLAGS_PRESERVE_EMPTY, the empty token will be
removed. If that results in an empty string array, %NULL will be
returned.
- if %NM_UTILS_STRSPLIT_SET_FLAGS_ALLOW_ESCAPING is set, then
whitespace that is backslash escaped is not removed.
Since this is a post-operation that happens after tokeninzing, it
could be done as a separate function. And we already have this function:
_nm_utils_unescape_plain() and _nm_utils_unescape_spaces().
However, that is ugly for several reasons:
- the stripping should be part of the tokenizing, you shouldn't need
several steps.
- nm_utils_strsplit_set_full() returns a "const char **" which
indicates the strings must not be freed. However, it is perfectly
valid to modify the string inplace. Hence, the post-op function
would need to cast the strings to "char *", which seems ugly
(although we do that on many places, and it's guaranteed to work).
- _nm_utils_unescape_plain()/_nm_utils_unescape_spaces() is indeed
already used together with nm_utils_strsplit_set_full(). However,
it requires to initialize the cb_lookup buffer twice. I would expect
that initializing the cb_lookup buffer is a large portion of what
the function does already (for short strings).
This issue will be solved in the next commit by adding yet another flag
which allows to unescape.
(cherry picked from commit 5b2b0dcadf)
Policy routing rules are global, and unlike routes not tied to an interface by ifindex.
That means, while we take full control over all routes of an interface during a sync,
we need to consider that multiple parties can contribute to the global set of rules.
That might be muliple connection profiles providing the same rule, or rules that are added
externally by the user. NMPRulesManager mediates for that.
This is done by NMPRulesManager "tracking" rules.
Rules that are not tracked by NMPRulesManager are completely ignored (and
considered externally added).
When tracking a rule, the caller provides a track-priority. If multiple
parties track a rule, then the highest (absolute value of the) priority
wins.
If the highest track-priority is positive, NMPRulesManager will add the rule if
it's not present.
When the highest track-priority is negative, then NMPRulesManager will remove the
rule if it's present (enforce its absence).
The complicated part is, when a rule that was previously tracked becomes no
longer tracked. In that case, we need to restore the previous state.
If NetworkManager added the rule earlier, then untracking the rule
NMPRulesManager will remove the rule again (restore its previous absent
state).
By default, if NetworkManager had a negative tracking-priority and removed the
rule earlier (enforced it to be absent), then when the rule becomes no
longer tracked, NetworkManager will not restore the rule.
Consider: the user adds a rule externally, and then activates a profile that
enforces the absence of the rule (causing NetworkManager to remove it).
When deactivating the profile, by default NetworkManager will not
restore such a rule! It's unclear whether that is a good idea, but it's
also unclear why the rule is there and whether NetworkManager should
really restore it.
Add weakly tracked rules to account for that. A tracking-priority of
zero indicates such weakly tracked rules. The only difference between an untracked
rule and a weakly tracked rule is, that when NetworkManager earlier removed the
rule (due to a negative tracking-priority), it *will* restore weakly
tracked rules when the rules becomes no longer (negatively) tracked.
And it attmpts to do that only once.
Likewise, if the rule is weakly tracked and already exists when
NMPRulesManager starts posively tracking the rule, then it would not
remove again, when no longer positively tracking it.
- fix the argument type to be "gint32" and not "int".
- assert in nmp_rules_manager_track_default() for the input
arguments.
- use boolean bitfield in private data.
The name "priority" is overused. Also rules have a "priority", but that'
something else.
Rename the priority of how rules are tracked by NMPRuleManager to
"track_priority".
All that setting track-default does, is calling nmp_rules_manager_track_default()
when the rules are first accessed.
That is not right API. Since nmp_rules_manager_track_default() is already public
API (good), every caller that wishes this behavior should track these routes explicitly.
The call to nm_utils_parse_variant_attributes() is useless. The following
_tc_read_common_opts() call does the same thing. This was probably left
in place by accident.
Before:
# nmcli c modify eth666 tc.qdiscs root
Error: failed to modify tc.qdiscs: '(null)' is not a valid kind The valid syntax is: '[root | parent <handle>] [handle <handle>] <qdisc>'.
After:
# nmcli c modify eth666 tc.qdiscs root
Error: failed to modify tc.qdiscs: kind is missing. The valid syntax is: '[root | parent <handle>] [handle <handle>] <kind>'.
==16725==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000005a159f at pc 0x00000046fc1b bp 0x7fff6038f900 sp 0x7fff6038f8f0
READ of size 1 at 0x0000005a159f thread T0
#0 0x46fc1a in _do_test_unescape_spaces libnm-core/tests/test-general.c:7791
#1 0x46fe5b in test_nm_utils_unescape_spaces libnm-core/tests/test-general.c:7810
#2 0x7f4ac5fe7fc9 in test_case_run gtestutils.c:2318
#3 0x7f4ac5fe7fc9 in g_test_run_suite_internal gtestutils.c:2403
#4 0x7f4ac5fe7e83 in g_test_run_suite_internal gtestutils.c:2415
#5 0x7f4ac5fe7e83 in g_test_run_suite_internal gtestutils.c:2415
#6 0x7f4ac5fe8281 in g_test_run_suite gtestutils.c:2490
#7 0x7f4ac5fe82a4 in g_test_run (/lib64/libglib-2.0.so.0+0x772a4)
#8 0x48240d in main libnm-core/tests/test-general.c:7994
#9 0x7f4ac5dc9412 in __libc_start_main (/lib64/libc.so.6+0x24412)
#10 0x423ffd in _start (/home/bgalvani/work/NetworkManager/libnm-core/tests/test-general+0x423ffd)
0x0000005a159f is located 49 bytes to the right of global variable '*.LC370' defined in 'libnm-core/tests/test-general.c' (0x5a1560) of size 14
'*.LC370' is ascii string 'nick-5, green'
0x0000005a159f is located 1 bytes to the left of global variable '*.LC371' defined in 'libnm-core/tests/test-general.c' (0x5a15a0) of size 1
'*.LC371' is ascii string ''
SUMMARY: AddressSanitizer: global-buffer-overflow libnm-core/tests/test-general.c:7791 in _do_test_unescape_spaces
Leak detection adds unhelpful messages to the stderr of nmcli, making
tests fail. For example:
=================================================================
==17156==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 256 byte(s) in 2 object(s) allocated from:
#0 0x7f08c7e27c88 in realloc (/lib64/libasan.so.5+0xefc88)
#1 0x7f08c7546e7d in g_realloc (/lib64/libglib-2.0.so.0+0x54e7d)
We already have code that parses exactly this kinds of string:
nm_utils_parse_inaddr_prefix_bin(). Use it.
Also, it doesn't use g_strsplit_set() to separate a string at the first
'/'. Total overkill.
