Fixes: 919a61bc ('platform/netlink: extend nl_nlmsghdr_to_str() for genl messages')

We just always want to set this. No need for a setter that is only
called once.

- replace "s_flags" field by explicit boolean fields.

- "s_msg_peek" now is simplified. Previously, we would default
  to peek, unless the user caller nl_socket_disable_msg_peek()
  or set nl_socket_set_msg_buf_size(). Simplify that. We now
  default to peek, unless NL_SOCKET_FLAGS_DISABLE_MSG_PEEK is set.

  We have no callers that call nl_socket_set_msg_buf_size(),
  so we can simplify that logic and just enable peeking by default.

- keep "s_auto_ack" field, although it is always TRUE and there
  is no API to toggle that. However, it is kept as a self-documenting
  thing, so we would know the relevant places where auto-ack matters.

- drop nl_socket_disable_msg_peek(). We have no caller of this function
  and we can set peeking in nl_socket_new(). We also don't need to
  change it after creation of the socket.

The real purpose is that we set the socket options before bind().
For that, we need to be able to specify the flag during nl_socket_new().

Another reason is that these are common questions to ponder while
creating a netlink socket. There shouldn't be several setter functions,
just specify the flag right away. These parameters are not going to
change afterwards (at least, we don't need/use that and we don't have
API for that either).

Print more details for generic netlink messages.

Also, pass the group that we obtained via NETLINK_PKTINFO.

Also, factor out simple to-string methods.

We will need this, for getting nl_pktinfo control messages
that contain the extended destination group number.

Also, drop NL_SOCK_PASSCRED. It was only used to not iterate over the
control messages, but doing that should be cheap.

They are not supposed to be modified.

Whether we use a socket blockingly or non-blocking is usually determined
upfront and does not change. Make it a parameter of nl_socket_new().
Also, it saves an additional syscall.

The meaning of the header depends on the netlink protocol. Add that parameter,
so we can also handle genl.

There are only two callers of nl_socket_new(). One for NETLINK_GENERIC
and one for NETLINK_ROUTE.

We already were enabling ext-ack for the rtnetlink socket. Also enable
it for the genl socket.

Do that, but just moving this inside nl_socket_new(). I cannot imagine a
case where we don't want this.

Create and use new nl_socket_new().

nl_socket_alloc() really does nothing but allocating the struct and
initializing the fd to -1. In all cases, we want to call nl_connect()
right after.

Combine the two. Then we also cannot  have a "struct nl_sock" without a
valid fd. This means several error checks can be dropped.

Note that former nl_connect() did several things at once. Maybe, for
more flexibility one would need to tweak what should be done there.
For now that is not necessary. In any case, if we need more flexibility,
then we would control what nl_connect() (now nl_socket_new()) does, and not
the split between nl_socket_alloc() and nl_connect().

We always call nl_recv() in a loop. If it would be necessary to clear
the variable, then it would need to happen inside the loop. But it's
not necessary.

Add parameter to accept a pre-allocated buffer for nl_recv(). In
practice, rtnetlink messages are not larger than 32k, so we can always
pre-allocate it and avoid the need to heap allocate it.

The socket BPF filter operates on the entire message (skb). One netlink
message (for RTM_NEWROUTE) usually contains a list of routes (struct
nlmsghdr), but the BPF filter was only looking at the first route to decide
whether to throw away the entire message. That is wrong.

This causes that we miss routes. It also means that the response to our
RTM_GETROUTE dump request could be filtered and we poll/wait (unsuccessfully)
for it:

  <trace> [1641829930.4111] platform-linux: netlink: read: wait for ACK for sequence number 26...

To get this right, the BPF filter would have to look at all routes in the
list to find out whether there are any we want to receive (and if
there are any, to pass the entire message, regardless that is might also
contain routes we want to ignore). As BPF does not support loops, that
is not trivial. Arguably, we still could do something where we look at a
bounded, unrolled number of routes, in the hope that there are not more
routes in one message that we support. The problem is that this BPF
filter is supposed to filter out massive amounts of routes, so most
messages will be filled to the brim with routes and we would have to
expect a high number of routes in the RTM_NEWROUTE that need checking.

