Store the asynchronous netlink slot in the Request object and release it
when the request is detached. This cancels the pending callback instead of
keeping the request alive until its reply arrives.
Follow-up for 80d62d4f1a
This adds an IPv4 counterpart to `IPv6ProxyNDPAddress=` for adding
manual entries to the kernel's IPv4 neighbour proxy table (check via
`ip -4 neighbour show proxy dev <dev>`). systemd-networkd only exposed
`IPv4ProxyARP=` for per-interface `proxy_arp` sysctl (automatic proxy
ARP) with no way to manage manual entries from a .network file.
To avoid duplicating the IPv6 proxy NDP code path, both families are
now combined into a single new `networkd-neighbor-proxy` module. The
IPv6 behaviour is preserved: `IPv6ProxyNDPAddress=` still implies
`IPv6ProxyNDP=yes` unless `IPv6ProxyNDP=` is explicitly disabled and
entries are still dropped if the kernel has no IPv6 support.
The same rule is applied to `IPv4ProxyARPAddress=`. It implies
`IPv4ProxyARP=yes` when the sysctl is not explicitly set and has no
effect if `IPv4ProxyARP=` has been set to false.
This keeps the user model symmetric and predictable across both
families: a single per-address setting that turns on the matching
per-interface sysctl automatically, while still letting system
administrators opt out by setting the boolean explicitly to false.
Note that the IPv4 manual NTF_PROXY entries installed here would
actually function without `proxy_arp` (unlike IPv6, where `proxy_ndp`
gates the manual entries); the implication is kept for symmetry with
`IPv6ProxyNDPAddress=` and is now called out explicitly in the man
page, together with the fact that enabling `proxy_arp` also activates
interface-wide automatic proxy ARP for routed-toward addresses on
connected subnets.
Parser-time validation rejects addresses the kernel would refuse:
the ANY/null address for both families, IPv4 and IPv6 multicast
and the IPv4 limited broadcast 255.255.255.255.
Signed-off-by: Aritra Basu <aritrbas+gh@cisco.com>
When a [SR-IOV] section has no setting, e.g.
```ini
[SR-IOV]
VirtualFunction=0
```
then the kernel previously replied -EINVAL, as we send a rtnl message
with an empty IFLA_VF_INFO container.
See See do_setvfinfo() in net/core/rtnetlink.c of the kernel.
When a [SR-IOV] section that has an unsupported settings by the
interface driver, then previously the kernel partially applied
settings and returned -EOPNOTSUPP. E.f.
```ini
[SR-IOV]
VirtualFunction=0
LinkState=auto
Trust=true
MACAddress=02:01:00:3e:61:34
```
and the interface does not support configuring the link state, then
the MAC address is assigned, but the trust is not applied:
```
enp3s0f0: Failed to configure SR-IOV virtual function 0, ignoring: Operation not supported
vf 0 link/ether 02:01:00:3e:61:34 brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off
```
To fix such issues, this makes networkd/udevd send each attribute
for VF one-by-one.
Fixes#37257 and #37275.
If it is already requested, the new request will be anyway silently refused by
link_queue_request_safe(), which returns 0 in such case. Let's return earlier.
There should be no functional change, just refactoring.
Previously,
1. use the passed Route object as is when a route is requested,
2. when the route becomes ready to configure, convert the Route object
if necessary, to resolve outgoing interface name, and split multipath
routes, and save them to the associated interfaces,
3. configure the route with the passed Route object.
However, there are several inconsistencies with what kernel does:
- The kernel does not merge nor split IPv4 multipath routes. However, we
unconditionally split multipath routes to manage.
- The kernel does not set gateway or so to a route if it has nexthop ID.
Fortunately, I do not find any issues caused by the inconsistencies. But
for safety, let's manage routes in a consistent way with the kernel.
This makes,
1. when a route is requested, split IPv6 multipath routes, but keep IPv4
multipath routes as is, and queue (possibly multiple) requests for
the route.
2. when the route becomes ready to configure, resolve nexthop and interface
name, and requeue request if necessary.
3. configure the (possibly split) route.
By using the logic,
- Now we manage routes in a mostly consistent way with the kernel.
- We can drop ConvertedRoutes object.
- Hopefully the code becomes much simpler.
To prevent the request freed in req->process().
This also makes a request that is not requested by a link detached on failure.
Otherwise, the request may periodically processed and failed forever.
This is similar to Request, but will be used on removing configuration
(e.g. address, route, and so on).
By using another queue for removing configuration, then we can avoid to
fill the reply callback buffer in sd-netlink by remove message calls.
Follow-up for 4e6a35e2b2.
Some checks are slightly heavy, and there may be huge number of
interfaces. So, prcessing whole queue multiple times in a single event
may decrease the performance. Let's process the queued requests once per
event.
Currently, link_queue_request_safe(), which is a wrapper of
request_new(), is called with a free function at
- link_request_stacked_netdev() at netdev/netdev.c,
- link_request_address() at networkd-address.c,
- link_request_nexthop() at networkd-nexthop.c,
- link_request_neighbor() at networkd-networkd.c.
For the netdev case, the reference counter of the passed object is increased
only when the function returns 1. So, on failure (with -ENOMEM)
previously we unexpectedly dropped the reference of the NetDev object.
Similarly, for Address and friends, the ownership of the object is moved to the
Request object only when the function returns 1. And on failure, previously
the object was freed twice.
Also, netdev_queue_request(), which is another wrapper of request_new()
potentially leaks memory when the same NetDev object is queued twice.
Fortunately, that should not happen as the function is called only once
per object.
This fixes the above issue, and now the ownership or the reference
counter of the object is changed only when it is succeeded with 1.
Then, all nexthops managed by networkd really exist (unless the kernel
silently removes a nexthop).
This is the same for nexthop already done by
3c283289ae and
0a0c2672db (for address), and
5d098f5d36 (for neighbor).
When an interface is being reconfigured with different bridge vlan
settings, this makes old vlan IDs on the interface removed.
This also makes the PVID= setting support negative boolean value, e.g. "no",
in which case, the currently assigned PVID (typically, assigned by the
kernel when an interface is joined to a bridge) is dropped.
This feature is requested by #15291.
Note, if a .network file has no settings about bridge vlan, networkd
keeps the currently assigned vlan IDs. That's intended, to make not
break existing setups.
When a .network file has only PVID=no line in [BridgeVLAN] section, then
all assigned vlan IDs are removed.
Fixes#29975.
Closes#15291.
This makes Request object takes hash, compare, free, and process functions.
With this change, the logic in networkd-queue.c can be mostly
independent of the type of the request or the object (e.g. Address) assigned
to the request, and it becomes simpler.
In most netlink handlers, we do the following,
1. decrease the message counter,
2. check the link state,
3. error handling,
4. update link state via e.g. link_check_ready().
The first two steps are mostly common, hence let's extract it.
Moreover, this is not only extracting the common logic, but provide a
strong advantage; `request_call_netlink_async()` assigns the relevant
Request object to the userdata of the netlink slot, and the request object
has full information about the message we sent. Hence, in the future,
netlink handler can print more detailed error message. E.g. when
an address is failed to configure, then currently we only show an
address is failed to configure, but with this commit, potentially we can
show which address is failed explicitly.
This does not change such error handling yet. But let's do that later.
Previously, even though all Request object are owned by Manager, they
do not have direct reference to Manager, but through Link or NetDev
object. But, as Link or NetDev can be NULL, we need to conditionalize
how to access Manager from Request with the type of the request.
This makes the way simpler, as now Request object has direct reference
to Manager.
This also rename request_drop() -> request_detach(), as in the previous
commit, the reference counter is introduced, so even if a reference of
a Request object from Manager is dropped, the object may still alive.
The naming `request_drop()` sounds the object will freed by the
function. But it may not. And `request_detach()` suggests the object
will not be managed by Manager any more, and I think it is more
appropreate.
This is just a cleanup, and should not change any behavior.
Currently, all Request object are always owned by Manager, and freed
when it is processed, especially, soon after a netlink message is sent.
So, it is not necessary to introduce the reference counter.
In a later commit, the Request object will _not_ be freed at the time
when a netlink message is sent, but assigned to the relevant netlink
slot as a userdata, and will be freed when a reply is received. So, the
owner of the Request object is changed in its lifetime. In that case, it
is convenient that the object has reference counter to avoid memleak or
double free.
This also renames e.g. request_process_address() -> address_process_request().
Also, this drops type checks such as `assert(req->type == REQUEST_TYPE_ADDRESS)`,
as in the later commits, the function of processing request, e.g.
`address_process_request()`, will be assigned to the Request object when
it is created. And the request type will be used to distinguish and to
avoid deduplicating requests which do not have any assigned objects,
like REQUEST_TYPE_DHCP4_CLIENT. Hence, the type checks in process functions
are mostly not necessary and redundant.
This is mostly cleanups and preparation for later commits, and should
not change any behavior.
This should not change any behavior, as req->netlink_handler is always
qdisc_handler or tclass_handler.
This is just a preparation for a later commit which introduces
request_call_netlink_async().
Previously, when UUID is requested for DUID, then the clients are
configured in callback of bus methods.
But now, 'request queue' was implemented, so we can use it to wait until
the product UUID is obtained.
In general we almost never hit those asserts in production code, so users see
them very rarely, if ever. But either way, we just need something that users
can pass to the developers.
We have quite a few of those asserts, and some have fairly nice messages, but
many are like "WTF?" or "???" or "unexpected something". The error that is
printed includes the file location, and function name. In almost all functions
there's at most one assert, so the function name alone is enough to identify
the failure for a developer. So we don't get much extra from the message, and
we might just as well drop them.
Dropping them makes our code a tiny bit smaller, and most importantly, improves
development experience by making it easy to insert such an assert in the code
without thinking how to phrase the argument.
Luca Boccassi
Aritra Basu
Yu Watanabe
Daan De Meyer
Luca Boccassi
Zbigniew Jędrzejewski-Szmek