Kernel enforces that all nexthops must be reachable through a route.
L3Cfg is generating dependent onlink routes to solve this problem but
the IPv4 ECMP commit is happening before that.
To solve this we introduce two boolean fields "is_new" and "is_ready" to
know in which state is the L3Cfg affected. Initially, "is_new" is TRUE
and "is_ready" is FALSE. Here we schedule a commit on idle and we set
"is_new" to FALSE. When revisiting, we set "is_ready" to TRUE and then
we set the ECMP IPv4 routes.
When a reapply kicks in we reset the L3Cfg state by setting "is_new" to
TRUE.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1520
(cherry picked from commit 7a844ecba9)
When we register/unregister a commit-type or when we add/remove a
config-data to NML3Cfg, that act only does the registration/addition.
Only on the next commit, are the changes actually done. The purpose
of that is to add/register multiple configurations and commit them later
when ready.
However, it would be wrong to not do the commit a short time after. The
configuration state is dirty with need to be committed, and that should
happen soon.
Worse, when a interface disappears, NMDevice will clear the ifindex and
the NML3Cfg instance, thereby unregistering all config data and commit
type. If we previously commited something, we need to do another follow-up
commit to cleanup that state.
That is for example important with ECMP routes, which are registered in
NMNetns. When NML3Cfg goes down, it always must unregister to properly
cleanup. Failure to do so, causes an assertion failure and crash. This
change fixes that.
Fix that by automatically schedule and idle commit on
register/unregister/add/remove of commit-type/config-data.
It should *always* be permissible to call a AUTO commit from
an idle handler, because various parties cannot use NML3Cfg
independently, and they cannot know when somebody else does a
commit.
Note that NML3Cfg remembers if it presiouvly did a commit
("commit_type_update_sticky"), so even if the last commit-type gets
unregistered, the next commit will still do a sticky update (one more
time).
The only remaining question is what happens during quitting. When
quitting, NetworkManager we may want to leave some interfaces up and
configured. If we were to properly cleanup the NML3Cfg we might need a
mechanism to handle that. However, currently we just leak everything
during quit, so that is not a concern now. It is something that needs
to be addressed in the future.
https://bugzilla.redhat.com/show_bug.cgi?id=2158394https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1505
NML3Cfg and NMNetns are already strongly related and cooperate.
An NML3Cfg instance is created via NMNetns, which is necessary
because NMNetns ensures that there is only one NML3Cfg instance per
ifindex and it won't ever make sense to have multiple NML3Cfg instances
per namespace.
Note that NMNetns tracks additional information for each NML3Cfg.
Previously, in a pointless attempt to separate code, it did so
by putting that information in another struct (L3CfgData).
But as the classes are strongly related, there really is no
reason why we cannot just attach this information to NML3Cfg
directly. Sure, we want that code has low coupling, high cohesion
but that doesn't mean we gain anything by putting data that is
strongly related to the NML3Cfg to another struct L3CfgData.
The advantage is we save some redundant data and an additional
L3CfgData. But the bigger reason is that with this change, it
will be possible to access the NMNetns specific data directly from
an NML3Cfg instance, without another dictionary lookup. Currently
such a lookup is never used, but it will be.
Basically, NML3Cfg and NMNetns shares some state. It is now in the
"internal_netns" field of the NML3Cfg instead of L3CfgData.
Support managing the loopback interface through NM as the users want to
set the proper mtu for loopback interface when forwarding the packets.
Additionally, the IP addresses, DNS, route and routing rules are also
allowed to configure for the loopback connection profiles.
https://bugzilla.redhat.com/show_bug.cgi?id=2060905
All the callers pass either AF_INET or AF_INET6, drop support for
AF_UNSPEC; this simplifies the function for the next commit that adds
a @conflicts argument.
We already have src/linux-headers, where we have complete copies of linux
user space headers. Of course that exists, because we want to use certain
features and don't depend on the installed kernel headers. Which works
well, because kernel user space API is stable, and we anyway want to
support compiling against a newer kernel and run against an older (e.g.
in a container). So having our copy of newer kernel headers is merely
as if we compiled against as newer kernel.
Add "src/nm-compat-headers" which has a similar purpose, but a different
approach. Instead of replacing the included header entirely, include
the system header and patch it with #define.
Use this for "linux/if_addr.h". Of course, the approach here is that we
no longer include <linux/if_addr.h> directly, but instead include
"nm-compat-headers/linux/if_addr.h".
These variants provide additional nm_assert() checks, and are thus
preferable.
Note that we cannot just blindly replace &g_array_index() with
&nm_g_array_index(), because the latter would not allow getting a
pointer at index [arr->len]. That might be a valid (though uncommon)
usecase. The correct replacement of &g_array_index() is thus
nm_g_array_index_p().
I checked the code manually and replaced uses of nm_g_array_index_p()
with &nm_g_array_index(), if that was a safe thing to do. The latter
seems preferable, because it is familar to &g_array_index().
Previously, nm_platform_ip_address_sync() would always add the "IFA_F_NOPREFIXROUTE"
flag. Add a way to let the caller control that.
Add a flags argument, with a new flag "with-noprefixroute". By default
(with flags "none"), nm_platform_ip_address_sync() would no longer
add "IFA_F_NOPREFIXROUTE" flag, but the caller can now opt-in to that.
The purpose is that on "lo" interface we will want to let kernel
handle the prefix route. So have a per-ifindex opt-in for controlling
this.
During nm_platform_ip_address_flush() we use "none" flags, because the
function anyway doesn't add any addresses, so it wouldn't matter.
There is no change in behavior.
Co-authored-by: Thomas Haller <thaller@redhat.com>
1) The "enabled-on-global-iface" flag was odd. Instead, have only
and "enabled" flag and skip (by default) endpoints on interface
that have no default route. With the new flag "also-without-default-route",
this can be overruled. So previous "enabled-on-global-default" now is
the same as "enabled", and "enabled" from before behaves now like
"enabled,also-without-default-route".
2) What was also odd, as that the fallback default value for the flags
depends on "/proc/sys/net/mptcp/enabled". There was not one fixed
fallback default, instead the used fallback value was either
"enabled-on-global-iface,subflow" or "disabled".
Usually that is not a problem (e.g. the default value for
"ipv6.ip6-privacy" also depends on use_tempaddr sysctl). In this case
it is a problem, because the mptcp-flags (for better or worse) encode
different things at the same time.
Consider that the mptcp-flags can also have their default configured in
"NetworkManager.conf", a user who wants to switch the address flags
could previously do:
[connection.mptcp]
connection.mptcp-flags=0x32 # enabled-on-global-iface,signal,subflow
but then the global toggle "/proc/sys/net/mptcp/enabled" was no longer
honored. That means, MPTCP handling was always on, even if the sysctl was
disabled. Now, "enabled" means that it's only enabled if the sysctl
is enabled too. Now the user could write to "NetworkManager.conf"
[connection.mptcp]
connection.mptcp-flags=0x32 # enabled,signal,subflow
and MPTCP handling would still be disabled unless the sysctl
is enabled.
There is now also a new flag "also-without-sysctl", so if you want
to really enable MPTCP handling regardless of the sysctl, you can.
The point of that might be, that we still can configure endpoints,
even if kernel won't do anything with them. Then you could just flip
the sysctl, and it would start working (as NetworkManager configured
the endpoints already).
Fixes: eb083eece5 ('all: add NMMptcpFlags and connection.mptcp-flags property')
- name things related to `in_addr_t`, `struct in6_addr`, `NMIPAddr` as
`nm_ip4_addr_*()`, `nm_ip6_addr_*()`, `nm_ip_addr_*()`, respectively.
- we have a wrapper `nm_inet_ntop()` for `inet_ntop()`. This name
of our wrapper is chosen to be familiar with the libc underlying
function. With this, also name functions that are about string
representations of addresses `nm_inet_*()`, `nm_inet4_*()`,
`nm_inet6_*()`. For example, `nm_inet_parse_str()`,
`nm_inet_is_normalized()`.
<<<<
R() {
git grep -l "$1" | xargs sed -i "s/\<$1\>/$2/g"
}
R NM_CMP_DIRECT_IN4ADDR_SAME_PREFIX NM_CMP_DIRECT_IP4_ADDR_SAME_PREFIX
R NM_CMP_DIRECT_IN6ADDR_SAME_PREFIX NM_CMP_DIRECT_IP6_ADDR_SAME_PREFIX
R NM_UTILS_INET_ADDRSTRLEN NM_INET_ADDRSTRLEN
R _nm_utils_inet4_ntop nm_inet4_ntop
R _nm_utils_inet6_ntop nm_inet6_ntop
R _nm_utils_ip4_get_default_prefix nm_ip4_addr_get_default_prefix
R _nm_utils_ip4_get_default_prefix0 nm_ip4_addr_get_default_prefix0
R _nm_utils_ip4_netmask_to_prefix nm_ip4_addr_netmask_to_prefix
R _nm_utils_ip4_prefix_to_netmask nm_ip4_addr_netmask_from_prefix
R nm_utils_inet4_ntop_dup nm_inet4_ntop_dup
R nm_utils_inet6_ntop_dup nm_inet6_ntop_dup
R nm_utils_inet_ntop nm_inet_ntop
R nm_utils_inet_ntop_dup nm_inet_ntop_dup
R nm_utils_ip4_address_clear_host_address nm_ip4_addr_clear_host_address
R nm_utils_ip4_address_is_link_local nm_ip4_addr_is_link_local
R nm_utils_ip4_address_is_loopback nm_ip4_addr_is_loopback
R nm_utils_ip4_address_is_zeronet nm_ip4_addr_is_zeronet
R nm_utils_ip4_address_same_prefix nm_ip4_addr_same_prefix
R nm_utils_ip4_address_same_prefix_cmp nm_ip4_addr_same_prefix_cmp
R nm_utils_ip6_address_clear_host_address nm_ip6_addr_clear_host_address
R nm_utils_ip6_address_same_prefix nm_ip6_addr_same_prefix
R nm_utils_ip6_address_same_prefix_cmp nm_ip6_addr_same_prefix_cmp
R nm_utils_ip6_is_ula nm_ip6_addr_is_ula
R nm_utils_ip_address_same_prefix nm_ip_addr_same_prefix
R nm_utils_ip_address_same_prefix_cmp nm_ip_addr_same_prefix_cmp
R nm_utils_ip_is_site_local nm_ip_addr_is_site_local
R nm_utils_ipaddr_is_normalized nm_inet_is_normalized
R nm_utils_ipaddr_is_valid nm_inet_is_valid
R nm_utils_ipx_address_clear_host_address nm_ip_addr_clear_host_address
R nm_utils_parse_inaddr nm_inet_parse_str
R nm_utils_parse_inaddr_bin nm_inet_parse_bin
R nm_utils_parse_inaddr_bin_full nm_inet_parse_bin_full
R nm_utils_parse_inaddr_prefix nm_inet_parse_with_prefix_str
R nm_utils_parse_inaddr_prefix_bin nm_inet_parse_with_prefix_bin
R test_nm_utils_ip6_address_same_prefix test_nm_ip_addr_same_prefix
./contrib/scripts/nm-code-format.sh -F
- drop unused "keep_deleted" parameter. It just doesn't make sense.
Even less sense than for rules/routes, where this was taken from.
- fix nmp_global_tracker_sync_mptcp_addrs() to delete addresses
with conflicting flags. We did not correctly delete existing
addresses, that were to be reconfigured with different flags.
Fixes: 5374c403d2 ('platfrom: handle MPTCP addresses with NMPGlobalTracker')
NetworkManager primarily manages interfaces in an independent fashion.
That means, whenever possible, we want to have a interface specific
view. In many cases, the underlying kernel API also supports that view.
For example, when configuring IP addresses or unicast routes, we do so
per interfaces and don't need a holistic view.
However, that is not always sufficient. For routing rules and certain
route types (blackhole, unreachable, etc), we need a system wide view
of all the objects in the network namespace.
Originally, NMPRulesManager was added to track routing rules. Then, it
was extended to also track certain route types, and the API was renamed to
NMPRouteManager.
This will also be used to track MPTCP addresses.
So rename again, to give it a general name that is suitable for what it
does. Still, the name is not great (suggestion welcome), but it should
cover the purpose of the API well enough. And it's the best I came
up with.
Rename.
In the past, nmp_lookup_init_object() could both lookup all object for a
certain ifindex, and lookup all objects of a type. That fallback path
already leads to an assertion failure fora while now, so nobody should
be using this function to lookup all objects of a certain type (for
what, we have nmp_lookup_init_obj_type()).
Now, remove the fallback path, and rename the function to what it really
does.
IPv6 temporary addresses are configured by kernel, with the
"ipv6.ip6-privacy" setting ("use_tempaddr" sysctl) and the
IFA_F_MANAGETEMPADDR flag.
As such, the idea was that during reapply we would not remove them.
However, that is wrong.
The only case when we want to keep those addresses, is if during reapply
we are going to configure the same primary address (with mngtmpaddr
flag) again. Otherwise, theses addresses must always go away.
This is quite serious. This not only affects Reapply. Also during disconnect
we clear IP configuration via l3cfg.
Have an ethernet profile active with "ipv6.ip6-privacy". Unplug
the cable, the device disconnects but the temporary IPv6 address is not
cleared. As such, nm_device_generate_connection() will now generate
an external profile (with "ipv6.method=disabled" and no manual IP addresses).
The result is, that the device cannot properly autoconnect again,
once you replug the cable.
This is serious for disconnect. But I could not actually reproduce the
problem using reapply. That is, because during reapply we usually
toggle ipv6_disable sysctl, which drops all IPv6 addresses. I still
went through the effort of trying to preserve addresses that we still
want to have, because I am not sure whether there are cases where we
don't toggle ipv6_disable. Also, doing ipv6_disable during reapply is
bad anyway, and we might want to avoid that in the future.
Fixes: 58287cbcc0 ('core: rework IP configuration in NetworkManager using layer 3 configuration')
(cherry picked from commit 518f6124c6)
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 often create the source with default priority, no destroy function and
attach it to the default context (g_main_context_default()). For that
case, we have wrapper functions like nm_g_timeout_add_source()
and nm_g_idle_add_source(). Use those.
There should be no change in behavior.
We can get a platform signal for any number of reasons. In particular,
we can get a signal that the object is present in platform, while the object
is tracked as zombie.
"Zombies" are objects that were actively configured by NetworkManager, but
now no longer and thus will need to be removed. We remember them as objects
that we need to delete.
The assertion was wrong. We don't need to handle the case "in_platform"
and linked in "os_zombie_lst" specially. If we get a signal that the
object exists while being a zombie, that is fine and not something to
handle specially.
Backtrace:
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007f6a208f1db5 in __GI_abort () at abort.c:79
#2 0x00007f6a212ed123 in g_assertion_message (domain=<optimized out>, file=<optimized out>, line=<optimized out>,
func=0x560e23ada2c0 <__func__.39909> "_obj_states_externally_removed_track", message=<optimized out>) at gtestutils.c:2533
#3 0x00007f6a2134620e in g_assertion_message_expr (domain=domain@entry=0x560e23b781a0 "nm", file=file@entry=0x560e23acec60 "src/core/nm-l3cfg.c", line=line@entry=920,
func=func@entry=0x560e23ada2c0 <__func__.39909> "_obj_states_externally_removed_track", expr=expr@entry=0x560e23ad1980 "c_list_is_empty(&obj_state->os_zombie_lst)") at gtestutils.c:2556
#4 0x0000560e23853f38 in _obj_states_externally_removed_track (self=self@entry=0x560e25f168e0, obj=<optimized out>, obj@entry=0x560e25e466a0, in_platform=in_platform@entry=1)
at src/core/nm-l3cfg.c:920
#5 0x0000560e2385b8ea in _nm_l3cfg_notify_platform_change (self=0x560e25f168e0, change_type=change_type@entry=NM_PLATFORM_SIGNAL_CHANGED, obj=0x560e25e466a0) at src/core/nm-l3cfg.c:1364
#6 0x0000560e23861251 in _platform_signal_cb (platform=<optimized out>, obj_type_i=<optimized out>, ifindex=<optimized out>, platform_object=0x560e25e466b8, change_type_i=2,
p_self=<optimized out>) at ./src/libnm-platform/nmp-object.h:443
#7 0x00007f6a1c4a914e in ffi_call_unix64 () at ../src/x86/unix64.S:76
#8 0x00007f6a1c4a8aff in ffi_call (cif=cif@entry=0x7fffac40e570, fn=fn@entry=0x560e23861100 <_platform_signal_cb>, rvalue=<optimized out>, avalue=avalue@entry=0x7fffac40e480)
at ../src/x86/ffi64.c:525
#9 0x00007f6a217fee85 in g_cclosure_marshal_generic (closure=<optimized out>, return_gvalue=<optimized out>, n_param_values=<optimized out>, param_values=<optimized out>,
invocation_hint=<optimized out>, marshal_data=<optimized out>) at gclosure.c:1490
#10 0x00007f6a217fe3bd in g_closure_invoke (closure=0x560e25df53c0, return_value=0x0, n_param_values=5, param_values=0x7fffac40e7a0, invocation_hint=0x7fffac40e720) at gclosure.c:804
#11 0x00007f6a21811945 in signal_emit_unlocked_R (node=node@entry=0x7f6a00008870, detail=detail@entry=0, instance=instance@entry=0x560e25ddd080, emission_return=emission_return@entry=0x0,
instance_and_params=instance_and_params@entry=0x7fffac40e7a0) at gsignal.c:3636
#12 0x00007f6a2181aa56 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7fffac40e9c0) at gsignal.c:3392
#13 0x00007f6a2181b093 in g_signal_emit (instance=instance@entry=0x560e25ddd080, signal_id=<optimized out>, detail=detail@entry=0) at gsignal.c:3448
#14 0x0000560e2392deea in nm_platform_cache_update_emit_signal (self=0x560e25ddd080, cache_op=NMP_CACHE_OPS_UPDATED, obj_old=<optimized out>, obj_new=<optimized out>)
at src/libnm-platform/nm-platform.c:8824
#15 0x0000560e238fd520 in event_handler_recvmsgs () at src/libnm-platform/nm-linux-platform.c:7183
#16 0x0000560e238fdcbf in event_handler_read_netlink () at src/libnm-platform/nm-linux-platform.c:9403
#17 0x0000560e238ffab3 in delayed_action_handle_one () at src/libnm-platform/nm-linux-platform.c:6238
#18 0x0000560e238ffcae in delayed_action_handle_all () at src/libnm-platform/nm-linux-platform.c:6256
#19 0x0000560e23901acc in do_delete_object () at src/libnm-platform/nm-linux-platform.c:7392
#20 0x0000560e2390227c in ip4_address_delete () at src/libnm-platform/nm-linux-platform.c:8782
#21 0x0000560e23922709 in nm_platform_ip4_address_delete (self=self@entry=0x560e25ddd080, ifindex=ifindex@entry=150, address=16843009, plen=<optimized out>, peer_address=16843009)
at src/libnm-platform/nm-platform.c:3574
#22 0x0000560e239275ab in nm_platform_ip_address_sync (self=0x560e25ddd080, addr_family=addr_family@entry=2, ifindex=150, known_addresses=<optimized out>, known_addresses@entry=0x0,
addresses_prune=0x560e25e81aa0) at src/libnm-platform/nm-platform.c:3984
#23 0x0000560e23855e17 in _l3_commit_one (self=0x560e25f168e0, addr_family=2, commit_type=<optimized out>, l3cd_old=<optimized out>, changed_combined_l3cd=<optimized out>)
at src/core/nm-l3cfg.c:4256
#24 0x0000560e2385fc5c in _l3_commit (self=0x560e25f168e0, commit_type=NM_L3_CFG_COMMIT_TYPE_REAPPLY, is_idle=<optimized out>) at src/core/nm-l3cfg.c:4353
#25 0x0000560e239c6a6d in nm_device_cleanup (self=0x560e25e985e0, reason=<optimized out>, cleanup_type=CLEANUP_TYPE_DECONFIGURE) at src/core/devices/nm-device.c:15082
#26 0x0000560e239c7522 in _set_state_full (self=0x560e25e985e0, state=<optimized out>, reason=<optimized out>, quitting=0) at src/core/devices/nm-device.c:15467
#27 0x0000560e239cd482 in queued_state_set (user_data=user_data@entry=0x560e25e985e0) at src/core/devices/nm-device.c:15706
#28 0x00007f6a2131b27b in g_idle_dispatch (source=0x560e25ebab60, callback=0x560e239cd3d0 <queued_state_set>, user_data=0x560e25e985e0) at gmain.c:5579
#29 0x00007f6a2131e95d in g_main_dispatch (context=0x560e25d97bc0) at gmain.c:3193
#30 g_main_context_dispatch (context=context@entry=0x560e25d97bc0) at gmain.c:3873
#31 0x00007f6a2131ed18 in g_main_context_iterate (context=0x560e25d97bc0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3946
#32 0x00007f6a2131f042 in g_main_loop_run (loop=0x560e25d730f0) at gmain.c:4142
#33 0x0000560e237c06ec in main (argc=<optimized out>, argv=<optimized out>) at src/core/main.c:509
Fixes: 929eae245d ('l3cfg: implement NM_L3CFG_CONFIG_FLAGS_ASSUME_CONFIG_ONCE and rework object state')
(cherry picked from commit 849a4eee5c)
l3cfg has a "temp_not_available" list of objects that couldn't be
added to platform, but can be added once some preconditions become
true (for example, a IPv6 route with a "src" attribute requires a
non-tentative src address to be present).
Retry to commit those objects once all addresses have completed
ACD/DAD.
(cherry picked from commit 9a090fdf7b)
Certain route types (blackhole, unreachable, prohibit) are not tied to
an interface. They are thus global and we need to track them system wide
(or better: per network namespace). That is done by NMPRouteManager.
For the routing rules, it's NMDevice itself to track/untrack the rules.
That is done for historical reasons, at the time, NML3Cfg did not exit.
Now with NML3Cfg, it seems that also NML3Cfg should be the part that
handles nodev routes. One reason is that we want to move IP
functionality out of NMDevice. So callers (NMDevice) would just add
blackhole routes to the NML3ConfigData and let NML3Cfg handle them.
Still, to handle these routes is rather different from regular routes.
Normally, NML3Cfg tracks an object state (ObjStateData) for each address/route,
and it hooks into platform signals to update the os_plobj field. Those signals
are dispatched by NMNetns and are only per-ifindex. Hence, NML3Cfg
wouldn't be notified about those nodev routes. Consequently, there
os_plobj could not be (efficiently) maintained and there is no
ObjStateData for such routes.
Instead, all that NML3Cfg does is have the routes in the NML3ConfigData and
tell NMPRouteManager about them. Seems simple enough. The only question
is when should NMPRouteManager sync? For now, we sync when the
track/untracking brings any changes and during reapply. Which is
probably fine.
(cherry picked from commit 9ab53e561a)
This will be required next, when we will have also routes without a
device. Split the generation of the route list out.
(cherry picked from commit e32bc6d248)
The ACD state handling is unfortunately very complicated. That is, because
we obviously need to track state about how ACD is going (the acd_data, and
in particular NML3AcdAddrState). Then there are various things that can
happen, which are the AcdStateChangeMode enums. All these state-changes
come together in one function: _l3_acd_data_state_change(), which is
therefore complicated (I don't think that it would become simpler by
spreading this code out to different functions, on the contrary).
Anyway.
So, what happens when we need to reset the n-acd instance? For example,
because the MAC address of the link changed or some error. I guess, we
need to restart probing.
Previously, I think this was not handled properly. We already tried to
fix this several times, the last was commit b331606386 ('l3cfg: on
n-acd instance-reset clear also ready ACD state'). There is still an
issue ([1]).
The bug [1] is, that we are in state NM_L3_ACD_ADDR_STATE_READY, during
ACD_STATE_CHANGE_MODE_TIMEOUT event. That leads to an assertion
failure.
#5 0x00007f23be74698f in g_assertion_message_expr (domain=0x5629aca70359 "nm", file=0x5629aca62aab "src/core/nm-l3cfg.c", line=2395, func=0x5629acb26b30 <__func__.72.lto_priv.4> "_l3_acd_data_state_change", expr=<optimized out>) at ../glib/gtestutils.c:3091
#6 0x00005629ac937e46 in _l3_acd_data_state_change (self=0x5629add69790, acd_data=0x5629add8d520, state_change_mode=ACD_STATE_CHANGE_MODE_TIMEOUT, sender_addr=0x0, p_now_msec=0x7ffded506460) at src/core/nm-l3cfg.c:2395
#7 0x00005629ac939f4d in _l3_acd_data_timeout_cb (user_data=user_data@entry=0x5629add8d520) at src/core/nm-l3cfg.c:1933
#8 0x00007f23be71c5a1 in g_timeout_dispatch (source=0x5629addd7a80, callback=0x5629ac939ee0 <_l3_acd_data_timeout_cb>, user_data=0x5629add8d520) at ../glib/gmain.c:4889
#9 0x00007f23be71bd4f in g_main_dispatch (context=0x5629adc6da00) at ../glib/gmain.c:3337
#10 g_main_context_dispatch (context=0x5629adc6da00) at ../glib/gmain.c:4055
That can only happen, (I think) when we scheduled the timeout
during an earlier ACD_STATE_CHANGE_MODE_INSTANCE_RESET event. Meaning,
we need to handle instance-reset better.
Instead, during instance-reset, switch always back to state PROBING, and
let the timeout figure it out.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=2047788
The first point is that ACD timeout is strongly tied to the current state. That
is (somewhat) obvious, because _l3_acd_data_state_set_full() will clear any pending
timeout. So you can only schedule a timeout *after* setting the state,
and setting the next state, will clear the timeout.
Likewise, note that l3_acd_data_state_change() for the event
ACD_STATE_CHANGE_MODE_TIMEOUT asserts that it is only called in the few
states where that is expected. See rhbz#2047788 where that assertion
gets hit.
The first point means that we must only schedule a timer when we are
also in a state that supports that. Add an assertion for that at the
point when scheduling the timeout. The assert at this point is useful,
because it catches the moment when we do the wrong thing (instead of
getting the assertion later during the timeout, when we no longer know
where the error happened).
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2047788
There was a disagreement whether higher priority numbers mean more/less
important. NMDevice and callers of nm_l3cfg_add_config() assumed that
higher numbers are more important, while _l3_config_datas_get_sorted_cmp()
did the inverse.
There is no obvious right or wrong. We only need to agree. It seems
slightly more sensible to keep the caller's interpretation.
The n-acd instance gets reset in various cases, for example
when the MAC address changes or during errors.
When that happens, we also need to forget all our pending probes,
because they would reference to a now-defunct n-acd instance.
When the address is currently in state NM_L3_ACD_ADDR_STATE_READY,
we possibly have a pending probe. We need to clean that up. Handle
it the same as in the other cases. Yes, this means we forget about
that the address was ready. But a reset of the n-acd state is a dramatic
event. It warrants some drastic start-over.
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2026288
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2028422
See-also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1035
Fixes: 9a76b07f74 ('l3cfg: fix assertion failure')
The force-commit flag is used to force the commit of an address or a
route from DHCP/RA even when it was removed from platform externally
(for example because it expired). Routes generated from the l3cd
should also have the flag set.
Without this, NM properly re-adds the DHCP address after the lease is
lost and obtained again, but fails to add the prefix-route.
Fixes: 2838b1c5e8 ('core: track force-commit flag for l3cd and platform objects')
https://bugzilla.redhat.com/show_bug.cgi?id=2033991https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1049
Fix the following assertion failure:
src/core/nm-l3cfg.c:2636:_l3_acd_data_state_change: assertion failed: (!acd_data->nacd_probe)
When AcdData enters state NM_L3_ACD_ADDR_STATE_READY, the duplicate
address detection procedure completed successfully but the address is
not configured yet on the interface. In the READY state we don't clear
the probe because the same probe can be reused also for defending the
address. Change the assertion.
https://bugzilla.redhat.com/show_bug.cgi?id=2026288https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1035
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
Problem: if l3cfg commits an address and routes from DHCP, when the
address expires those objects are removed automatically. NM tracks the
objects as missing as if the user removed them. This is to prevent
l3cfg to committing them again. If the lease if renewed, l3cfg should
be allowed to commit those objects again.
Introduce a l3cd flag to indicate that it should be force-committed
once, and propagate this flag to platform objects. In this way, l3cfg
can avoid committing again objects that are removed externally, but it
can commit them when the l3cd changes.
Fixes-test: @bridge_down_to_l2_only
If an address is removed externally, it should be considered ready
from ACD point of view. In this way it can be commit again (for
example, if the DHCP lease is renewed).
Sticky update flag forces a commit at UPDATE level after unmanaging
a device. As a result, all the link local addresses will be removed.
To prevent the commit after unmanaging a device, clear sticky update
flag.
Signed-off-by: Wen Liang <liangwen12year@gmail.com>
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>