We could still ignore routes in _new_from_nl_route(), to enter the
cache. That means, we would catch them later than with the BPF filter,
but still before a lot of the overhead of handling them. That probably
will be done next, but for now revert the commit.

This reverts commit e9ca5583.

https://bugzilla.redhat.com/show_bug.cgi?id=2037411

Routing daemons can add a large amount of routes to the
system. Currently NM receives netlink notifications for all those
routes and exposes them on D-Bus. With many routes, the daemon becomes
increasingly slow and uses a lot of memory.

The rtm_protocol field of the route indicates the source of the
route. From /usr/include/linux/rtnetlink.h, the allowed values are:

  #define RTPROT_UNSPEC          0
  #define RTPROT_REDIRECT        1        /* Route installed by ICMP redirects;
                                             not used by current IPv4 */
  #define RTPROT_KERNEL          2        /* Route installed by kernel */
  #define RTPROT_BOOT            3        /* Route installed during boot */
  #define RTPROT_STATIC          4        /* Route installed by administrator */

  /* Values of protocol >= RTPROT_STATIC are not interpreted by kernel;
     they are just passed from user and back as is.
     It will be used by hypothetical multiple routing daemons.
     Note that protocol values should be standardized in order to
     avoid conflicts.
   */

  #define RTPROT_GATED           8        /* Apparently, GateD */
  #define RTPROT_RA              9        /* RDISC/ND router advertisements */
  #define RTPROT_MRT            10        /* Merit MRT */
  #define RTPROT_ZEBRA          11        /* Zebra */
  #define RTPROT_BIRD           12        /* BIRD */
  #define RTPROT_DNROUTED       13        /* DECnet routing daemon */
  #define RTPROT_XORP           14        /* XORP */
  #define RTPROT_NTK            15        /* Netsukuku */
  #define RTPROT_DHCP           16        /* DHCP client */
  #define RTPROT_MROUTED        17        /* Multicast daemon */
  #define RTPROT_KEEPALIVED     18        /* Keepalived daemon */
  #define RTPROT_BABEL          42        /* Babel daemon */
  #define RTPROT_OPENR          99        /* Open Routing (Open/R) Routes */
  #define RTPROT_BGP           186        /* BGP Routes */
  #define RTPROT_ISIS          187        /* ISIS Routes */
  #define RTPROT_OSPF          188        /* OSPF Routes */
  #define RTPROT_RIP           189        /* RIP Routes */
  #define RTPROT_EIGRP         192        /* EIGRP Routes */

Since NM uses only values <= RTPROT_STATIC, plus RTPROT_RA and
RTPROT_DHCP, add a BPF filter to the netlink socket to discard
notifications for other route types.

https://bugzilla.redhat.com/show_bug.cgi?id=1861527
!1038

Thomas Haller authored Nov 09, 2021 and Beniamino Galvani committed Nov 29, 2021

We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.

So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.

As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).

The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as

  Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)

