Merge lots of fixes in preparation for the 1.2.0 release - #133
Open
apconole wants to merge 23 commits into
Open
Conversation
In an environment with 576 virtual NICs, when the function eloop_sock_table_dispatch calls FD_ISSET, the value of file descriptor table->table[i].sock exceeds 1024 (the kernel fds structure has only 1024 bits). As a result, the glibc determines that a buffer overflow occurs and aborts the process. To solve this problem, we change select to poll because poll has no restriction on the fd size. Poll allows file descriptors to listen to different events, such as POLLIN and POLLOUT. Therefore, no need to create different queues for different events, we delete writers and exceptions and rename readers to sock_table. Signed-off-by: Yizhen Hu <huyizhen2024@163.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
The existing test suites are difficult to execute, and require a number of special setup to work properly. Not every test has good documentation and the execution time is slow. Introduce a new test suite based on pytest to help run concurrent cases and execute from a CI path. Signed-off-by: Aaron Conole <aconole@redhat.com>
These are the tests stored in the qbg subdirs. Over time, they haven't been well maintained, so future work should be to re-evaulate them for removal or repair. Signed-off-by: Aaron Conole <aconole@redhat.com>
Some of these aren't strictly enforcing an 802.1AB compliance because there could be systems which are relying on the built-in non-compliant behavior. Signed-off-by: Aaron Conole <aconole@redhat.com>
This will make parallel builds functional. Signed-off-by: Aaron Conole <aconole@redhat.com>
This will run the various configurations and setups for our testing and merge work. This should help to merge fixes faster. Signed-off-by: Aaron Conole <aconole@redhat.com>
The QBG and VDP test utils haven't been getting regular updates as the rest of the codebase has been evolving, so the compilation issues that they generate with -Wall -Werror went unnoticed. With the new integration test and santizer jobs, they are now being built so need updating. Signed-off-by: Aaron Conole <aconole@redhat.com>
Unprivilged user NS has many different security policies, and that is evident when attempting to run under github actions. Resolve these issues by switching to relying on 'sudo' instead of using unprivileged userns, etc. This should resolve the issues when trying to run under github actions with the downside being devels needing to run with sudo. Signed-off-by: Aaron Conole <aconole@redhat.com>
GCC 7 through 11 are no longer considered supported, and the workflow jobs for them do not run - rather they stall indefinitely. Signed-off-by: Aaron Conole <aconole@redhat.com>
Looks like this has been an issue since introduction. Signed-off-by: Aaron Conole <aconole@redhat.com>
rxProcessFrame() checked tlv_offset against agent->rx.sizein before reading a 2-byte TLV header at that offset, but the check did not account for the size of the read itself. When tlv_offset landed exactly on the last byte of the received frame, the check passed and the subsequent 2-byte read of *tlv_head_ptr ran one byte past the end of the heap-allocated framein buffer. Caught by AddressSanitizer: READ of size 2 ... heap-buffer-overflow #0 rxProcessFrame lldp/rx.c:175 ... 0 bytes after 34-byte region ... Require room for the full TLV header before dereferencing it. Fixes: a37b7e0 ("lldpad: initial git commit") Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
eloop_run() only reallocates the pollfd array (fds) and re-syncs fds_count when eloop.sock_table.count changes, immediately before calling poll(). But eloop_process_pending_signals(), called after poll() returns and before eloop_sock_table_dispatch(), can invoke a signal handler that registers a new socket and grows eloop.sock_table.count without touching fds. eloop_sock_table_dispatch() then iterated up to the new, larger table->count while indexing into the smaller, stale fds array, reading past its end. Caught by AddressSanitizer: READ of size 2 ... heap-buffer-overflow #0 eloop_sock_table_dispatch eloop.c:212 ... 6 bytes after 32-byte region ... allocated by ... realloc ... eloop_run eloop.c:479 Pass the fds array's actual populated size (fds_count) into eloop_sock_table_dispatch() and bound the dispatch loop by it, so entries added to the table after fds was last sized are simply picked up on a later iteration once fds has been resized to match. Fixes: a37b7e0 ("lldpad: initial git commit") Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
vdpnl_get() declared struct nlattr *tb[IFLA_MAX + 1] (IFLA_MAX + 1 elements, valid indices 0..IFLA_MAX) but passed sizeof(tb) / sizeof(tb[0]), i.e. IFLA_MAX + 1 itself, as the maxtype argument to nla_parse(). nla_parse() zeroes (maxtype + 1) pointer slots at the start of tb, so it wrote IFLA_MAX + 2 entries into an array sized for only IFLA_MAX + 1, overflowing the stack by one pointer. Caught by AddressSanitizer: WRITE of size 424 ... stack-buffer-overflow #0 memset intel#1 nla_parse intel#2 vdpnl_get qbg/vdpnl.c:356 ... offset 1696 overflows this variable ... 'tb' (line 352) Pass IFLA_MAX, matching the conventional nla_parse(tb, IFLA_MAX, ...) usage and the array's valid index range. Fixes: ee8b4d2 ("vdpnl remove mynla_xxx functions") Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
test_missing_end_tlv_rejected assumed that, absent an End Of LLDPDU
TLV, parsing would read garbage bytes past the last real TLV as a
bogus TLV header and reject the resulting garbage declared length via
the body-length overflow check ("Frame overflow error"). That was
only true because of the rx.c off-by-one bounds bug fixed in c41cb93
("rx: Fix heap-buffer-overflow reading TLV header past frame end"),
which let rxProcessFrame() read a TLV header one byte past the end of
framein instead of stopping first.
With that bug fixed, this frame is correctly rejected earlier and more
precisely: the frame ends exactly where the next TLV header would
start, so there isn't room left to read a header at all, and
rxProcessFrame() reports "Frame overrun" rather than reading past the
buffer to reach the body-overflow check. Update the test's expectation
and docstring to match this correct, safe behavior.
Fixes: e5e0957 ("test: Include lldp compliance tests.")
Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
Both NetNS (helpers/netns.py) and PairedNetNS (helpers/paired_netns.py)
create their per-namespace mount namespace with plain
'unshare --mount', then mount fresh tmpfs (and, for PairedNetNS, a
bind mount) onto /tmp and /dev/shm inside it to isolate concurrently
running test cases from each other.
On any host where the root filesystem is mounted with 'shared'
propagation (systemd's default - check with
'findmnt -o PROPAGATION /'), a new mount namespace still starts out as
a peer in that same shared propagation group unless told otherwise:
mounts made *inside* the new namespace propagate straight back out to
the host's own mount namespace instead of staying contained.
In practice this meant:
- NetNS's 'mount -t tmpfs tmpfs /tmp' also mounted a fresh, empty,
root-owned tmpfs over the host's real /tmp, shadowing its
contents and leaving it unmounted only if something explicitly
unmounts it (nothing here does).
- PairedNetNS's 'mount --bind <shared_tmp> /tmp' bind-mounted the
host's real /tmp onto that same ephemeral directory; the
'shutil.rmtree(self._shared_tmp)' in stop() then deleted the
host's actual /tmp contents through that leaked bind mount.
Fix this in two parts:
1. Pass 'unshare --propagation private' so each namespace's mount tree
is detached from the host's propagation group before anything is
mounted inside it - the same technique used by
Docker/systemd-nspawn/LXC for this exact reason. On its own this
was *not* sufficient in testing (still observed leaking onto host
/tmp), most likely because 'ip netns exec' itself unshares its own
mount namespace ahead of ours - one layer more than a single
recursive --propagation pass accounts for - so also explicitly
re-assert MS_PRIVATE ('mount --make-rprivate') on /tmp and /dev/shm
from inside each namespace, right before replacing them. This is
the same two-step "unshare, then explicit mount --make-rprivate"
idiom runc/libcontainer use, rather than relying on unshare(1)'s
--propagation flag alone.
2. PairedNetNS's self._shared_tmp - the bind-mount source for intel#2 above
- was created with tempfile.mkdtemp(prefix="qbg-shared-tmp-") and
no 'dir=', which defaults to the host's real /tmp. That alone put a
real, host-/tmp-resident directory on both ends of the bind mount
regardless of any namespace isolation on the target side, and explains
the deeply nested qbg-shared-tmp-A/qbg-shared-tmp-B/... chains
observed after repeated runs: each new mkdtemp() landed one level
inside the previous run's leaked view of /tmp. Create it under the
same outside-/tmp scratch directory conftest.py's SCRATCH_ROOT
already uses for exactly this reason instead.
This bug predates the recent move to requiring real root (sudo) for
these namespaces: the old unprivileged-userns-based implementation had
the identical propagation gap (mount propagation is governed by peer
group membership, not by which user namespace owns the mount), it just
happened not to get exercised.
Fixes: a9a71f4 ("test: Add existing legacy cases.")
Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
get_dcb_capabilities() and get_dcb_numtcs() each declared 'int i' and incremented it in a for loop purely as decoration - its value was never read anywhere in the loop body or after the loop, only the rta_parent/rta_child pointer comparison and advancement actually drive iteration. Drop the unused variable and the now-pointless increment, converting both loops to while loops. Fixes: a37b7e0 ("lldpad: initial git commit") Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
dont_advertise_dcbx_all() accumulated DCB_LOCAL_CHANGE_* bits into a local 'u32 event_flag', but the function is void and never reads event_flag anywhere - not returned, not passed to the global EventFlag DCB_SET_FLAGS()/run_feature_protocol() machinery dcb_protocol.c uses for this exact purpose elsewhere. It was a pure dead store with no effect on behavior. Remove it along with the now-pointless |= assignments. Fixes: a37b7e0 ("lldpad: initial git commit") Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
set_ets_tsa_map() declared 'int i' and incremented it in a for loop purely as decoration - its value was never read anywhere in the loop body or after the loop, only the 'tokens' pointer (advanced via strtok()) actually drives iteration. Drop the unused variable and the now-pointless increment, converting the loop to a while loop. Fixes: a59f219 ("lldpad: Add IEEE 802.1Qaz module") Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
mgrid2str() called strncpy(buf, (char *)p->mgrid, len) with len set to
the *destination* buffer's size (mgridbuf, VDP_UUID_STRLEN + 2 = 42
bytes), while p->mgrid is a fixed 16-byte field (VDP22_MGRIDSZ) filled
via a raw memcpy() from wire/config data elsewhere (vdp22_bridge_create()),
with no guarantee of a nul byte anywhere within those 16 bytes.
strncpy() reads from the source until it finds a nul or has copied
'len' bytes, whichever comes first - with no nul within the first 16
bytes of p->mgrid, this reads up to len (42) bytes from a 16-byte
field, running into whatever else follows it in struct vsi22. GCC's
-Wstringop-truncation ('specified bound equals destination size') was
flagging exactly this pattern, one that also happens to not guarantee
buf ends up nul-terminated even when it doesn't run past p->mgrid.
The function had already computed the right bound just above ('nul',
the index of the last non-nul byte within p->mgrid, found by scanning
backward): copy exactly that many bytes with memcpy() and
nul-terminate explicitly, instead of asking strncpy() to find a
terminator that isn't guaranteed to exist.
Fixes: 1c96e28 ("vdp22 support tracing for manager id")
Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
Unlike the other basic TLV types (System Name, Port Description, etc.), the Management Address TLV may appear more than once in a single LLDPDU, e.g. one per address family (IPv4, IPv6). Switches commonly do this. Commit 44006eb ("lldp: Reject frames with duplicate TLVs") treated a second Management Address TLV as a fatal frame error and aborted processing the entire frame with `goto out`. This caused all subsequent TLVs (including 802.1Qaz ETS/PFC/APP TLVs and the End TLV) to be skipped, resulting in the ETS state machine never receiving the peer's configuration. This broke PFC/ETS learning from switches that send multiple Management Address TLVs, such as Nvidia Cumulus Linux switches (MSN4600, MSN4700). Fix by removing the checks for if an additional TLV type 8 is received. Instead leaving the idempotent check that the RCVD_LLDP_TLV_TYPE8 bit is set and then freeing the tlv and continuing on with the processing of TLVs. Due to mgmtadd being write-only, it has been removed as there is no need for it in rxProcessFrame. Signed-off-by: Michal Schmidt <mschmidt@redhat.com> Signed-off-by: Thomas Walsh <thwalsh@redhat.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
No existing test exercised the Management Address TLV (type 8) at all, let alone lldpad's tolerance for a second one - the gap that 5035bd3 ("lldp: Tolerate multiple management address TLVs") just fixed. Add a management_address() TLV builder to helpers/lldp_wire.py, and a compliance test that sends two Management Address TLVs (one IPv4, one IPv6, as real switches sending multiple do) followed by a plain optional TLV. Asserting just "the frame wasn't rejected" wouldn't catch a regression back to the old behavior on its own - the old code path aborted the frame with `goto out` on the second Management Address TLV, which happens to also count as "accepted enough" by some looser measure since frame reception itself was never in question. The real bug was that abort skipping every TLV after it, including the End Of LLDPDU TLV. So the test asserts the *later* TLV (a System Name) actually shows up in the neighbor table, which only happens if parsing continued past the second Management Address TLV to the end of the frame. Verified against a live lldpad with the fix applied: 21 passed, 0 failed (test/pytest/test_lldp_compliance.py). Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aaron Conole <aconole@redhat.com>
Simple copy&paste mistake, the output label must match the config key. Fixes: 1d7fc77 ("lldp: Allow lldptool to modify optional TLV's content") Signed-off-by: Marko Hauptvogel <marko.hauptvogel@tu-dresden.de> Signed-off-by: Aaron Conole <aconole@redhat.com>
Simple copy&paste mistake, the test-flag needs to be true here. Fixes: 092a3d7 ("lldptool: support multiple arguments instead of one") Signed-off-by: Marko Hauptvogel <marko.hauptvogel@tu-dresden.de> Signed-off-by: Aaron Conole <aconole@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull features a number of changes, many bugfixes, and some CI integration to run the legacy VDP/QBG tests.
It is in preparation for cutting the branch-1.2, which will then be used as the next release. An approval here will mean:
@penguin359 @praeluceo @neelisrinivas @gonzoleeman