The number of authentication retires is useful also for passwords aside
802-1x settings. For example, src/devices/wifi/nm-device-wifi.c also has
a retry counter and uses a hard-coded value of 3.
Move the setting, so that it can be used in general. Although it is still
not implemented for other settings.
This is an API and ABI break.
Only NMPolicy should be concerned with handling autoconnect, and
blocking it.
Move the code. Note that there is a slight possible change in
behavior, as the order of when the connection is blocked changes,
based on the different times when the device changed signal gets
executed. But that shouldn't be a problem.
Since commit 4a6fd0e83e (device: honor the
connection.autoconnect-retries for 802.1X) and the related bug bgo#723084,
we reuse the autoconnect-retries setting to control the retry count
for requesting passwords.
I think that is wrong. These are two different settings, we should not
reuse the autoconnect retry counter while the device is still active.
For example, the user might wish to set autoconnect-retries to infinity
(zero). In that case, we would retry indefinitly to request a password.
That could be problematic, if there is a different issue with the
connection, that makes it appear tha the password is wrong.
A full re-activation might succeed, but we would never stop retrying
to authenticate. Instead, we should have two different settings for
retrying to authenticate and to autoconnect.
This is a change in behavior compared to 1.8.
Names like
- nm_settings_connection_get_autoconnect_retries
- nm_settings_connection_set_autoconnect_retries
- nm_settings_connection_reset_autoconnect_retries
are about the same thing, but they are cumbersome to grep
because they share not a common prefix.
Rename them from SUBJECT_VERB_OBJECT to SUBJECT_OBJECT_VERB,
which sounds odd in English, but seems preferred to me.
Now you can grep for "nm_settings_connection_autoconnect_retries_" to
get all accessors of the retry count, or "nm_settings_connection_autoconnect_"
to get all accessors related to autoconnect in general.
Since 32b3eb1181 [core: merge IPv4 and IPv6 implementation of
nm_utils_ip4_property_path()], nm_utils_sysctl_ip_conf_path() introduced
in cd271d5cb1 [core: add nm_utils_sysctl_ip_conf_is_path() util] is used to
cunstruct sysctl paths and it is way less tolerant towards using something
that is not an interface name in the path.
It's always been incorrect to assume the ifname is a linux link name and
it resulted it ugly, if benign, sysctl access attempts such as
"/sys/class/net/28:B2:BD:5D:23:AB/phys_port_id" etc.
Now it triggers an assertion failure. Let's guard all such accesses.
Fixes: 32b3eb1181
Fixes: cd271d5cb1
For a while now, all NMPObject instances are not modified after
being cached. They are immutable, and can be passed around by keeping
a reference to them.
No longer copy the NMPlatformLink data to a @info variable. Instead,
take a reference (which ensures that the instance stays alive). It
won't change, as it's immutable.
The advantage is, that whenever you see a NMPlatformLink pointer,
for exmple in device_recheck_slave_status(), you can be sure that
it's actually a NMPObect, and NMP_OBJECT_UP_CAST() will work.
We now can be enslaved and have L3 configuration at the same time.
This also reduces some unnecessary complexity, because the decision to
progress to IP_CHECK or SECONDARIES now happens in a single place, in
the check_ip_state() routine.
The OpenVSwitch interfaces come into existence by their enslavement to a port.
They can also bear an IP4 or IP6 configuration -- waiting on a carrier would
deadlock the acitvation.
That one is special. All interfaces that are attached to OpenVSwitch
ports appear as slaves to that one even for our purposes we like to
pretend they're slaves to the actual OpenVSwitch bridges.
For some software devices, the platform link appears only after they've been
realized. Update their properties and let them know that the link has changed
so they can eventually proceed with activation.
Also, reset the properties (udi, iface, driver) that are set from the platform
link when the link goes away. At that point they don't reflect reality anymore.
Removes some code duplication too.
2. NetworkManager-1.9.2/src/devices/nm-device-factory.c:312:
returned_pointer: Assigning value from "g_slist_append(list,
g_object_ref(factory))" to "list" here, but that stored value is
overwritten before it can be used.
Fixes: 98afc76184
Fixes: 449940af1d
Coverity doesn't like this. Refactor a bit, hoping that it fares better.
1. Defect type: ASSERT_SIDE_EFFECT
1. NetworkManager-1.9.2/src/devices/nm-device.c:10226: assignment_where_comparison_intended: Assignment "ip_ifindex = nm_device_get_ip_ifindex(self)" has a side effect. This code will work differently in a non-debug build.
2. NetworkManager-1.9.2/src/devices/nm-device.c:10226: remediation: Did you intend to use a comparison ("==") instead?
2. NetworkManager-1.9.2/src/devices/nm-device-factory.c:312:
returned_pointer: Assigning value from "g_slist_append(list,
g_object_ref(factory))" to "list" here, but that stored value is
overwritten before it can be used.
Fixes: 98afc76184
2. NetworkManager-1.9.2/src/devices/nm-device-factory.c:312:
returned_pointer: Assigning value from "g_slist_append(list,
g_object_ref(factory))" to "list" here, but that stored value is
overwritten before it can be used.
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).
Add a proxy setting to generated connections because after commit
6f94b16507 ("libnm: fix nm_connection_diff() for settings without
properties") the matching between a connection having the setting and
another connection without it fails. Before the commit, since no proxy
property is marked as inferrable, such comparison succeeded.
Since commit 5c299454b4 we can configure
multiple default-routes.
That is especially useful with IPv6 to configure multiple routers.
It will also be useful, once we allow configuring manual default-routes,
like regular static routes.
However the problem is, that the default-route for the manual gateway
and the gateway from DHCP both get the same metric. So it's undefined
which route is used. To avoid that problem, and to restore previous
behavior, don't accept any default-routes if a gateway is set.
Fixes: 5c299454b4
Setting the MTU might fail when the underlying device's MTU
is not set.
Detect that case, and log a better warning message.
Unfortunately, it's tricky to detect whether this is a complete
failure, or whether we will later try again to change the MTU.
So, we log a failure, altough later we might fix it. It would
be better not to warn about non-errors.
and nm_utils_ip6_property_path(). The API with static buffers
looks a bit nicer. But I think they are dangerous, because
we tend to pass the buffer down several layers of the stack, and
it's not immediately clear, that we don't overwrite the static
buffer again (which we probably did not, but it's hard to verify
that there is no bug there).
Kernel does not allow setting the MTU of a VLAN larger
then the MTU of the underlying device. Hence, we might
initially fail to set a large MTU of the VLAN, but we
have to retry when the MTU of the parent changes.
https://bugzilla.redhat.com/show_bug.cgi?id=1414901
By using a macro, we don't cast all the types to guint. Instead,
we use their native types directly. Hence, we don't need
nm_hash_update_uint64() nor nm_hash_update_ptr().
Also, for types smaller then guint like char, we save hashing
the all zero bytes.
siphash24() is wildly used by projects nowadays.
It's certainly slower then our djb hashing that we used before.
But quite likely it's fast enough for us, given how wildly it is
used. I think it would be hard to profile NetworkManager to show
that the performance of hash tables is the issue, be it with
djb or siphash24.
Certainly with siphash24() it's much harder to exploit the hashing
algorithm to cause worst case hash operations (provided that the
seed is kept private). Does this better resistance against a denial
of service matter for us? Probably not, but let's better be safe then
sorry.
Note that systemd's implementation uses a different seed for each hash
table (at least, after the hash table grows to a certain size).
We don't do that and use only one global seed.
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.
The privious NM_HASH_* macros directly operated on a guint value
and were thus close to the actual implementation.
Replace them by adding a NMHashState struct and accessors to
update the hash state. This hides the implementation better
and would allow us to carry more state. For example, we could
switch to siphash24() transparently.
For now, we still do a form basically djb2 hashing, albeit with
differing start seed.
Also add nm_hash_str() and nm_str_hash():
- nm_hash_str() is our own string hashing implementation
- nm_str_hash() is our own string implementation, but with a
GHashFunc signature, suitable to pass it to g_hash_table_new().
Also, it has this name in order to remind you of g_str_hash(),
which it is replacing.
