Currently there is no problem. However, DelayedActionType is a packed
enum, and if we add a few more enum values, it might happen that
DELAYED_ACTION_TYPE_MAX is 0x8000 and DelayedActionType effectively
uint16_t.
When that happens, the code would become an infinite loop, because
0x8000 is not larger than DELAYED_ACTION_TYPE_MAX, but `<<= 1`
shifts out the bit, making it zero.
Avoid that.
If nm_platform_get_cache_tc() is disabled, there is no need to refresh
it. Filter those flags out.
Also, don't duplicate the code and add a helper function
delayed_action_schedule_refresh_all().
Reorder fields in DelayedActionWaitForNlResponseData, so that
the struct size is optimal due to the alignment constraints.
Also, when we remember enum values and embed them somewhere, it's nice
if they only take the space actually needed. _nm_packed solves that.
The term "addr_family" is used very frequently, and it usually is an
auto variable or a function parameter.
It is interesting to search where this field is used. So rename to give
it a unique (and better fitting) name.
While at it, use gint8 to encode the addr_family. It's always
sufficient, and this reduces the size of RefreshAllInfo from 8 bytes
to two.
The genl types that we care about are well known. Add an enum
for them, so we can do a lookup by index.
To kernel, the corresponding names (like "wireguard") are also well
known. However, the family-id, that we need when using genl are
allocated dynamically. So we need to lookup the family-id, and by having
an enum for the genl type, we can do so generically.
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.
NMPObject is a union. It's not clear to me that C guarnatees that
designated initializers will meaningfully set all fields to zero. Use
memset() instead.
- replace "s_flags" field by explicit boolean fields.
- "s_msg_peek" now is simplified. Previously, we would default
to peek, unless the user caller nl_socket_disable_msg_peek()
or set nl_socket_set_msg_buf_size(). Simplify that. We now
default to peek, unless NL_SOCKET_FLAGS_DISABLE_MSG_PEEK is set.
We have no callers that call nl_socket_set_msg_buf_size(),
so we can simplify that logic and just enable peeking by default.
- keep "s_auto_ack" field, although it is always TRUE and there
is no API to toggle that. However, it is kept as a self-documenting
thing, so we would know the relevant places where auto-ack matters.
- drop nl_socket_disable_msg_peek(). We have no caller of this function
and we can set peeking in nl_socket_new(). We also don't need to
change it after creation of the socket.
The real purpose is that we set the socket options before bind().
For that, we need to be able to specify the flag during nl_socket_new().
Another reason is that these are common questions to ponder while
creating a netlink socket. There shouldn't be several setter functions,
just specify the flag right away. These parameters are not going to
change afterwards (at least, we don't need/use that and we don't have
API for that either).
We will need this, for getting nl_pktinfo control messages
that contain the extended destination group number.
Also, drop NL_SOCK_PASSCRED. It was only used to not iterate over the
control messages, but doing that should be cheap.
There really is no need for two(!) heap allocations while parsing
the netlink message. We already have it in the buffer. Just use it.
Note that netlink attributes need to be aligned to 4 bytes. But
nlmsg_next() already ensures that, so not even for alignment purpose we
need to clone the message.
Create a new "struct nl_msg_lite" that can hold pointers to everything
we need.
Whether we use a socket blockingly or non-blocking is usually determined
upfront and does not change. Make it a parameter of nl_socket_new().
Also, it saves an additional syscall.
- "priv->nlh" to "priv->sk_rtnl": as we also have an genl socket,
"nlh" is not a good name. The point is that this is rtnetlink.
Also, "h" sounds like a handle, that is, a file descriptor.
Make this clearer with a "sk_" prefix.
- "priv->genl" to "priv->sk_genl_sync": This socket is only used for synchronous
operations, that is, it is passed to various independent components, that use
it to send a request and wait for the response (while consuming all messages).
We will have a use for a second socket, hence the "_sync" part.
The "sk_" prefix is for consistency with "sk_rtnl".
- "priv->event_source" to "priv->rtnl_event_source". Just make it
clearer, that this is for the rtnetlink socket. In any case,
this field is hardly used at all, it can have a sturdy name.
Sockets are really a fundamental thing we require to operate.
We cannot meaningfully operate, if we fail to create them.
That is also why a too low file descriptor limit is fatal
and unsupported. This is similar with out of memory situations.
Just require that we always are able to create the generic
netlink socket.
There are only two callers of nl_socket_new(). One for NETLINK_GENERIC
and one for NETLINK_ROUTE.
We already were enabling ext-ack for the rtnetlink socket. Also enable
it for the genl socket.
Do that, but just moving this inside nl_socket_new(). I cannot imagine a
case where we don't want this.
Create and use new nl_socket_new().
nl_socket_alloc() really does nothing but allocating the struct and
initializing the fd to -1. In all cases, we want to call nl_connect()
right after.
Combine the two. Then we also cannot have a "struct nl_sock" without a
valid fd. This means several error checks can be dropped.
Note that former nl_connect() did several things at once. Maybe, for
more flexibility one would need to tweak what should be done there.
For now that is not necessary. In any case, if we need more flexibility,
then we would control what nl_connect() (now nl_socket_new()) does, and not
the split between nl_socket_alloc() and nl_connect().
On m68k we get a static assertion, that NMPlatformIP4Address.address
is not at the same offset as NMPlatformIPAddress.address_ptr.
On most architectures, the bitfields fits in a gap between the fields,
but not on m68k, where integers are 2-byte aligned.
Try to workaround a coverity warning:
30. NetworkManager-1.39.3/src/core/vpn/nm-vpn-connection.c:2000:
overrun-buffer-val: Overrunning array "address.ax.address_ptr" of 1
bytes by passing it to a function which accesses it at byte offset 3.
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')
Try to do one change at a time when reconfiguring addresses, to not
remove several/all addresses at once.
For IP addresses, kernel cares about the order in which they were added.
This mostly affects source address selection, and the "secondary" flag
for IPv4 addresses. The order is thus related to the priority of an
address.
There is no direct kernel API to change the order. Instead, we have to
add them in the correct order. During a sync, if an address already
exists in the wrong order, we need to remove it, and re-add it.
Btw, with IPv4 addresses added first via netlink are the primary
address, while with IPv6 it's reverse.
Previously, we would first iterate over all addresses and remove those
that had a conflicting order. This means, that we would potentially
remove all addresses for a short while, before readding them. That seems
problematic.
Instead, first track all addresses that are in the wrong order. And in
the step when we add/update the address, remove it. We now only remove
and address shortly before re-adding it. This way the time for which the
address on the interface is missing is shorter. More importantly, we will
never remove all addresses at the same time.
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
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.
