During nm_lldp_neighbor_parse(), the NMLldpNeighbor is not yet added to
the NMLldpRX instance. Consequently, n->lldp_rx is NULL.
Note how we use lldp_x for logging, because we need it for the context
for which interface the logging statement is.
Thus, those debug logging statements will follow a NULL pointer and lead
to a crash.
Fixes: 630de288d2 ('lldp: add libnm-lldp as fork of systemd's sd_lldp_rx')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1550
When creating the socket for listening to LLDP frames we are setting
NM_ETHERTYPE_LLDP (0x88cc) as protocol. In most of the cases, that is
correct but when the interface is attached as a port to a OVS bridge,
kernel is not matching the protocol correctly. The reason might be that
some metadata is added to the packet, but we are not completely sure
about it.
Instead, we should use ETH_P_ALL to match all the protocols. Later, we
have a eBPF filter to drop the packet by multicast MAC address or
protocol. This is how lldpd is doing it for example.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1903
NMPrioq is taken from systemd's "prioq.c". It is a nice data structure,
that accepts and an index pointer, to directly access elements inside
the heap.
Previously, the API didn't require a consistent index, while the data is
not inside the heap. nm_prioq_{update,shuffle,remove}()) call find_item(),
which silently accepts wrong indexes and assumes the element is not in
the heap.
Keeping the index in sync with the data seems error prone. Accepting any
index without asserting may be convenient for the user (as the user is
not required to pre-initialize the index with NM_PRIOQ_IDX_NULL).
However, it also misses to catch potential bugs.
Now the index must be kept consistent, in particular also if the element
is not enqueued. This means, you must initialize them with
NM_PRIOQ_IDX_NULL.
We currently use the systemd LLDP client, which we consume by forking
systemd code. That is a maintenance burden, because it's not a
self-contained, stable library that we use. Hence there is a need for an
individual library or properly integrating the fork in our tree.
Optimally, we would create a new nettools project with an LLDP library.
That was not done because:
- nettools may want to be dual licensed with LGPL-2.1+ and Apache.
Systemd code is LGPL-2.1+ so it is fine for NetworkManager but
possibly not for nettools.
- nettools provides independent librares, as such they don't have an
event loop, instead they expose an epoll file descriptor and the user
needs to integrate it. Systemd and NetworkManager on the other hand
have their established event loop (sd_event and GMainContext,
respectively). It's simpler to implement the library on those terms,
in particular porting the systemd library from sd_event to
GMainContext.
- NetworkManager uses glib and has various helper utils. While it's
possible to do without them, it's more work.
The main reason to not write a new NetworkManager-agnostic library from
scratch, is that it's much simpler to fork the systemd library and make
it part of NetworkManager, than making it a nettools library.
Do it.
