oxidecomputer / oxidecomputer/maghemite

BFD doesn't guarantee local discriminators are unique or non-zero

Open
#865 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bfd Bug mgd needs testing rust
Dominant language
Rust
Stars
94
Forks
6
Avg merge
1d 8h
Merged PRs (30d)
12

Description

The following is an issue identified by AI when I asked it to do an adversarial code review of #795. This was pre-existing in the sync/pthread impl, which is why it was not addressed as part of the async swap-out.

Problem

PeerInfo::with_random_discriminator() currently uses an unrestricted
random u32.

This permits:

  • discriminator zero, which RFC 5880 forbids; and
  • collisions with another active local BFD session.

The chance of zero is small for an individual session, but collisions
grow according to the birthday bound as the number of active sessions
increases. More importantly, the required invariant is not represented
or enforced anywhere.

An outgoing session with discriminator zero will send packets that a
compliant peer must discard.

Required behavior

RFC 5880 requires bfd.LocalDiscr to be:

  • nonzero; and
  • unique across all BFD sessions on the system.

It recommends selecting a random value subject to those constraints.

Proposed direction

  • Represent a local discriminator as NonZeroU32.
  • Allocate discriminators at the daemon level, where all active sessions
    are visible.
  • Generate random candidates until one is both nonzero and unused.
  • Release the discriminator when the session has completely shut down.

The packet wire representation can remain u32, but outgoing packet
construction should make an invalid value unrepresentable.

Testing

  • Generated discriminators are never zero.
  • Discriminators are unique across a large collection of concurrently
    active test sessions.
  • A forced RNG collision causes retry rather than duplicate allocation.
  • Removing a session permits safe eventual reuse after shutdown.

References

  • RFC 5880 §6.3
  • RFC 5880 §6.8.1

Context

This behavior predated the async implementation and was retained by #795
and #796.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at PeerInfo::with_random_discriminator() and trace how the daemon creates and shuts down BFD sessions. Then inspect outgoing packet construction and the existing async and sync/pthread implementations. Done means nonzero, unique discriminators with retry-on-collision behavior, safe reuse after shutdown, and tests covering the listed cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.