When we must synchronize IPv6 addresses, we compare the order of
addresses to set with what is currently set on platform. Starting from
addresses with lower priority, when a mismatch is found we remove it
from platform and also remove all following addresses, so that we can
re-add them in the right order.
Since kernel keeps addresses internally sorted by scope, we should
consider each scope separately in order to avoid unnecessary address
deletions. For example, if we want to configure addresses
fe80::1/64,2000::1/64 and we currently have on platform 2000::1/64,
it's not necessary to remove the existing address; we can just add the
link-local one.
Co-authored-by: Thomas Haller <thaller@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1814557
(cherry picked from commit 0118ad5125)
NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
(cherry picked from commit 6f9a478b7d)
Surisingly, the compiler may detect the remaining obj_type in
the default switch. Then, inlining nmp_class_from_type() it may detect
that this is only possible to hit with an out or range access to
_nmp_classes array.
Rework the code to avoid that compiler warning. It's either way not
supposed to happen.
Also, drop the default switch case and explicitly list the enum values.
Otherwise it is error prone to forget a switch case.
(cherry picked from commit 9848589fbf)
nm_utils_is_valid_iface_name() is a public API of libnm-core, let's use
our internal API.
$ sed -i 's/\<nm_utils_is_valid_iface_name\>/nm_utils_ifname_valid_kernel/g' $(git grep -l nm_utils_is_valid_iface_name)
(cherry picked from commit 6e9a36ab9f)
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘nmp_utils_ethtool_get_permanent_address’:
src/platform/nm-platform-utils.c:854:29: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[0]’} [-Werror=zero-length-bounds]
854 | if (NM_IN_SET (edata.e.data[0], 0, 0xFF)) {
./shared/nm-glib-aux/nm-macros-internal.h:731:20: note: in definition of macro ‘_NM_IN_SET_EVAL_N’
Fix this warning.
(cherry picked from commit d892a35395)
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘ethtool_get_stringset’:
src/platform/nm-platform-utils.c:355:27: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[0]’} [-Werror=zero-length-bounds]
355 | len = sset_info.info.data[0];
| ~~~~~~~~~~~~~~~~~~~^~~
In file included from src/platform/nm-platform-utils.c:12:
/usr/include/linux/ethtool.h:647:8: note: while referencing ‘data’
647 | __u32 data[0];
| ^~~~
Fix this warning.
(cherry picked from commit 16e1e44c5e)
I think this solution is not right, because "char buf" is not guaranteed
to have the correct alignment. Revert, and solve it differently.
This reverts commit 6345a66153.
(cherry picked from commit 1fd7e45139)
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘nmp_utils_ethtool_get_permanent_address’:
src/platform/nm-platform-utils.c:854:29: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u8[0]’ {aka ‘unsigned char[0]’} [-Werror=zero-length-bounds]
854 | if (NM_IN_SET (edata.e.data[0], 0, 0xFF)) {
./shared/nm-glib-aux/nm-macros-internal.h:731:20: note: in definition of macro ‘_NM_IN_SET_EVAL_N’
Fix this warning.
(cherry picked from commit 5076fc0ca0)
GCC 10 complains about accesses to elements of zero-length arrays that
overlap other members of the same object:
src/platform/nm-platform-utils.c: In function ‘ethtool_get_stringset’:
src/platform/nm-platform-utils.c:355:27: error: array subscript 0 is outside the bounds of an interior zero-length array ‘__u32[0]’ {aka ‘unsigned int[0]’} [-Werror=zero-length-bounds]
355 | len = sset_info.info.data[0];
| ~~~~~~~~~~~~~~~~~~~^~~
In file included from src/platform/nm-platform-utils.c:12:
/usr/include/linux/ethtool.h:647:8: note: while referencing ‘data’
647 | __u32 data[0];
| ^~~~
Fix this warning.
(cherry picked from commit 6345a66153)
Previously NetworkManager would wrongly add a broadcast address for the
network prefix that would collide with the IP address of the host on
the other end of the point-to-point link thus exhausting the IP address
space of the /31 network and preventing communication between the two
nodes.
Configuring a /31 address before this commit:
IP addr -> 10.0.0.0/31, broadcast addr -> 10.0.0.1
If 10.0.0.1 is configured as a broadcast address the communication
with host 10.0.0.1 will not be able to take place.
Configuring a /31 address after this commit:
IP addr -> 10.0.0.0/31, no broadcast address
Thus 10.0.0.0/31 and 10.0.0.1/31 are able to correctly communicate.
See RFC-3021. https://tools.ietf.org/html/rfc3021https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/295https://bugzilla.redhat.com/show_bug.cgi?id=1764986
(cherry picked from commit fa144b5ae9)
This is necessary on Travis/Ubuntu 16.04, otherwise the test
fails with
# NetworkManager-MESSAGE: <warn> [1575301791.7600] platform-linux: do-add-link[nm-test-device/team]: failure 95 (Operation not supported)
Aborted (core dumped)
# test:ERROR:../src/platform/tests/test-link.c:353:test_software: assertion failed: (software_add (link_type, DEVICE_NAME))
ERROR: src/platform/tests/test-link-linux - too few tests run (expected 76, got 6)
(cherry picked from commit f7e3cc0b71)
- systemd-networkd and initscripts both support it.
- it seems suggested to configure routes with scope "link" on AWS.
- the scope is only supported for IPv4 routes. Kernel ignores the
attribute for IPv6 routes.
- we don't support the aliases like "link" or "global". Instead
only the numeric value is supported. This is different from
systemd-networkd, which accepts names like "global" and "link",
but no numerical values. I think restricting ourself only to
the aliases unnecessarily limits what is possible on netlink.
The alternative would be to allow aliases and numbers both,
but that causes multiple ways to define something and has
thus downsides. So, only numeric values.
- when setting rtm_scope to RT_SCOPE_NOWHERE (0, the default), kernel
will coerce that to RT_SCOPE_LINK. This ambiguity of nowhere vs. link
is a problem, but we don't do anything about it.
- The other problem is, that when deleting a route with scope RT_SCOPE_NOWHERE,
this acts as a wild care and removes the first route that matches (given the
other route attributes). That means, NetworkManager has no meaningful
way to delete a route with scope zero, there is always the danger that
we might delete the wrong route. But this is nothing new to this
patch. The problem existed already previously, except that
NetworkManager could only add routes with scope nowhere (i.e. link).
Track whether IP addresses were added by NM or externally. In this way
it becomes possible in a later commit to add prefix route only for
addresses added by NM.
The targets that involve the use of the `NetworkManager` library,
built in the `src` build file have been improved by applying a set
of changes:
- Indentation has been fixed.
- Set of objects used in targets have been grouped together.
- Aritificial dependencies used to group dependencies and custom
compiler flags have been removed and their use replaced with
proper dependencies and compiler flags to avoid any confussion.
IPv6 router advertisement messages contain the following parameters
(RFC 4861):
- Reachable time: 32-bit unsigned integer. The time, in
milliseconds, that a node assumes a neighbor is reachable after
having received a reachability confirmation. Used by the Neighbor
Unreachability Detection algorithm. A value of zero means
unspecified (by this router).
- Retrans Timer: 32-bit unsigned integer. The time, in milliseconds,
between retransmitted Neighbor Solicitation messages. Used by
address resolution and the Neighbor Unreachability Detection
algorithm. A value of zero means unspecified (by this router).
Currently NM ignores them; however, since it leaves accept_ra=1, the
kernel parses RAs and applies those parameters for us [1].
In the next commit kernel handling of RAs will be disabled, so let NM
set those neighbor-related parameters.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/ndisc.c?h=v5.2#n1353
... and nm_utils_fd_get_contents() and nm_utils_file_set_contents().
Don't mix negative errno return value with a GError output. Instead,
return a boolean result indicating success or failure.
Also, optionally
- output GError
- set out_errsv to the positive errno (or 0 on success)
Obviously, the return value and the output arguments (contents, length,
out_errsv, error) must all agree in their success/failure result.
That means, you may check any of the return value, out_errsv, error, and
contents to reliably detect failure or success.
Also note that out_errsv gives the positive(!) errno. But you probably
shouldn't care about the distinction and use nm_errno_native() either
way to normalize the value.
Seen on gitlab-ci.
NMPlatformSignalAssert: ../src/platform/tests/test-link.c:260, test_slave(): failure to accept signal [0,2] times: link-changed-changed ifindex 15 (3 times received)
ERROR: src/platform/tests/test-link-linux - too few tests run (expected 76, got 6)
ERROR: src/platform/tests/test-link-linux - exited with status 133 (terminated by signal 5?)
(cherry picked from commit 483de2bb93)
nl80211_alloc_msg() just allocates some memory, using glib's allocators.
Hence it cannot fail, and we don't need to check for that.
Drop the unnecessary %NULL checks.
(cherry picked from commit 990a7bee9d)
Usually we check the result of nla_nest_start(). Also, in most cases where this
function would return %NULL, it's an actual bug. That is, because our netlink
message is allocated with a large buffer, and in most cases we append there a well
known, small amount of data.
To make coverity happy, handle the case and assert.
(cherry picked from commit 243458836a)
NMPlatformObject is a base-type of all actual platform structs.
We very seldomly use this type directly. Most callers that pass
the plobj to nmp_object_new() will need to cast it.
Make the varible a void pointer to not require the cast.
We usually want to combine the fields from "struct timespec" to
have one timestamp in either nanoseconds or milliseconds.
Use nm_utils_clock_gettime_*() util for that.
IP addresses, routes, TC and QDiscs are all tied to a certain interface.
So when NetworkManager manages an interface, it can be confident that
all related entires should be managed, deleted and modified by NetworkManager.
Routing policy rules are global. For that we have NMPRulesManager which
keeps track of whether NetworkManager owns a rule. This allows multiple
connection profiles to specify the same rule, and NMPRulesManager can
consolidate this information to know whether to add or remove the rule.
NMPRulesManager would also support to explicitly block a rule by
tracking it with negative priority. However that is still unused at
the moment. All that devices do is to add rules (track with positive
priority) and remove them (untrack) once the profile gets deactivated.
As rules are not exclusively owned by NetworkManager, NetworkManager
tries not to interfere with rules that it knows nothing about. That
means in particular, when NetworkManager starts it will "weakly track"
all rules that are present. "weakly track" is mostly interesting for two
cases:
- when NMPRulesManager had the same rule explicitly tracked (added) by a
device, then deactivating the device will leave the rule in place.
- when NMPRulesManager had the same rule explicitly blocked (tracked
with negative priority), then it would restore the rule when that
block gets removed (as said, currently nobody actually does this).
Note that when restarting NetworkManager, then the device may stay and
the rules kept. However after restart, NetworkManager no longer knows
that it previously added this route, so it would weakly track it and
never remove them again.
That is a problem. Avoid that, by whenever explicitly tracking a rule we
also make sure to no longer weakly track it. Most likely this rule was
indeed previously managed by NetworkManager. If this was really a rule
added by externally, then the user really should choose distinct
rule priorities to avoid such conflicts altogether.
nmtst: initialize nmtst_get_rand() with NMTST_SEED_RAND=0
/link/bogus: OK
/link/loopback: OK
/link/internal: OK
/link/external: OK
/link/software/bridge: OK
/link/software/bond: OK
/link/software/team: NMPlatformSignalAssert: ../src/platform/tests/test-link.c:331, test_slave(): failure to accept signal [0,2] times: 'link-changed-changed' ifindex 15 (3 times received)
--- stderr ---
/builds/NetworkManager/NetworkManager/tools/run-nm-test.sh: line 264: 106682 Trace/breakpoint trap --quiet --error-exitcode= --leak-check=full --gen-suppressions=all --num-callers=100 --log-file=
The test failed. Also check the valgrind log at '/builds/NetworkManager/NetworkManager/build/src/platform/tests/test-link-linux.valgrind-log'
Drop nm_platform_link_get_address_as_bytes() and introduce
nmp_link_address_get_as_bytes() so that it becomes possible to obtain
also the broadcast address without an additional lookup of the link.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
While at it, rename the "addr" field to "l_address". The term "addr" is
used over and over. Instead we should use distinct names that make it
easier to navigate the code.
nmtst_get_rand_int() was originally named that way, because it
calls g_rand_int(). But I think if a function returns an uint32, it
should also be named that way.
Rename.
