The order of addresses matters. For "ipv4.addresses", the list
contains the primary address first. For "ipv6.addresses", the
order was reverted. This was also documented behavior.
The previous patch just changed behavior with respect to relative order
of static IPv6 addresses and autoconf6/DHCPv6. As we seem in the mood
for changing behavior, here is another one.
Now the addresses are interpreted in an order consistent with IPv4 and
how one might expect: preferred addresses first.
(cherry picked from commit 3d6b6aa317)
The order of addresses can matter for source address selection.
This is described in RFC 6724 section 5, but if the rules don't
determine a clear winner, the order matters.
Change the relative order of IPv6 addresses. Previously, we would prefer
autoconf6, over DHCPv6, over manual addresses. Now that got reverted
to make more sense and be consistent with IPv4.
Also, if we had multiple autoconf6 addresses (received at different
moments in time), then previously a newly received address would be
added with highest priority. Now, the older address will be preferred
and that order will be enforced (this can be a problem, see (*) below).
For IPv4, it's all simple and sensible. When we add addresses in kernel
via netlink, the first address (of a subnet) becomes the primary.
Note that we only control the order of addresses of the same subnet.
The addresses in ipv4.addresses" are sorted with primary address first.
In the same way is the order for addresses in NML3ConfigData and for
@known_addresses in nm_platform_ip_address_sync(), all primary-first.
Also, manual addresses are sorted with higher priority compared to DHCPv4
addresses (at least since NetworkManager 1.36). That means the way how we
merge NML3ConfigData makes sense (nm_l3_config_data_merge()) because we first
merge the static configuration, then the DHCPv4 configuration, where we just
append the lower priority DHCPv4 addresses.
For IPv6, the address priority is messed up. On netlink/kernel, the last added
address becomes the preferred one (we thus need to add them in the order of
lowest priority first). Consequently and historically, the IPv6 addresses in
@known_addresses parameter to nm_platform_ip_address_sync() were
lowest priority first. And so they were tracked in NML3ConfigData
and in the profile ("ipv6.addresses"). That is confusing.
Also, we usually want to merge NML3ConfigData with different priorities
(e.g. static configuration from the profile before autoconf6/DHCPv6),
as we do with IPv4. However, since internally IPv6 addresses are tracked in
reverse order, it means later NML3ConfigData would be appended and get effectively
a higher priority. That means, autoconf6 addresses were preferred over DHCPv6 and
over manual "ipv6.addresses", respectively. That seems undesirable and inconsistent
with IPv4. Change that. This is a change in behavior.
Note that changing the order of addresses means to remove and re-add
them in the right (inverse) order, with lease important first. This
means, when we add a new address with lower priority, we need to remove
all higher priority addresses temporarily, before readding them. That
is a problem(*).
Note that in the profile, "ipv6.addresses" is still tracked in reverse
order. This did not change, but might change later.
(cherry picked from commit 4a548423b9)
Next we are going to assert that the prefix length is valid.
The test needs to have valid prefix lengths too. Adjust.
(cherry picked from commit a850e438a7)
Of course, the prefix length cannot be larger than 32 or 128.
But as C does implicit conversions, a buggy prefix length can
lead to a (wrongly) valid prefix length.
Make the type uint32, to prevent that (at least for common cases,
unless you pass a huge 64 bit integer).
(cherry picked from commit 0cf9db42d4)
Some were duplicated. Drop those.
Some function were in an order where they required forward declarations.
Reorder.
(cherry picked from commit d7990b359b)
For convenience, most to-string methods call nm_utils_to_string_buffer_init().
This allows to omit the string buffer and use a global (thread-local)
buffer.
That "convenience" seems error prone. Start drop it.
Start by adding a g_return_if_reached() assertion to catch the cases
that use it.
(cherry picked from commit 27752bfd5b)
These string functions allow to omit the string buffer. This is for
convenience, to use a global (thread-local) buffer. I think that is
error prone and we should drop that "convenience" feature.
At various places, pass a stack allocated buffer.
(cherry picked from commit b87afac8e8)
I want to get rid of "_nm_utils_to_string_buffer" (or at least, limit
and control its use). Currently it's used all over the place only
to get the size of it. Add a define instead.
(cherry picked from commit 36e709c021)
We call sync many times. Often there is nothing to update. Check the
cache first, before (re) adding it.
Note that many addresses have a limited lifetime, that is, a lifetime
that keeps counting down with seconds granularity. For those (common)
cases we will only avoid the call to kernel if there are two syncs
within less than a second.
(cherry picked from commit 528a63d9cc)
It is rather unlikely, that we call this function with no existing
routes/addresses. Hence, usually this does not safe an allocation
of the GPtrArray.
However, it's slightly less code and makes more sense this way
(instead of checking afterwards, whether the array is empty and
destroy it).
(cherry picked from commit 6bc9b73c55)
The code is disabled at compile time. It's only useful for printf
debugging to modify the source to get more logging.
(cherry picked from commit fcb4033a81)
ASSUME is causing more troubles than benefits it provides. This patch is
dropping NM_L3_CFG_COMMIT_TYPE_ASSUME and assume_config_once. NM3LCfg
will commit as if the sys-iface-state is MANAGED.
This patch is part of the effort to remove ASSUME from NetworkManager.
After ASSUME is dropped when starting NetworkManager it will take full
control of the interface, re-configuring it. The interface will be
managed from the start instead of assumed and then managed.
This will solve the situations where an interface is half-up and then a
restart happens. When NetworkManager is back it won't add the missing
addresses (which is what assume does) so the interface will fail during
the activation and will require a full activation.
https://bugzilla.redhat.com/show_bug.cgi?id=2050216https://bugzilla.redhat.com/show_bug.cgi?id=2077605https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1196
(cherry picked from commit bf5927b978)
We already have a comparison of NMPlatformIPXAddress with the modes
"full" and "id". The former is needed to fully compare two addresses,
the latter as identity for tracking addresses in the cache.
In NetworkManager we also use the NMPlatformIP[46]Address structure to
track the addresses we want to configure. When we add them in kernel,
we will later see them in the platform cache. However, some fields
will be slightly different. For example, "addr_source" address will
always be "kernel", because that one is not a field we configure in
kernel. Also, the "n_ifa_flags" probably differ (getting "permanent"
and "secondary" flags).
Add a compare function that can ignore such differences.
Also add nm_platform_vtable_address for accessing the IPv4 and IPv6
methods generically (based on an "IS_IPv4" variable).
(cherry picked from commit ef1b60c061)
nmp_utils_lifetime_get() calculates the lifetime of addresses,
and it bases the result on a "now" timestamp.
If you have two addresses and calculate their expiry, then we want to
base it on top of the same "now" timestamp, meaning, we should
only call nm_utils_get_monotonic_timestamp_sec() once. This is also a
performance optimization. But much more importantly, when we make a
comparison at a certain moment, we need that all sides have the same
understanding of the current timestamp.
But nmp_utils_lifetime_get() does not always require the now timestamp.
And the caller doesn't know, whether it will need it (short of knowing
how nmp_utils_lifetime_get() is implemented). So, make the now parameter
an in/out argument. If we pass in an already valid now timestamp, use
that. Otherwise, fetch the current time and also return it.
(cherry picked from commit deb37401e9)
When attaching a bond port, kernel will reset the MTU of the port ([1],
[2]). Configuring a different MTU on the port seems not a sensible
thing for the user to do.
Still, before commit e67ddd826f ('device: commit MTU during stage2')
we would first attach the bond port before setting the MTU. That
changed, and now the MTU set by kernel wins.
Btw, this change in behavior happens because we attach the port in
stage3 (ip-config), which seems an ugly thing to do.
Anyway, fix this by setting the MTU after attaching the ports, but still
in stage3.
It is probably not sensible for the user to configure a different MTU.
Still, if the user requested it by configuration, we should apply it.
Note that NetworkManager has some logic to constrain the MTU based on
the parent/child and controller/port. In many regards however, NetworkManager
does not fully understand or enforce the correct MTU and relies on the
user to configure it correctly. After all, if the user misconfigures the
MTU, the setup will have problems anyway (and in many cases neither
kernel nor NetworkManager could know that the configuration is wrong).
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/bonding/bond_main.c?h=v5.17#n3603
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/bonding/bond_main.c?h=v5.17#n4372https://bugzilla.redhat.com/show_bug.cgi?id=2071985
Fixes: e67ddd826f ('device: commit MTU during stage2')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1199
(cherry picked from commit 6804c2ba04)
Fixes this error:
checking whether more special flags are required for pthreads... no
checking for PTHREAD_PRIO_INHERIT... yes
./configure: line 30294: ,as_fn_error: command not found
checking for a Python interpreter with version >= 3... python
checking for python... /usr/bin/python
Fixes: 3affccf29b ('tests: fix undefined references to pthread')
(cherry picked from commit a8284b1d3b)
Each connection object includes a 64KiB scratch buffer used for
receiving packets. When many instances of the client are created,
those buffers use a significant amount of memory. For example, 500
clients take ~30MiB of memory constantly reserved only for those
buffers.
Since the buffer is used only in the function and is never passed
outside, a stack allocation would suffice; however, it's not wise to
do such large allocations on the stack; dynamically allocate it.
https://github.com/nettools/n-dhcp4/issues/26https://github.com/nettools/n-dhcp4/pull/2764513e31c0
(cherry picked from commit a5a5654f18)
According to WPA3_Specification_v3.0 section 2.3, when operating in
WPA3-Personal transition mode an AP:
- shall set MFPC to 1, MFPR to 0.
Therefore, do not operate in WPA3-Personal transition mode when PMF is set to
disabled. This also provides a way to be compatible with some devices that are
not fully compatible with WPA3-Personal transition mode.
Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1186
(cherry picked from commit b6eb237a27)
The entire point of the dance in nm_platform_ip_address_sync() is to ensure that
conflicting IPv4 addresses are in their right order, that is, they have
the right primary/secondary flag.
Kernel only sets secondary flags for addresses that are in the same
subnet, and we also only care about the relative order of addresses
that are in the same subnet. In particular, because we rely on kernel's
"secondary" flag to implement this.
But kernel only treads addresses as secondary, if they share the exact
same subnet. For example, 192.168.0.5/24 and 192.168.0.6/25 would not
be treated as primary/secondary but just as unrelated addresses, even if
the address cleared of it's host part is the same.
This means, we must not only hash the network part of the addresses, but
also the prefix length. Implement that, by tracking the full NMPObject.
(cherry picked from commit 619dc2fcab)
None of the callers really handle the return value of nm_platform_ip_address_sync()
or whether the function encountered problems. What would they anyway do
about that?
For IPv4 we were already ignoring errors to add addresses, but for IPv6 we
aborted. That seems wrong. As the caller does not really handle errors,
I think we should follow through and add all addresses in case of error.
Still, also collect a overall "success" of the function and return it.
(cherry picked from commit cedaa191d4)
In the past, nm_platform_ip_address_sync() only had the @known_addresses
argument. We would figure out which addresses to delete and which to preserve,
based on what addresses were known. That means, @known_addresses must have contained
all the addresses we wanted to preserve, even the external ones. That approach
was inherently racy.
Instead, nowadays we have the addresses we want to configure (@known_addresses)
and the addresses we want to delete (@prune_addresses). This started to change in
commit dadfc3abd5 ('platform: allow injecting the list of addresses to prune'),
but only commit 58287cbcc0 ('core: rework IP configuration in NetworkManager using
layer 3 configuration') actually changed to pass separate @prune_addresses argument.
However, the order of IP addresses matters and there is no sensible kernel API
to configure the order (short of adding them in the right order), we still need
to look at all the addresses, check their order, and possibly delete some.
That is, we need to handle addresses we want to delete (@prune_addresses)
but still look at all addresses in platform (@plat_addresses) to check
their order.
Now, first handle @prune_addresses. That's simple. These are just the
addresses we want to delete. Second, get the list of all addresses in
platform (@plat_addresses) and check the order.
Note that if there is an external address that interferes with our
desired order, we will leave it untouched. Thus, such external addresses
might prevent us from getting the order as desired. But that's just
how it is. Don't add addresses outside of NetworkManager to avoid that.
Fixes: 58287cbcc0 ('core: rework IP configuration in NetworkManager using layer 3 configuration')
(cherry picked from commit 80f8e23992)
No need to try further. The verdict is clear.
From the log:
<debug> [1649424031.1507] connectivity: (wlan0,IPv4,427) can't resolve a name via systemd-resolved: GDBus.Error:org.freedesktop.resolve1.NoNameServers: No appropriate name servers or networks for name found
<debug> [1649424031.1507] connectivity: (wlan0,IPv4,427) start request to 'http://fedoraproject.org/static/hotspot.txt' (try resolving 'fedoraproject.org' using system resolver)
(cherry picked from commit 5b779c1ab7)
This can lead to a crash. The code might continue to call
system_resolver_resolve(), then it has no more cancellable.
That means, if the task gets cancelled, then the callback
will still return and result in a crash.
There is no need to cancel or clear the cancellable during
normal operation. It will be cleaned up at the end.
This leads to an assertion error (or possibly crash):
...
#6 0x00005584ff461e67 in system_resolver_resolve_cb (source_object=<optimized out>, res=0x5585016b9190, user_data=user_data@entry=0x558501667800) at src/core/nm-connectivity.c:798
#7 0x00007f348a02419a in g_task_return_now (task=0x5585016b9190) at ../gio/gtask.c:1219
#8 0x00007f348a0241dd in complete_in_idle_cb (task=task@entry=0x5585016b9190) at ../gio/gtask.c:1233
#9 0x00007f3489e263eb in g_idle_dispatch (source=0x7f3464001070, callback=0x7f348a0241d0 <complete_in_idle_cb>, user_data=0x5585016b9190) at ../glib/gmain.c:5897
...
Fixes: 57d226d3f0 ('connectivity: resolve hostname ourselves to avoid blocking libcurl')
(cherry picked from commit 62b1f9766a)
We passed on the CFLAGS, but they also contain
"-DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_40 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_40"
which causes compiler warnings:
GISCAN src/libnm-client-impl/NM-1.0.gir
/data/src/NetworkManager/tmp-introspect_17ddrdb/NM-1.0.c: In function ‘dump_object_type’:
/data/src/NetworkManager/tmp-introspect_17ddrdb/NM-1.0.c:251:13: warning: Not available before 2.70
251 | if (G_TYPE_IS_FINAL (type))
| ^~~~~~~~~~~~~~~~~
/data/src/NetworkManager/tmp-introspect_17ddrdb/NM-1.0.c: In function ‘dump_fundamental_type’:
/data/src/NetworkManager/tmp-introspect_17ddrdb/NM-1.0.c:369:13: warning: Not available before 2.70
369 | if (G_TYPE_IS_FINAL (type))
| ^~~~~~~~~~~~~~~~~
Filter them out.
See-also: https://gitlab.gnome.org/GNOME/gobject-introspection/-/merge_requests/331
(cherry picked from commit 1234e5583a)