[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format

- make nla_attr_minlen[] and array of uint8_t. That is large enough for
  all values we have.

- don't handle NLA_UNSPEC specially. nla_attr_minlen[NLA_UNSPEC] returns
  zero just fine.

The "utils" part does not seem useful in the name.

Note that we also have NMStrBuf, which is named nm_str_buf_*().
There is an unfortunate similarity between the two, but it's still
distinct enough (in particular, because one takes an NMStrBuf and
the other not).

Subjectively, I think this looks better.

Coverity thinks there is a problem here:

    Error: TAINTED_SCALAR (CWE-20): [#def233]
    NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1437: tainted_argument: Calling function "recvmsg" taints argument "msg".
    NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: tainted_data: Passing tainted expression "msg.msg_controllen" to "g_realloc", which uses it as an allocation size.
    NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
    # 1456|
    # 1457|           msg.msg_controllen *= 2;
    # 1458|->         msg.msg_control = g_realloc(msg.msg_control, msg.msg_controllen);
    # 1459|           goto retry;
    # 1460|       }

but the problem is not the tainted data. The problem is how should
we handle MSG_CTRUNC? If we reach MSG_CTRUNC we already lost a message.
Retrying to receive the next message is not going to fix that and is
wrong.

Also, there really is no reason why any truncation should happen. The only
ancillary data that should be present is the sender information, and for
that our buffer is supposed to be large enough.

So, simply ignore truncation. It shouldn't happen, if it happened we
cannot recover from it (aside failing an assertion), and all we really
care are the retrieved credentials. If truncation happened, we might
not have retrieved the credentials, but then that is for the caller
to handle (by rejecting the message as untrusted).

!872

NetworkManager core is huge. We should try to split out
parts that are independent.

Platform code is already mostly independent. But due to having it
under "src/", there is no strict separation/layering which determines
the parts that can work independently. So, while the code is mostly
independent (in practice), that is not obvious from looking at the
source tree. It thus still contributes to cognitive load.

Add a shared library "shared/nm-platform", which should have no
dependencies on libnm-core or NetworkManager core.

In a first step, move the netlink code there. More should follow.

These SPDX license identifiers are deprecated ([1]). Update them.

[1] https://spdx.org/licenses/

  sed \
     -e '1 s%^/\* SPDX-License-Identifier: \(GPL-2.0\|LGPL-2.1\)+ \*/$%/* SPDX-License-Identifier: \1-or-later */%' \
     -e '1,2 s%^\(--\|#\|//\) SPDX-License-Identifier: \(GPL-2.0\|LGPL-2.1\)+$%\1 SPDX-License-Identifier: \2-or-later%' \
     -i \
     $(git grep -l SPDX-License-Identifier -- \
         ':(exclude)shared/c-*/' \
         ':(exclude)shared/n-*/' \
         ':(exclude)shared/systemd/src' \
         ':(exclude)src/systemd/src')

Our coding style recommends C style comments (/* */) instead of C++
(//). Also, systemd (which we partly fork) uses C style comments for
the SPDX-License-Identifier.

Unify the style.

  $ sed -i '1 s#// SPDX-License-Identifier: \([^ ]\+\)$#/* SPDX-License-Identifier: \1 */#' -- $(git ls-files -- '*.[hc]' '*.[hc]pp')

Thomas Haller authored Sep 28, 2020 and Antonio Cardace committed Sep 28, 2020

sed -i \
     -e 's/^'$'\t'' \*/     */g' \
     -e 's/^'$'\t\t'' \*/         */g' \
     -e 's/^'$'\t\t\t'' \*/             */g' \
     -e 's/^'$'\t\t\t\t'' \*/                 */g' \
     -e 's/^'$'\t\t\t\t\t'' \*/                     */g' \
     -e 's/^'$'\t\t\t\t\t\t'' \*/                         */g' \
     -e 's/^'$'\t\t\t\t\t\t\t'' \*/                             */g' \
     $(git ls-files -- '*.[hc]')

Run:

    ./contrib/scripts/nm-code-format.sh -i
    ./contrib/scripts/nm-code-format.sh -i

Yes, it needs to run twice because the first run doesn't yet produce the
final result.

Signed-off-by: Antonio Cardace <acardace@redhat.com>

Yuri Chornoivan authored Jul 04, 2020 and Thomas Haller committed Jul 07, 2020

!565

  $ find * -type f |xargs perl contrib/scripts/spdx.pl
  $ git rm contrib/scripts/spdx.pl

Kernel 5.2 is adding stricter checking for netlink messages.
In particular, for certain API it checks now that NLA_F_NESTED flag is
set for nested attributes ([1]).

Note that libnl3 does not ever set this flag, and since our netlink
implementation is copied from there, certain netlink messages are now
rejected as invalid.

On the other hand, libmnl always adds this flag ([2]). So we should do that
as well.

In particular, this affects the WireGuard netlink API causing request
from NetworkManager to be rejected ([3]).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b424e432e770d6dd572765459d5b6a96a19c5286
[2] https://git.netfilter.org/libmnl/tree/src/attr.c?id=5937dfcb0185f5cb9cf275992ea701ec4e619d9c#n535
[3] NetworkManager/NetworkManager#212

NetworkManager/NetworkManager!210

We no longer add these. If you use Emacs, configure it yourself.

Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.

No manual changes, just ran commands:

    F=($(git grep -l -e '-\*-'))
    sed '1 { /\/\* *-\*-  *[mM]ode.*\*\/$/d }'     -i "${F[@]}"
    sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"

Check remaining lines with:

    git grep -e '-\*-'

The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.