==6207== Syscall param ioctl(SIOCETHTOOL) points to uninitialised byte(s)
==6207== at 0x514603B: ioctl (syscall-template.S:78)
==6207== by 0x19FC2F: _ioctl_call (nm-platform-utils.c:183)
==6207== by 0x1A026B: _ethtool_call_handle (nm-platform-utils.c:319)
==6207== by 0x1A031F: ethtool_get_stringset (nm-platform-utils.c:378)
==6207== by 0x1A03BC: ethtool_get_stringset_index (nm-platform-utils.c:414)
==6207== by 0x1A181E: nmp_utils_ethtool_supports_vlans (nm-platform-utils.c:912)
==6207== by 0x1756D7: link_supports_vlans (nm-linux-platform.c:6508)
==6207== by 0x1A81D8: nm_platform_link_supports_vlans (nm-platform.c:1536)
==6207== by 0x14B96B: test_internal (test-link.c:602)
==6207== by 0x4F5C18D: test_case_run (gtestutils.c:2597)
==6207== by 0x4F5C18D: g_test_run_suite_internal (gtestutils.c:2685)
==6207== by 0x4F5BF33: g_test_run_suite_internal (gtestutils.c:2697)
==6207== by 0x4F5C679: g_test_run_suite (gtestutils.c:2772)
==6207== by 0x4F5C694: g_test_run (gtestutils.c:2007)
==6207== by 0x166B4D: main (test-common.c:2092)
==6207== Address 0x1ffeffeecf is on thread 1's stack
==6207== in frame #1, created by _ioctl_call (nm-platform-utils.c:110)
==6207==
"ifname" is the stack-allocated array "known_ifnames" of suitable
IFNAMSIZ bytes. But it may not be fully initialized, so using memcpy()
to copy the string leads to unintialized warning.
We really should only copy the valid bytes, either with strcpy() or our
nm_utils_ifname_cpy() wrapper.
Fixes: 856322562e ('platform/ethtool,mii: retry ioctl when interface name was renamed for ehttool/mii')
"struct ifreq" contains a union field, and initalizing the struct is not
guaranteed to fill all bytes with zero (it only sets the first union
member to zero).
Since we later return the entire struct, ensure that it's initialized to
all zero by using memset().
ethtool/mii API is based on the ifname. As an interface can be renamed,
this API is inherently racy. We would prefer to use the ifindex instead.
The ifindex of a device cannot change (altough it can repeat, which opens a
different race *sigh*).
Anyway, we were already trying to minimize the race be resolving the
name from ifindex immediately before the call to ethtool/mii.
Do better than that. Now resolve the name before and after the call. If
the name changed in the meantime, we have an indication that a race
might have happend (but we cannot be sure).
Note that this can not catch every possible kind of rename race. If you are very
unlucky a swapping of names cannot be detected.
For getters this is relatively straight forward. Just retry when we
have an indication to fall victim to a race (up to a few times). Yes, we
still cannot be 100% sure, but this should be very reliable in practice.
For setters (that modify the device) we also retry. We do so under the
assumption that setting the same options multiple times has no bad effect.
Note that for setters the race of swapping interface names is particularly
bad. If we hit a very unlucky race condition, we might set the setting on
the wrong interface and there is nothing we can do about it. The retry only
ensures that eventually we will set it on the right interface.
Note that this involves one more if_indextoname() call for each operation (in
the common case when there is no renaming race). In cases where we make
multiple ioctl calls, we cache and reuse the information though. So, for such
calls the overhead is even smaller.
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
(cherry picked from commit af07ed01c0)
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()).
For static functions inside a module, the compiler determines on its own
whether to inline the function.
Also, "inline" was used at some places that don't immediatly look like
candidates for inlining. It was most likely a copy&paste error.
In the past, the headers "linux/if.h" and "net/if.h" were incompatible.
That means, we can either include one or the other, but not both.
This is fixed in the meantime, however the issue still exists when
building against older kernel/glibc.
That means, including one of these headers from a header file
is problematic. In particular if it's a header like "nm-platform.h",
which itself is dragged in by many other headers.
Avoid that by not including these headers from "platform.h", but instead
from the source files where needed (or possibly from less popular header
files).
Currently there is no problem. However, this allows an unknowing user to
include <net/if.h> at the same time with "nm-platform.h", which is easy
to get wrong.
As of upstream kernel v4.18-rc8.
Note that we name the features like they are called in ethtool's
ioctl API ETH_SS_FEATURES.
Except, for features like "tx-gro", which ethtool utility aliases
as "gro". So, for those features where ethtool has a built-in,
alternative name, we prefer the alias.
And again, note that a few aliases of ethtool utility ("sg", "tso", "tx")
actually affect more than one underlying kernel feature.
Note that 3 kernel features which are announced via ETH_SS_FEATURES are
explicitly exluded because kernel marks them as "never_changed":
#define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \
NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)
Also, add two more features "tx-tcp-segmentation" and
"tx-tcp6-segmentation". There are two reasons for that:
- systemd-networkd supports setting these two features,
so lets support them too (apparently they are important
enough for networkd).
- these two features are already implicitly covered by "tso".
Like for the "ethtool" program, "tso" is an alias for several
actual features. By adding two features that are already
also covered by an alias (which sets multiple kernel names
at once), we showcase how aliases for the same feature can
coexist. In particular, note how setting
"tso on tx-tcp6-segmentation off" will behave as one would
expect: all 4 tso features covered by the alias are enabled,
except that particular one.
There is little difference in practice because there is only one caller.
Still re-use the SocketHandle also for mii. If only, to make it clear
that SocketHandle is not only suitable for ethtool, but also mii.
Previously, each call to ethtool_get() would resolve the ifindex and
create a new socket for the ethtool request.
This is partly done, because ethtool only supports making requests by
name. Since interfaces can be renamed, this is inherrently racy. So,
we want to fetch the latest name shortly before making the request.
Some functions like nmp_utils_ethtool_supports_vlans() require multiple
ioctls. And next, we will introduce more ethtool functions, that make an
even larger number of individual requests.
Add a simple SocketHandle struct, to create the socket once and reuse
it for multiple requests. This is still entirely internal API in
"nm-platform-utils.c".
ethtool_get_stringset() will be used later, independently.
Also, don't trust and ensure that the block of strings
returned by ETHTOOL_GSTRINGS are NUL terminated.
We already have IP4LL and maybe should re-use that also for IPv6.
However, when adding the prefix route for IPv6 link local addresses,
we want to add it with protocol "kernel", unlike "user" for IPv4.
There is no strong reason for this. I don't think the protocol matters
much. But up to now kernel automatically adds this prefix route, so
as we are going to change that and let NetworkManager add it, keep the
protocol at "kernel".
We call nmp_utils_ethtool_get_driver_info() twice when receiving a
netlink message, but we don't need a clone of the string values.
Instead, expose a data structure that should be stack allocated
by the caller.
The ioctl APIs ethtool/mii require an interface ifname. That is inherrently
racy as interfaces can be renamed. This cannot be fixed, we can only
minimize the time between verifying the ifname and calling ioctl.
We already had problems with that when ethtool would access an interface
by name that didn't exists. See commit ab41c13b06 .
Checking for an existing interface only helps avoiding races when an interface
gets deleted. It does not help against renaming.
Go one step further, and instead of checking whether such an ifname
exists, try to get the ifname based on the ifindex immediately before
we need it.
This brings an additional overhead for each ethtool access.
- use nm_auto_close cleanup attribute
- optionally, return the found ifname
- don't stat "phy80211". If such an entity can be opened,
just assume it's a directory.
Added platform functions to retrieve device link mode status and to
switch from auto to manual link negotiation:
nm_platform_ethtool_get_link_settings
nm_platform_ethtool_set_link_settings
We'll soon not only do the router discovery, but announce ourselves as a
reouter. "Neighbor discovery" sounds to be a more appropriate name for
the class than "Router discovery".
For addresses (NMPlatformIPAddress) the @addr_source field is ignored
on a platform level. That is, all addresses inside the platform cache
have this value set to NM_IP_CONFIG_SOURCE_KERNEL. Maybe, for that reason,
the source should not be a part of the NMPlatformIPAddress structure, but
it is convenient for users to piggy back the source inside the platform
address structure.
For routes, the source is stored in NMPlatformIPRoute's @rt_source
field. When adding a route to kernel, we set the @rtm_protocol of the
route depending on the source. However, we want to map different source
values to the same protocol value.
On the other hand, when kernel sends us a route that gets put inside
the cache, we must preserve the protocol value and must not map
different protocol values to the same source.
The reason is, that a user can add two routes that only differ by
@rtm_protocol. In that sense, the @rtm_protocol fields is part of the
unique ID of a kernel route, and thus different values must map to
different sources.
Fix this, by extending the range of NMIPConfigSource to contain
a range of protocol fields.
We handle cloned routes (that have rtm_flags RTM_F_CLONED) differently.
We used to mark such routes by hacking NMIPConfigSource to have a special
value. No longer do this, because it mixes different concepts.
Note that the rt_cloned filed fits into a hole in the aligment
of NMPlatformIPRoute. Thus there is almost no overhead to this
change.
Most functions defined in "nm-platform-utils.h" perform a lookup
of link properties, for example via ethtool or sysfs. Those functions
depend on the system configuration, such as the current network namespace.
Move the simple helper functions away to "nm-core-internal.h", so that
all remaining functions from "nm-platform-utils.h" are really related to
somthing that interacts with the system/kernel.
Coverity complains rightly about "strncpy (dst, ifname, IFNAMSIZ)"
because it might leave @dst non-NULL-terminated, in case @ifname
is too long (which already would be a bug in the first place).
Replace the strcpy() uses by a new helper nm_utils_ifname_cpy()
that asserts against valid arguments.
"NetworkManagerUtils.h" contains a bunch of helper tools for core
daemon ("src/").
Unfortunately, it has dependencies to other parts of core,
such as "nm-device.h" and "nm-platform.h". Split out a part
of tools that are independent so that they can be used without
dragging in other dependencies.
"nm-core-utils.h" should only use libnm-core, "nm-logging.h"
and shared.
"NetworkManagerUtils.h" should provide all "nm-core-utils.h" and
possibly other utilities that have larger dependencies.
Drivers are stupid, and just like the platform ignores an all zeros
permanent address, so should it ignore all ones.
NetworkManager[509]: <debug> [1453743778.854919] [devices/nm-device.c:8885] nm_device_update_hw_address(): [0x190370] (eth0): hardware address now 86:18:52:xx:xx:xx
NetworkManager[509]: <debug> [1453743778.855438] [devices/nm-device.c:9138] constructed(): [0x190370] (eth0): read initial MAC address 86:18:52:xx:xx:xx
NetworkManager[509]: <debug> [1453743778.861602] [devices/nm-device.c:9148] constructed(): [0x190370] (eth0): read permanent MAC address FF:FF:FF:FF:FF:FF
Add a new 'ignore' option to NMSettingWired.wake-on-lan which disables
management of wake-on-lan by NetworkManager (i.e. the pre-existing
option will not be touched). Also, change the default behavior to be
'ignore' instead of 'disabled'.
https://bugzilla.gnome.org/show_bug.cgi?id=755182
