'sriov_drivers_autoprobe' was added in kernel 4.12. With previous
kernel versions NM is currently unable to set any SR-IOV parameter
because it tries to read 'sriov_drivers_autoprobe' which doesn't
exist, assumes that current value is -1 and tries to change it,
failing.
When the file doesn't exist, drivers are automatically probed so we
can assume the value is 1. In this way NM is able to activate a
connection with sriov.autoprobe-drivers=1 (the default) even on older
kernel versions.
Fixes: 1e41495d9a ('platform: sriov: write new values when we can't read old ones')
https://bugzilla.redhat.com/show_bug.cgi?id=1695093
Add and implement NMPlatformRoutingRule types and let the platform cache
handle rules.
Rules are special in two ways:
- they don't have an ifindex. That makes them different from all other
currently existing NMPlatform* types, which have an "ifindex" field and
"implement" NMPlatformObjWithIfindex.
- they have an address family, but contrary to addresses and routes, there
is only one NMPlatformRoutingRule object to handle both address
families.
Both of these points require some special considerations.
Kernel treats routing-rules quite similar to routes. That is, kernel
allows to add different rules/routes, as long as they differ in certain
fields. These "fields" make up the identity of the rules/routes. But
in practice, it's not defined which fields contribute to the identity
of these objects. That makes using the netlink API very hard. For
example, when kernel gains support for a new attribute which
NetworkManager does not know yet, then users can add two rules/routes
that look the same to NetworkManager. That can easily result in cache
inconsistencies.
Another problem is, that older kernel versions may not yet support all
fields, which NetworkManager (and newer kernels) considers for identity.
The older kernel will not simply reject netlink messages with these unknown
keys, instead it will proceed adding the route/rule without it. That means,
the added route/rule will have a different identity than what NetworkManager
intended to add.
Currently, there is a directy one to one relation between
- DELAYED_ACTION_TYPE_REFRESH_ALL_*
- REFRESH_ALL_TYPE_*
- NMP_OBJECT_TYPE_*
For IP addresses, routes and routing policy rules, when we request
a refresh-all (NLM_F_DUMP), we want to specify the address family.
For addresses and routes that is currently solved by having two
sets of NMPObject types, for each address family one.
I think that is cumbersome because the implementations of both address
families are quite similar. By implementing both families as different
object types, we have a lot of duplicate code and it's hard to see where
the families actually differ. It would be better to have only one NMPObject
type, but then when we "refresh-all" such types, we still want to be able
to dump all (AF_UNSPEC) or only a particular address family (AF_INET, AF_INET6).
Decouple REFRESH_ALL_TYPE_* from NMP_OBJECT_TYPE_* to make that
possible.
While these numbers are strongly related to DELAYED_ACTION_TYPE_REFRESH_ALL_*,
they differ in their meaning.
These are the refresh-all-types that we support. While some of the delayed-actions
are indeed for refresh-all, they are not the same thing.
Rename the enum.
The function is unused. It would require redesign to work with
future changes, and since it's unused, just drop it.
The long reasoning is:
Currently, a refresh-all is tied to an NMPObjectType. However, with
NMPObjectRoutingRule (for policy-routing-rules) that will no longer
be the case.
That is because NMPObjectRoutingRule will be one object type for
AF_INET and AF_INET6. Contrary to IPv4 addresses and routes, where
there are two sets of NMPObject types.
The reason is, that it's preferable to treat IPv4 and IPv6 objects
similarly, that is: as the same type with an address family property.
That also follows netlink, which uses RTM_GET* messages for both
address families, and the address family is expressed inside the
message.
But then an API like nm_platform_refresh_all() makes little sense,
it would require at least an addr_family argument. But since the
API is unused, just drop it.
The condition got accidentally reversed, which means we're always
undecided and thus wrongly assuming support and never being able to set
any addresses.
This would bother the few that are struck with 3.4 android kernels. Very
few indeed, given this got unnoticed since 1.10.
Fixes: 8670aacc7c ('platform: cleanup detecting kernel support for IFA_FLAGS and IPv6LL')
We already have NMPLookup to express a set of objects that can be looked
up via the multi-index.
Change the behavior of nmp_cache_dirty_set_all(), not to accept an
object-type. Instead, make it generally useful and accept a lookup
instance that is used to filter the elements that should be set as
dirty.
for-each macros can be nice to use. However, such macros are potentially
error prone, as they abstract C control statements and scoping blocks.
I find it ugly to expand the macro to:
for (...)
if (...)
Instead, move the additional "if" check inside the loop's condition
expression, so that the macro only expands to one "for(;;)" statement.
It does not matter too much, but the code optimizes for deletion events.
In that case, we don't require parsing the full netlink message.
Instead, it's enough to parse only the ID part so that we can find the
object in the cache that is to be removed removed.
Fix that for qdisc/tfilter objects.
I think the code before was correct. At the very least because
we only run the while-loop at most once because multipath routes
are not supported.
However, it seems odd that the while loop checks for
"tlen >= rtnh->rtnh_len"
but later we do
"tlen -= RTNH_ALIGN (rtnh->rtnh_len)"
Well, arguably, tlen itself is aligned to 4 bytes (as kernel sends
the netlink message that way). So, it was indeed fine.
Still, confusing. Try to check more explicitly for the buffer sizes.
nla_data_as() has a static assertion that casting to the pointer is
valid (regarding the alignment of the structure). It also contains
an nm_assert() that the data is in fact large enough.
It's safer, hence prefer it a some places where it makes sense.
- nlmsg_append_struct() determines the size based on the argument.
It avoids typing, but more importantly: it avoids typing redundant
information (which we might get wrong).
- also, declare the structs as const, where possible.
- drop explicit MAX sizes like
static const struct nla_policy policy[IFLA_INET6_MAX+1] = {
The compiler will deduce that.
It saves redundant information (which is possibly wrong). Also,
the max define might be larger than we actually need it, so we
just waste a few bytes of static memory and do unnecesary steps
during validation.
Also, the compiler will catch bugs, if the array size of policy/tb
is too short for what we access later (-Warray-bounds).
- avoid redundant size specifiers like:
static const struct nla_policy policy[IFLA_INET6_MAX+1] = {
...
struct nlattr *tb[IFLA_INET6_MAX+1];
...
err = nla_parse_nested (tb, IFLA_INET6_MAX, attr, policy);
- use the nla_parse*_arr() macros that determine the maxtype
based on the argument types.
- move declaration of "static const struct nla_policy policy" variable
to the beginning, before auto variables.
- drop unneeded temporay error variables.
In practice, we don't fail to create the nlmsg, because in glib
malloc() cannot fail and we always create large enough buffers.
Anyway, handle the error correctly, and reduce the in-progress
counter again.
We will need more flags.
WireGuard internal tools solve this by embedding the change flags inside
the structure that corresponds to NMPlatformLnkWireGuard. We don't do
that, NMPlatformLnkWireGuard is only for containing the information about
the link.
Use the NM_ERRNO_NATIVE() macro that asserts that these errno numbers are
indeed positive. Using the macro also serves as a documentation of what
the meaning of these numbers is.
That is often not obvious, whether we have an nm_errno(), an nm_errno_native()
(from <errno.h>), or another error number (e.g. WaitForNlResponseResult). This
situation already improved by merging netlink error codes (nle),
NMPlatformError enum and <errno.h> as nm_errno(). But we still must
always be careful about not to mix error codes from different
domains or transform them appropriately (like nm_errno_from_native()).
When the logging level is DEBUG or TRACE, we keep all the sysctl
values we read in a cache to log how they change. Currently there is
no limit on the size of this cache and it can take a large amount of
memory.
Implement a LRU cache where the oldest entries are deleted to make
space for new ones.
https://github.com/NetworkManager/NetworkManager/pull/294
Previously, _nm_logging_clear_platform_logging_cache was an extern variable,
and NMLinuxPlatform would set it to a function pointer at certain points.
That's unnecessary complex, also when trying to make nm-logging thread-safe,
it's just more global variables that need to be considered. Don't do it
that way, but just link in a regular function.
Since commit 9ecdba316 ('platform: create netlink messages directly
without libnl-route-3') we're unconditionally setting IFA_ADDRESS to
the peer address, even if there's no peer and it's all zeroes.
The kernel actually stopped caring somewhere around commit caeaba790
('ipv6: add support of peer address') in v3.10, but Ubuntu Touch likes
to run Android's v3.4 on some poorly supported hardware.
Fixes: 9ecdba316chttps://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/77
The caller may not wish to replace existing peers, but only update/add
the peers explicitly passed to nm_platform_link_wireguard_change().
I think that is in particular interesting, because for the most part
NetworkManager will configure the same set of peers over and over again
(whenever we resolve the DNS name of an IP endpoint of the WireGuard
peer).
At that point, it seems disruptive to drop all peers and re-add them
again. Setting @replace_peers to %FALSE allows to only update/add.
Add cmp/hash functions that correctly honor the well known fields, instead
of doing memcmp/memcpy of the entire sockaddr structure.
Also, move the set function to nm_sock_addr_union_cpy() and
nm_sock_addr_union_cpy_untrusted(). This also gets it right
to ensure all bytes of the union are initialized (to zero).
