Commit graph

22 commits

Author SHA1 Message Date
Thomas Haller
6b0f67b736
connectivity: skip unexpected address families in system_resolver_resolve_cb()
This actually cannot happen, because GInetAddress is either
IPv4 or IPv6. Still.
2022-06-23 17:11:28 +02:00
Thomas Haller
c1682b2556
connectivity: handle "NoNameServers" resolved error and don't callback to system resolver
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)
2022-04-08 15:50:35 +02:00
Thomas Haller
64e65d35b3
connectivity: don't clear "concheck.resolve_cancellable" early in systemd_resolved_resolve_cb()
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)
2022-04-08 15:50:34 +02:00
Thomas Haller
a892746578
connectivity: only enable verbose libcurl debug logging with "NM_LOG_CONCHECK" environment
For regular operation -- even for `level=TRACE` -- it's just too verbose.
Only enable it if the environment "NM_LOG_CONCHECK=1" is set.

An environment variable is a bit unwieldy to use, but this
is really just for a heavy libcurl debugging session.
2022-04-04 16:09:36 +02:00
Thomas Haller
1dc16931be
connectivity: refactor easy_debug_cb()
It seems nicer to me to choose a message in the switch
and only print at one place.
2022-04-04 16:09:36 +02:00
Thomas Haller
57d226d3f0
connectivity: resolve hostname ourselves to avoid blocking libcurl
Usually we anyway require systemd-resolved to resolve the hostname for
connectivity checking. Only systemd-resolved provides a per-interface
API. Without it, connectivity check (together with bumping the route
metric) has problems.

Anyway. If we had no systemd-resolved or it failed, we would just call
libcurl. That would then try to resolve the name, using whatever resolver
libcurl has enabled. Often that is the threaded resolver, which calls
libc's blocking getaddrinfo() API on a thread.

libcurl has a bug ([1]) that can cause the process to block, waiting to join
the resolver thread:

  #0  0x00007ffff781fb27 in __pthread_timedjoin_ex () at /lib64/libpthread.so.0
  #1  0x00007ffff7c0ac9a in Curl_thread_join () at /lib64/libcurl.so.4
  #2  0x00007ffff7c0d693 in thread_wait_resolv () at /lib64/libcurl.so.4
  #3  0x00007ffff7bf9284 in multi_done () at /lib64/libcurl.so.4
  #4  0x00007ffff7bfb588 in curl_multi_remove_handle () at /lib64/libcurl.so.4
  #5  0x000055555574adc3 in cb_data_complete

That's not acceptable. Resolve the name ourselves using glib's implementation
(which also does getaddrinfo() in a thread). If we fail, we no longer call to
libcurl.

[1] https://github.com/curl/curl/issues/8515

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/312
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/404
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/934
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/970

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1176
2022-04-04 16:02:00 +02:00
Thomas Haller
14b9a9bd9d
connectivity: fix constructing hosts list for CURLOPT_RESOLVE
Curl's CURLOPT_RESOLVE expects one list entry per host. That
documentation ([1]) also makes that clear that the form is
"[+]HOST:PORT:ADDRESS[,ADDRESS]".

The way we constructed the list, only the last entry was honored:

  <trace> [1647551393.5362] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:18.159.254.57' to curl resolve list
  <trace> [1647551393.5363] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:152.19.134.142' to curl resolve list
  <trace> [1647551393.5363] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:18.192.40.85' to curl resolve list
  ...
  <trace> [1647551393.5366] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:85.236.55.6' to curl resolve list
  <trace> [1647551393.5366] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:38.145.60.20' to curl resolve list
  ...
  <trace> [1647551393.5415] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:18.159.254.57 to DNS cache\012
  <trace> [1647551393.5416] connectivity: (eth0,IPv4,25) libcurl: == Info: RESOLVE fedoraproject.org:80 is - old addresses discarded!\012
  <trace> [1647551393.5416] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:152.19.134.142 to DNS cache\012
  <trace> [1647551393.5417] connectivity: (eth0,IPv4,25) libcurl: == Info: RESOLVE fedoraproject.org:80 is - old addresses discarded!\012
  ...
  <trace> [1647551393.5422] connectivity: (eth0,IPv4,25) libcurl: == Info: RESOLVE fedoraproject.org:80 is - old addresses discarded!\012
  <trace> [1647551393.5423] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:38.145.60.20 to DNS cache\012
  <trace> [1647551393.5424] connectivity: (eth0,IPv4,25) libcurl: == Info: Hostname fedoraproject.org was found in DNS cache\012
  <trace> [1647551393.5424] connectivity: (eth0,IPv4,25) libcurl: == Info:   Trying 38.145.60.20:80...\012

There are two possible fixes. Either join all addresses in one
entry, or use the '+' modifier. Do the former.

Now we get:

  <trace> [1647551967.0378] connectivity: (eth0,IPv4,25) set curl resolve list to 'fedoraproject.org:80:38.145.60.21,152.19.134.142,152...
  ...
  <trace> [1647551967.0559] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:38.145.60.21,152.19.134.142,152.1...
  <trace> [1647551967.0560] connectivity: (eth0,IPv4,25) libcurl: == Info: Hostname fedoraproject.org was found in DNS cache\012
  <trace> [1647551967.0561] connectivity: (eth0,IPv4,25) libcurl: == Info:   Trying 38.145.60.21:80...\012

[1] https://curl.se/libcurl/c/CURLOPT_RESOLVE.html

Reported-by: Bastien Nocera <hadess@hadess.net>

