Vllp client - #64
Merged
Merged
Vllp client#64
Conversation
All of these are reachable by a peer on the bus, and none of them need malice -- a noisy CAN bus or a peer that has lost sync is enough. They were found by a new adversarial-peer test suite and by tightening the buffer accounting in the existing one. net/vllp.c: - vllp_channel_receive() leaked the whole reassembled chain when a message failed its CRC. STAILQ_INIT() has already detached it from the channel at that point, so pb is the only remaining reference and the channel teardown that follows cannot help. One corrupted message costs a message worth of buffers, permanently, so a device on a noisy bus bleeds its pbuf pool until it stops talking to anything. - The reassembly limit was enforced by closing the channel, which calls into the application bound to it -- but the management channel is the one channel with no application. A peer sending more fragments than the limit without ever setting the last-fragment bit dereferenced a NULL function pointer on the net thread. It now drops the link, which is the only sensible response to an oversized management message, and an assert documents the invariant in vllp_channel_net_close(). - vllp_disconnect() reset the management channel's transmit queue but not its reassembly queue. The management channel outlives the session, so a half-reassembled inbound message stayed queued and the next session's fragments were appended to it: the message then failed its CRC, which reset the link, which stranded another partial message. A link that resets under load never recovered. - The management-channel handler's default case returned without freeing the message it had been handed, leaking a buffer per frame carrying an opcode the spec reserves. net/pbuf.c: - pbuf_free_queue_irq_blocked() freed the chain but left the queue head pointing at the freed buffers. Most callers discard the containing object immediately and never notice; the ones that flush a queue and keep using it (vllp_disconnect() on the management channel, l2cap's reassembly flush) freed the same buffers again on the next flush and corrupted the pool free list, which then surfaces as a crash in whatever unrelated code allocates next. It now leaves the queue empty. Tests: - suite_vllp_frames.c: a new suite driving the server from a peer that hand-assembles frames, so the paths a correct client never asks for get exercised on purpose -- oversized management messages, reserved opcodes, and a partial message left behind by a session reset. Each phase then checks that the server still serves and that the pool is where it started. - suite_vllp.c: a heavy payload-corruption phase whose only assertion is buffer accounting, plus an end-of-run pool check. The existing corrupt-1 phase only ever loses a single one-fragment message, which is small enough to hide inside any sane tolerance. Every fix was verified by reverting it and confirming the suite fails and names the cause. Co-Authored-By: Claude <noreply@anthropic.com>
Lets a gateway on a CAN bus offer a console for each unit on the bus: the
gateway is a VLLP client on one link per unit, each link keeps a channel
open to the unit's ordinary "shell" service, and that channel is wired to
a local virtual console. An operator telnets to the gateway and runs
"attach unit3". Nothing changes on the units.
The client role lives in src/net/vllp.c alongside the server rather than
in a file of its own -- it is the same protocol, and the fragmentation,
flow control, retransmission and reassembly code is shared verbatim. What
the client adds is the handshake (a cookie, and a SYN once a second until
the server answers), channel-id allocation, the OPEN/OPEN_RESPONSE half of
channel management, and the mirrored per-channel CRC IVs.
Behind ENABLE_VLLP_CLIENT, default no. This has to be a compile-time
option rather than dead code the linker drops: the client paths hang off
vllp_rx(), the keepalive timer and vllp_maybe_tx(), all of which a server
needs, so --gc-sections cannot reach them. Measured on vexpress-a9, a
server-only build pays 336 bytes of text and nothing at all in RAM --
sizeof(vllp_t) is unchanged, and cmd_show_vllp comes out the same size it
was. With the client compiled in it is 3.5 KB and 48 bytes per link
endpoint.
The show_vllp additions are behind the flag as well, including the
per-channel buffer counts. Those are useful on a server too -- they are
what localises a draining pool to a channel and a queue -- but CLI
commands live in a KEEP'd section, so anything added there is paid for by
every build whether it is reachable or not.
The bind timer is set up in vllp_client_create() rather than on the shared
path, because assigning its callback is what makes the whole bind
machinery reachable: doing it in vllp_create() linked ~600 bytes of it
into images that only ever instantiate servers.
mios ships two orthogonal primitives here and leaves the joining to the
application, because which CAN ids, which console names, which remote
service and how many units are all board policy:
vcon_t *vc = vcon_create("unit1", 4096, 128);
vllp_t *v = vllp_client_create(0x530, 0x531, 8, 3);
vllp_client_bind(v, "shell", unit_console_open, vc);
vllp_client_bind() keeps a channel open for the life of the system,
re-opening it on every new session and, after a refused open, on a
backoff. It asks the application to bind a fresh pushpull each time, so
the pushpull contract ("after close() the network side calls nothing
again") holds across reconnects without the application having to know
about them.
vcon_pushpull_open() is the vcon end of that. It spawns no thread for any
number of consoles: push() and pull() run in net context, and keystrokes
reach the engine through vcon_set_backend_notify(), which is new. Without
that hook pull() only runs when the once-a-second keepalive next moves the
engine, so a keystroke waits up to a full second -- measured 1 ms with the
hook against 1 s without. The alternative was a poll thread per console,
which on a nine-unit gateway is nine stacks.
ENABLE_VCON was off everywhere and no in-tree platform set it, so none of
src/util/vcon.c or src/shell/cmd_vcon.c had ever been compiled, let alone
executed, by CI. host.mk now enables it and there are suites for it.
Tests, all in virtual time:
- vcon: the console itself. Scrollback replay, cursor resync when a
client falls behind the window, multi-client mirroring, input merging,
the ring's wraparound arithmetic, and the consoles/attach commands
including the ^A escape handling.
- vllp-client: the client against the server, over a looped-back virtual
CAN bus. Includes the two invariants that fail silently and totally if
they are wrong -- the mirrored CRC IVs, and that the client advances
its per-channel IV counter for every OPEN it sends because the server
advances its own for every OPEN it receives, including refused ones.
- vcon-vllp: nine units end to end, with real remote shells. Crosstalk
between links, both directions busy at once, detach/re-attach,
keystroke latency, unit reboots, and loss/duplication/corruption. Also
at a CAN-sized buffer pool and at a starved one with injected
allocation failures.
Supporting scaffolding: vcan_loop.{c,h} loops a vcan back on itself so
many endpoints in one binary talk over the virtual bus, and carries the
fault injection; testterm.{c,h} is a buffered stream standing in for an
attached terminal.
Also completes PBUF_ORIGIN_TRACE, which plumbed an origin string through
the pool but never stored it. It now records the origin per arena slot and
pbuf_origin_dump() lists what is checked out, which is what identified the
CRC-failure leak fixed in the previous commit. Left switched off.
Co-Authored-By: Claude <noreply@anthropic.com>
Running "attach" on a shell reached over VLLP brought the device down, and once it stopped doing that, showed nothing. The crash: cmd_attach() polls its terminal through vcon_client_wait(), that terminal is the pushpull-to-stream bridge, and pushpull streams had no poll method -- so poll() hit its assert. Nothing had ever polled one before; the bridge is normally read and written straight from a shell thread. The same applies to any pushpull transport, so BLE L2CAP and MBUS shells were equally affected, and splice_bidir() on such a stream would have gone the same way. pushpull_stream_poll() reports readable when a message is queued and also when the stream has shut down, so a poller wakes up to see the shutdown rather than sleeping through it, and it gives the network side the same "I have room" nudge that read() does before sleeping -- without which a peer that was told to stop sending is never told it may resume. The silence: pushpull_stream_write() only hands data to the network once a fragment fills or someone flushes, and neither cmd_attach() nor vcon_bind()'s pump ever flushed their terminal. Against a plain byte stream that goes unnoticed; against a pushpull stream a nested session's console output sat in the buffer indefinitely. Keystrokes arrived and the far shell answered -- its scrollback grew -- but nothing came back to the operator, so the console looked dead. Both pumps now flush when they run out of things to do, which still lets a burst coalesce into full fragments. cmd_attach() also ignored a negative read from its terminal. With the poll method in place a shut-down terminal is always ready, so the loop would have made no progress, slept on a stream that never blocks, and spun at priority 2 forever. It now leaves the loop, which is also what should happen when the link carrying an attached session drops. Found by asking what happens with nested sessions -- attach to a unit, then attach to something else from that unit's shell. That is a reasonable thing to do on a bus of units and none of it worked. The escape handling itself needed no change and behaves the way screen does: the outermost cmd_attach() is the first filter a byte meets, so ^A d always detaches the outermost session from any depth, and reaching an inner level means doubling the prefix for every level above it. Left that way deliberately -- preferring the innermost cannot be done without the outer loop knowing whether an inner one exists, and the only channel it could learn that on is the console output itself. Written down in cmd_vcon.c, including that the count is a power of two and that detaching an outer session leaves the inner ones attached. The vcon-vllp suite grows a nested-attach phase that attaches to a unit, attaches again from that unit's shell, checks that traffic actually flows through both levels, and checks that each detach sequence unwinds exactly one level. Co-Authored-By: Claude <noreply@anthropic.com>
The other client suite runs mios against mios. That catches a great deal,
because only one of the two roles is new and the server is a reference
that was not touched -- but it cannot catch a convention both ends read
the same wrong way out of the same source file, and the VLLP wire has a
fair number of those: what the MTU byte in the SYN holds, the cookie
becoming the link CRC IV, the per-channel IV derivation and which end
inverts it, the counter deriving those IVs stepping in lockstep, the
OPEN/OPEN_RESPONSE encoding.
This suite drives the mios client against the host reference stack
(host/dsig/vllp.c, what the `dsig` tool ships) playing the server, in sim
mode, over the virtual CAN bus. The roles are the mirror of suite_vllp.c,
where the host client drives the mios server.
Two symmetric mutations show what it buys, both self-consistent so
mios-vs-mios stays green:
- putting the unadapted MTU in the SYN and expecting it there:
vllp-client PASS, vllp-xcheck FAIL
- seeding the per-channel IV counter at a different value:
vllp-client PASS, vllp-xcheck FAIL
hvllp_create_server() was already renamed by vllp_sim.h but never
declared, so this adds it to vllp_sim_api.h along with the result struct
the open-channel callback returns. Two notes there worth keeping: sim mode
has no rx-dispatch thread, so an accepted channel must return NULL
callbacks and be drained with hvllp_channel_read(); and a read that times
out marks the channel closed for good, so the peer blocks with a generous
deadline rather than polling on short ones.
The production pthread build of host/dsig is unaffected and still builds.
Co-Authored-By: Claude <noreply@anthropic.com>
stm32_bxcan_send_pb() copies into the transmit mailbox and leaves the pbuf alone. The direct path in stm32_bxcan_output() gets away with that because it returns the pbuf and the stack frees it, but the queued path in the transmit interrupt spliced a pbuf off our own queue and then dropped the pointer. Every frame that went through the software queue leaked one pbuf, permanently. Frames only queue when the mailbox is still busy, so this needed a burst to show up, and dsig_send_cb() drains its whole queue back to back. On a board with ten VLLP links the pool -- 32 buffers -- was gone in seconds, which starves the whole stack, not just CAN. It went unnoticed this long because a board has to emit dsig to reach the queued path at all: the low-latency path drops rather than queues, and until now the boards using bxcan only ever received. Also guard the pbuf_drop() result, which was passed to send_pb() unchecked, and release the mailbox if it does come back NULL so a failure there cannot wedge transmission. Co-Authored-By: Claude <noreply@anthropic.com>
The queue behind the transmit mailbox had no depth limit. A bus with no other node on it never acknowledges a frame, so the mailbox stays busy indefinitely and the queue grows until the pbuf pool is empty -- which takes down every other protocol on the board, not just CAN. Cap it and count the drops. FDCAN already behaves this way when its hardware FIFO fills. Dropping is the right answer here: protocols that care about a frame retransmit, and the ones that do not are periodic, where the next sample supersedes whatever was dropped. Co-Authored-By: Claude <noreply@anthropic.com>
print_info reported only the error counters, so there was no way to tell from the shell whether the interface was moving packets at all, whether it was dropping them, or whether receives were failing for want of a pbuf -- the last of which was entirely silent. Diagnosing a draining pbuf pool on a bxcan board meant guessing. Report the same shape FDCAN already does: per-FIFO receive counts, NoPbufs, transmitted, drops and the current queue depth. Decode the last error code instead of printing the raw field, and put the three bus-state flags on one line. Co-Authored-By: Claude <noreply@anthropic.com>
PBUF_ORIGIN_TRACE records which allocation site owns each data buffer and pbuf_origin_dump() lists them, but nothing ever called it, so turning the option on produced no output and the tracking was unreachable in practice. Call it from pbuf_status(), so a build with the option on answers "who is holding the pool" through the same command that already reports how little of it is left. Still off by default. Co-Authored-By: Claude <noreply@anthropic.com>
show_vllp listed the channel management channel as ESTABLISHED under a Disconnected link, which reads as though something is up when nothing is. Channel 14 is ESTABLISHED from the moment the link is created: it is a sentinel for "needs no setup", not a state it ever reached. The flow bits and the free-channel-id mask are just as misleading in that state -- both print their reset values, and 0x3fff in particular looks like information. So skip the whole per-channel block when there is no session and print only the header and the binds, where "waiting" is the fact that actually matters. Co-Authored-By: Claude <noreply@anthropic.com>
An output filter was an allow-list only: first prefix match wins and no match means the signal does not go out. That is the wrong shape when all a board wants is to keep one id range off an interface, because expressing it means enumerating every id the board legitimately emits -- an inventory that silently drops the next signal someone adds. An entry with DSIG_FLAG_DENY, followed by a catch-all, states the constraint instead. Backward compatible: no match still means deny, so existing allow-lists and a NULL filter behave exactly as before, and dsig_output() consumes the bit so no driver sees it. Bit 0 is already DSIG_FLAG_EXTENDED, which the bxcan output path reads. mask_from_prefixlen(0) computed 1 << 32, which is undefined, so the catch-all had no well-defined spelling until now. Nothing used a zero prefix length before. Co-Authored-By: Claude <noreply@anthropic.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
1 participant
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.
No description provided.