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.
Since function nmp_utils_open_sysctl() can avoid race condition, use it
in wifi_utils_is_wifi() to open sysfs and correctly check if it's a wifi
device.
https://bugzilla.gnome.org/show_bug.cgi?id=775613
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Since commit 9fafb382db, we would
explicitly set libnl's socket buffer size to 4*getpagesize().
That is also the default of libnl itself. Additionally, we would
workaround too small buffers by increasing the buffer size up to 512K.
A too small buffer causes messages to be lost. Usually, that only
results in a cache-resync, which isn't too bad. Lost messages are however
a problem if the lost message was an ACK that we were waiting for.
However, it is rather unlikely to happen, because it's expected that
the buffer size gets adjusted already when the cache is filled initially,
before any other requests are pending.
Still, let's increase the default buffer size to 32K, hoping that this
initial value is already large enough to avoid the problem altogether.
Note that iproute2 also uses a buffer size of 32K [1] [2].
Alternatively, we could use MSG_PEEK like systemd does [3]. However,
that requires two syscalls per message.
[1] https://patchwork.ozlabs.org/patch/592178/
[2] https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/tree/lib/libnetlink.c?id=f5f760b81250630da23a4021c30e802695be79d2#n274
[3] cd66af2274/src/libsystemd/sd-netlink/netlink-socket.c (L323)
We don't want to enable MSG_PEEK due to the overhead. But when we detect
that we just lost a message due to MSG_TRUNC, increase the buffer size and
retry.
See-also: 55ea6e6b6c
- use _NM_GET_PRIVATE() and _NM_GET_PRIVATE_PTR() everywhere.
- reorder statements, to have GObject related functions (init, dispose,
constructed) at the bottom of each file and in a consistent order w.r.t.
each other.
- unify whitespaces in signal and properties declarations.
- use NM_GOBJECT_PROPERTIES_DEFINE() and _notify()
- drop unused signal slots in class structures
- drop unused header files for device factories
There is a "goto retry" in do_change_link_request(), at that point,
seq_result has the value -EOPNOTSUPP, instead of
WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN.
Fixes: 02fb3eff48
Trying to set a property on a device that does not exist is not something
necessarily wrong. Don't print error/warning messages.
<trace> [1467707267.2887] device[0x55a74adbdaf0] (enp0s25): set-hw-addr: setting MAC address to 'AA:BB:CC:DD:EE:FF' (reset, unmanage)...
<debug> [1467707267.2887] platform: link: setting '(null)' (2) hardware address
<debug> [1467707267.2887] platform-linux: link: change 2: address: 68:F7:28:61:68:F7 (6 bytes)
<debug> [1467707267.2887] platform-linux: do-request-link: 2
<debug> [1467707267.2888] platform-linux: netlink: recvmsg: error message from kernel: No such device (19) for request 226
<debug> [1467707267.2888] platform-linux: netlink: recvmsg: error message from kernel: No such device (19) for request 227
<error> [1467707267.2888] platform-linux: do-change-link[2]: failure changing link: failure 19 (No such device)
<warn> [1467707267.2888] device (enp0s25): set-hw-addr: failed to reset MAC address to 68:F7:28:61:68:F7 (unmanage)
nl_recv() in libnl3 before version 3.2.15 would return dangling pointers
if nl_recv() fails or has nothing to read [1].
Workaround that by explicitly clearing @buf and @creds.
https://bugzilla.gnome.org/show_bug.cgi?id=767986
[1] 69468517d0
We don't need the token set in platform for our address mode generation,
but having it set makes it possible to correctly generate and assume
connections that use tokens.
The only user of platform who accesses this field is NMDevice,
when calling nm_platform_link_get_ipv6_token(). It cares more
about whether the token is all-zero or set to something.
Another use of inet6_token.is_valid was so that when we receive a
netlink message without IFLA_INET6_TOKEN attribute, that we don't
treat the value as zero, although it is just unknown. Fix that
instead in a better way by setting the value from the cache, if
IFLA_AF_SPEC doesn't provide it.
Also, when printing the token in nm_platform_link_to_string()
treat it as an IPv6 address (inet_ntop).
Sometimes the netlink event lacks the IFLA_ADDRESS attribute with
the MAC address of the link. In this case, take the value from
the cached link instance. A missing netlink attribute should have the
meaning of reusing the previous value, not clearing the address.
It's not enough to consider IF_LOWER_UP flag. Instead,
the important flag is actually IF_UP.
Actually, I suspect that IF_LOWER_UP is not needed. But for
now leave it, in order not to break something.
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.
The "source" field of NMPlatformIPRoute (now "rt_source") maps to the
protocol field of the route. The source of NMPlatformIPAddress (now
"addr_source") has no direct equivalent in the kernel.
As their use is different, they should have different names. Also,
the name "source" is used all over the place. Hence give the fields
a more distinct name.
For some device types, we use the DEVTYPE from sysfs to determine the
link type. However, the way we read from sysfs can race with device
renames and we could miss the chance to read DEVTYPE correctly.
This doesn't completely fix the sysfs race, but cures the boot-time race
with systemd renaming the device while we are initializing the link.
We ideally should use GUDev for all sysfs accesses, but that would need
some more work for this particular case as currently we need the link type
before we have an udev device instance.
https://bugzilla.gnome.org/show_bug.cgi?id=764803
Co-Authored-By: Beniamino Galvani <bgalvani@redhat.com>