Fixes: 2cec94bacc ('connectivity: use systemd-resolved for resolving the check endpoint')

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/648#note_1301596

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1153
2022-03-18 15:02:42 +01:00
Bastien Nocera
4e6b18e595 connectivity: Add debug to curl queries
Add debug output when we use curl to make queries.

References: https://curl.se/libcurl/c/debug.html
2022-03-17 20:41:31 +00:00
Thomas Haller
ca9c67565a
connectivity: use GSource instead of source id for tracking timeout/idle source
I think we should move away from using the source-ids.

Having a "GSource*" pointer makes it clearer what this is, compared to a
guint source ID. Also, g_source_remove() always needs to first do a hash
lookup (with locking) to resolve the source ID to the GSource. This is
unnecessary.
2022-03-17 10:27:33 +01:00
Thomas Haller
63d68e7f42
connectivity: minor cleanup resolve_cb() to use nm_utils_addr_family_to_size()
And change code so that clang-format produces nicer alignment.
2022-03-17 10:10:41 +01:00
Thomas Haller
593d08db81
connectivity: skip unexpected addr family from resolved response
In practice, systemd-resolved will give us always AF_INET/AF_INET6. Still,
check for that.
2022-03-17 10:10:22 +01:00
Thomas Haller
7a1734926a
connectivity,cloud-setup: restrict curl protocols to HTTP and HTTPS
See-also: https://fedoraproject.org/wiki/Changes/CurlMinimal_as_Default#Benefit_to_Fedora
See-also: 55b90ee00b

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1121
2022-02-24 09:37:58 +01:00
Thomas Haller
615221a99c format: reformat source tree with clang-format 13.0
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.

So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.

As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).

The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as

  Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)

[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
2021-11-29 09:31:09 +00:00
Thomas Haller
58287cbcc0 core: rework IP configuration in NetworkManager using layer 3 configuration
Completely rework IP configuration in the daemon. Use NML3Cfg as layer 3
manager for the IP configuration of an interface. Use NML3ConfigData as
pieces of configuration that the various components collect and
configure. NMDevice is managing most of the IP configuration at a higher
level, that is, it starts DHCP and other IP methods. Rework the state
handling there.

This is a huge rework of how NetworkManager daemon handles IP
configuration. Some fallout is to be expected.

It appears the patch deletes many lines of code. That is not accurate, because
you also have to count the files `src/core/nm-l3*`, which were unused previously.

Co-authored-by: Beniamino Galvani <bgalvani@redhat.com>
2021-11-18 16:21:29 +01:00
Thomas Haller
4e109bacab
clang-format: use "IndentPPDirectives:None" instead of "BeforeHash"
Subjectively, I think this looks better.
2021-07-09 08:49:06 +02:00
Thomas Haller
61d2af3518
all: use nm_{idle,timeout}_add_source() instead of g_source_attach() 2021-06-28 13:31:35 +02:00
Thomas Haller
70e5d8e5bd
all: don't explicitly include <glib-unix.h>
We get it now always by "nm-macros-internal.h".
2021-06-28 13:31:34 +02:00
Beniamino Galvani
4f9ff47e2d dns: add function to retrieve the systemd-resolved plugin instance
Now that we have nm_dns_systemd_resolved_resolve_address(), we also
need a way to obtain a reference to the plugin.

(cherry picked from commit 7285bc56a9)
2021-06-11 21:59:11 +02:00
Thomas Haller
21321ac736
clang-format: reformat code with clang 12
The format depends on the version of the tool. Now that Fedora 34 is
released, update to clang 12 (clang-tools-extra-12.0.0-0.3.rc1.fc34.x86_64).
2021-05-04 13:56:26 +02:00
Thomas Haller
fdf9614ba7
build: move "libnm-core/" to "src/" and split it
"libnm-core/" is rather complicated. It provides a static library that
is linked into libnm.so and NetworkManager. It also contains public
headers (like "nm-setting.h") which are part of public libnm API.

Then we have helper libraries ("libnm-core/nm-libnm-core-*/") which
only rely on public API of libnm-core, but are themself static
libraries that can be used by anybody who uses libnm-core. And
"libnm-core/nm-libnm-core-intern" is used by libnm-core itself.

Move "libnm-core/" to "src/". But also split it in different
directories so that they have a clearer purpose.

The goal is to have a flat directory hierarchy. The "src/libnm-core*/"
directories correspond to the different modules (static libraries and set
of headers that we have). We have different kinds of such modules because
of how we combine various code together. The directory layout now reflects
this.
2021-02-18 19:46:51 +01:00
Thomas Haller
dc2afc9b77
all: add "src/core/nm-default-daemon.h" as replacement for "nm-default.h" 2021-02-09 12:38:18 +01:00
Thomas Haller
ac1a9e03e4
all: move "src/" directory to "src/core/"
Currently "src/" mostly contains the source code of the daemon.
I say mostly, because that is not true, there are also the device,
settings, wwan, ppp plugins, the initrd generator, the pppd and dhcp
helper, and probably more.

Also we have source code under libnm-core/, libnm/, clients/, and
shared/ directories. That is all confusing.

We should have one "src" directory, that contains subdirectories. Those
subdirectories should contain individual parts (libraries or
applications), that possibly have dependencies on other subdirectories.
There should be a flat hierarchy of directories under src/, which
contains individual modules.

As the name "src/" is already taken, that prevents any sensible
restructuring of the code.

As a first step, move "src/" to "src/core/". This gives space to
reorganize the code better by moving individual components into "src/".

For inspiration, look at systemd's "src/" directory.

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/743
2021-02-04 09:45:55 +01:00
Renamed from src/nm-connectivity.c (Browse further)