NetworkManager/NetworkManager!1168

Next we are going to assert that the prefix length is valid.
The test needs to have valid prefix lengths too. Adjust.

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).

Some were duplicated. Drop those.

Some function were in an order where they required forward declarations.
Reorder.

We have this util function, presumably because it's good to have it.
Use it.

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.

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.

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.

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.

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).

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.

There is only one caller of _addr_array_clean_expired(), and it always
provides the "idx" pointer.

It seems easier to read, than passing a boolean parameter.

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).

The code is disabled at compile time. It's only useful for printf
debugging to modify the source to get more logging.

NetworkManager/NetworkManager!1168

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.

Fixes: 2f68a500 ('platform: fix the order of addition of primary and secondary IPv4 addresses')

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.

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 dadfc3ab ('platform: allow injecting the list of addresses to prune'),
but only commit 58287cbc ('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: 58287cbc ('core: rework IP configuration in NetworkManager using layer 3 configuration')

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)

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: 57d226d3 ('connectivity: resolve hostname ourselves to avoid blocking libcurl')

Currently wpa_supplicant doesn't support FT in AP mode. FT-PSK and
FT-EAP are simply not negotiated with the STA. FT-SAE gets negotiated
but then the key derivation is not supported, leading to a
authentication failure.

Even if support for FT in AP mode is introduced in wpa_supplicant in
the future, it will require additional parameters as the nas
identifier and the mobility domain, which are currently not provided
by NM.

Disable all FT key-mgmts in AP mode since they are useless and cause
issues (FT-SAE).

See-also: https://mail.gnome.org/archives/networkmanager-list/2022-March/msg00016.html
See-also: http://lists.infradead.org/pipermail/hostap/2022-April/040352.html

!1184

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

Let's set the "src" (RTA_PREFSRC) of DHCP routes.
This helps with source address selection.

This can matter if the interface also has static addresses
configured.

Systemd-networkd also does this ([1], [2]).

[1] https://github.com/systemd/systemd/commit/ac2dce5f36bb8b1a877ff765e6a4dfde6bfb2d49
[2] https://github.com/systemd/systemd/blob/5b89bff55f45235f72d30d90fd489fe2247ad00d/src/network/networkd-dhcp4.c#L395

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1995372

NetworkManager/NetworkManager!1173

With "rc1" mode, we install more than one tarballs (the one for 1.37.90
and 1.39.0). If we reach this point, we already pushed the git tags.
There is no way back.

Ignore errors at first and try to release all tarballs. Only signal the error
at the end.