The "onlink" flag for IPv4 routes is part of the route ID.
Consider it in nm_platform_ip4_route_cmp().
Also, allow configuring the flag when adding a route.
Note that for IPv6, the onlink flag is still ignored.
Pretty much like kernel does.
We need to pass more alias-types. Instead of having numbered
versions, use variadic number of macro arguments.
Also, fix build failure with old compiler:
In file included from src/nm-ip6-config.c:24:
./src/nm-ip6-config.h:44:29: error: controlling expression type 'typeof (ipconf_iter->current->obj)' (aka 'const void *const') not compatible with any generic association type
*out_address = has_next ? NMP_OBJECT_CAST_IP6_ADDRESS (ipconf_iter->current->obj) : NULL;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fixes: b1810d7a68
_NM_GET_PRIVATE() used typeof() to propagate constness of the @self
pointer. However, that means, it could only be used with a self pointer
of the exact type. That means, you explicitly had to cast from (GObject *)
or from (void *).
The requirement is cumbersome, and often led us to either create @self
pointer we didn't need:
NMDeviceVlan *self = NM_DEVICE_VLAN (device);
NMDeviceVlanPrivate *priv = NM_DEVICE_VLAN_GET_PRIVATE (self);
or casting:
NMDeviceVlanPrivate *priv = NM_DEVICE_VLAN_GET_PRIVATE ((NMDevice *) device);
In both cases we forcefully cast the source variable, loosing help from
the compiler to detect a bug.
For "nm-linux-platform.c", instead we commonly have a pointer of type
NMPlatform. Hence, we always forcefully cast the type via _NM_GET_PRIVATE_VOID().
Rework the macro to use _Generic(). If compiler supports _Generic(), then we
will get all compile time checks as desired. If the compiler doesn't support
_Generic(), it will still work. You don't get the compile-time checking of course,
but you'd notice that something is wrong once you build with a suitable
compiler.
30. NetworkManager-1.9.2/src/settings/plugins/keyfile/nms-keyfile-writer.c:218:
check_return: Calling "g_mkdir_with_parents" without checking return
value (as is done elsewhere 4 out of 5
times).
25. NetworkManager-1.9.2/src/platform/nm-linux-platform.c:3969:
check_return: Calling "_nl_send_nlmsg" without checking return value (as
is done elsewhere 4 out of 5 times).
34. NetworkManager-1.9.2/src/nm-core-utils.c:2843:
negative_returns: "fd2" is passed to a parameter that cannot be negative.
26. NetworkManager-1.9.2/src/devices/wwan/nm-modem-broadband.c:897:
check_return: Calling "nm_utils_parse_inaddr_bin" without checking
return value (as is done elsewhere 4 out of 5 times).
3. NetworkManager-1.9.2/src/devices/bluetooth/nm-bluez5-manager.c:386:
check_return: Calling "g_variant_lookup" without checking return value
(as is done elsewhere 79 out of 83 times).
16. NetworkManager-1.9.2/libnm-util/nm-setting.c:405:
check_return: Calling "nm_g_object_set_property" without checking return
value (as is done elsewhere 4 out of 5 times).
Setting the MTU failes under regular conditions, for example when
setting the MTU of a master larger then the MTU of the slaves.
Logging a warning it too alarming.
When comparing an unsigned and a signed integer, the signed integer
is promoted to unsigned, resulting in a very large number.
See the checks "nwrote < len - 1", where nwrote might be -1
to indicate failure. The condition would not be TRUE due to
promoting -1 to the max int value.
Hence, sysctl_set() was rather wrong.
Replace the usage of g_str_hash() with our own nm_str_hash().
GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.
Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.
This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.
At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
These static variables really never be modified.
Mark them as const, which allows the linker to mark them as
read-only.
The problem is libnl3's API, which has these parameters
not as const. Add a workaround for that. Clearly libnl3 is
not gonna modify the policy, that the API was fixed too [1]
[1] b4802a17a7
Kernel does not allow to add a route with table 0 (RT_TABLE_UNSPEC). It
effectively is an alias for the main table. We must consider that when
comparing routes sementically.
No need for duplicate log lines
<debug> [1506146476.8462] platform: link: adding tap tap0 owner 107 group -1
<debug> [1506146476.8462] platform-linux: link: add tap tap0 owner 107 group -1
Merge them.
Also, for consistency change the logging output for adding generic
interfaces in nm_platform_link_add().
After commit 5a69b27a64 ("platform: let platform operations only
consider kernel response") the platform only relies on kernel messages
and doesn't check if a deleted object is gone from the cache. For IPv6
addresses it can happen that the RTM_DELADDR comes after the ack, and
this causes random failures in test /address/ipv6/general-2:
[10.8009] platform: address: deleting IPv6 address 2001:db8:a🅱️1:2:3:4/64, ifindex 12 dev nm-test-device
[10.8009] platform-linux: delayed-action: schedule wait-for-nl-response (seq 55, timeout in 0.199999680, response-type 0)
[10.8009] platform-linux: delayed-action: handle wait-for-nl-response (any)
[10.8009] platform-linux: netlink: recvmsg: new message (2), flags 0x0100, seq 55
[10.8009] platform-linux: delayed-action: complete wait-for-nl-response (seq 55, timeout in 0.199980533, response-type 0, success)
[10.8009] platform-linux: do-delete-ip6-address[12: 2001:db8:a🅱️1:2:3:4]: success
**
NetworkManager:ERROR:src/platform/tests/test-common.c:1127:_ip_address_del: assertion failed: (external_command)
Use the same workaround in place for the addition of IPv6 addresses,
i.e. refetch the object if the address is still present after the ack.
When unenslaving an interface from a bridge, kernel sends a RTM_DELLINK
message with ifi_family AF_BRIDGE. We only care about regular
RTM_DELLINK/RTM_NEWLINK messages, ignore all but ifi_family AF_UNSPEC.
There is also test_nl_bugs_spuroius_dellink(), added in commit
8a87a91813 for related bug rh#1302037.
The workaround was masking a bug in NetworkManager (to not ignore AF_BRIDGE
messages) and can now be removed as well.
Also downgrade <error> logging messages to <warn>. An external
condition should never be able to trigger an <error>, and clearly
there is always a external race that can cause a netlink command
to fail.
Let nm_platform_ip_route_add() and friends return an NMPlatformError
failure reason.
Also, do_add_addrroute() did not return the response from kernel.
Instead, it determined success/failure based on the presence of the
object in the cache. That is racy and does not allow to give a failure
reason from kernel.
Instead, determine success solely based on the netlink reply from
kernel. The received errno shall be authorative, there is no need
to second guess the response.
There is a problem that netlink is not a reliable protocol. In case
of receive buffer overflow, the response is lost and we don't know
whether the command succeeded (it likely did). It's unclear how to fix
that, but for now just return "unspecified" error. We probably avoid
that already by having a huge buffer size.
Also, downgrade the error message to <warn> level. <error> is really
for bugs only.
Inspired from iproute2. As such, don't use libnl3's "struct nl_msg", but
add _nl_addattr_l() and use a stack-allocated "struct nlmsghdr". With
this, we are closer to the raw netlink API. It really is simple enough.
The complicated part of the patch is that we re-use the existing netlink
socket for events. Hence, we must process the socket via our common
event_handler_recvmsgs(). That also means, that we get the netlink
response a few layers down the stack and have to return the result
via DelayedActionWaitForNlResponseData.
This will make us stop worry how relevant are chunks of compat code with
older kernels when deciding whether it's worth supporting/testing them.
As if we actually were testing old kernels.
- cache the result in NMPlatformPrivate. No need to call the virtual
function every time. The result is not ever going to change.
- if we are unable to detect support, assume support. Those features
were added quite a while ago to kernel, we should default to "support".
Note, that we detect support based on the presence of the absence of
certain netlink flags. That means, we will still detect no support.
The only moment when we actually use the fallback value, is when we
didn't encounter an RTM_NEWADDR or AF_INET6-IFLA_AF_SPEC message yet,
which would be very unusual, because we fill the cache initially and
usually will have some addresses there.
- for no strong reason, track "undetected" as numerical value zero,
and "support"/"no-support" as 1/-1. We already did that previously for
_support_user_ipv6ll, so this just unifies the implementations.
The minor reason is that this puts @_support_user_ipv6ll to the BSS
section and allows us to omit initializing priv->check_support_user_ipv6ll_cached
in platforms constructor.
- detect _support_kernel_extended_ifa_flags also based on IPv4
RTM_NEWADDR messages. Originally, extended flags were added for IPv6,
and later to IPv4 as well. Once we see an IPv4 message with IFA_FLAGS,
we know we have support.
Deleting an IPv4 route with metric zero will either delete the intended route,
or if no such route exists, it will delete another existing route with different
metric (but otherwise matching parameters).
I think this is a shortcoming of the kernel API. It allows omitting
the metric during delete. However, it gives not way to express to
explicitly delete an IPv4 route with metric zero, but no other.
Since we only delete routes that we obtain from the platform cache
in the first place, we don't need the workaround. Of course, there
is still a race that platform cache might be out of date at the
moment we attempt to delete the route. Or the cache might be
inconsistent, both cases leading to deletion of the wrong route.
But such cases should be very rare, and only present when the user
changes the routing table outside of NM.
Until now, NetworkManager's platform cache for routes used the quadruple
network/plen,metric,ifindex for equaliy. That is not kernel's
understanding of how routes behave. For example, with `ip route append`
you can add two IPv4 routes that only differ by their gateway. To
the previous form of platform cache, these two routes would wrongly
look identical, as the cache could not contain both routes. This also
easily leads to cache-inconsistencies.
Now that we have NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID, fix the route's
compare operator to match kernel's.
Well, not entirely. Kernel understands more properties for routes then
NetworkManager. Some of these properties may also be part of the ID according
to kernel. To NetworkManager such routes would still look identical as
they only differ in a property that is not understood. This can still
cause cache-inconsistencies. The only fix here is to add support for
all these properties in NetworkManager as well. However, it's less serious,
because with this commit we support several of the more important properties.
See also the related bug rh#1337855 for kernel.
Another difficulty is that `ip route replace` and `ip route change`
changes an existing route. The replaced route has the same
NM_PLATFORM_IP_ROUTE_CMP_TYPE_WEAK_ID, but differ in the actual
NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID:
# ip -d -4 route show dev v
# ip monitor route &
# ip route add 192.168.5.0/24 dev v
192.168.5.0/24 dev v scope link
# ip route change 192.168.5.0/24 dev v scope 10
192.168.5.0/24 dev v scope 10
# ip -d -4 route show dev v
unicast 192.168.5.0/24 proto boot scope 10
Note that we only got one RTM_NEWROUTE message, although from NMPCache's
point of view, a new route (with a particular ID) was added and another
route (with a different ID) was deleted. The cumbersome workaround is,
to keep an ordered list of the routes, and figure out which route was
replaced in response to an RTM_NEWROUTE. In absence of bugs, this should
work fine. However, as we only rely on events, we might wrongly
introduce a cache-inconsistancy as well. See the related bug rh#1337860.
Also drop nm_platform_ip4_route_get() and the like. The ID of routes
is complex, so it makes little sense to look up a route directly.
NMPCache can preserve the order of the objects. Until now, the order
was however arbitrary. Soon we will require to preserve the order of
routes.
During a dump, force appending new objects at the end. That ensures,
correct ordering during the dump.
Note that we track objects in several distrinct indexes. Those partition the
set of all objects. Outside a dump when receiving events about new objects (e.g.
RTM_NEWROUTE), it is very unclear at which place the new object should be sorted.
It is especially unclear, as an object might move from one partition (of
an index) to another.
In general, a deterministic order will only be useful in one particular
instance: the NMP_CACHE_ID_TYPE_ROUTES_BY_DESTINATION index for routes.
In this case, we will ensure a particular order of the routes.
The new device type represents a PPP interface, and will implement the
activation of new-style PPPoE connections, i.e. the ones that don't
claim the parent device.
Via the flags of the RTM_NEWROUTE netlink message, kernel and iproute2
support various variants to add a route.
- ip route add
- ip route change
- ip route replace
- ip route prepend
- ip route append
- ip route test
Previously, our nm_platform_ip4_route_add() function was basically
`ip route replace`. In the future, we should rather user `ip route
append` instead.
Anyway, expose the netlink message flags in the API. This allows to
use the various forms, and makes it also more apparent to the user that
they even exist.
- kernel ignores rtm_tos for IPv6 routes. While iproute2 accepts it,
let libnm reject TOS attribute for routes as well.
- move the tos field from NMPlatformIPRoute to NMPlatformIP4Route.
- the tos field is part of the weak-id of an IPv4 route. Meaning,
`ip route add` can add routes that only differ by their TOS.
