oxidecomputer / oxidecomputer/maghemite

BFD session del/add may race to update RIB nexthop shutdown status

Open
#858 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.

Title: BFD: prevent stale RIB writes from superseded sessions

Problem

BFD session teardown does not wait for all RIB work associated with the
session to finish.

Session::drop() aborts the session's async tasks, but RibTask performs
RDB updates through tokio::task::spawn_blocking(). Once a blocking task
has started, aborting the async task waiting for it does not cancel the
blocking operation.

The shutdown handle returned when removing a peer waits only for the UDP
listener. It does not wait for the session's RIB task or an in-flight
blocking RDB update.

Failure scenario

  1. Session A begins a RIB update for peer P.
  2. A is removed.
  3. The async RIB task is aborted, but its blocking update remains live.
  4. Session B is created for the same peer.
  5. B publishes its current state to the RIB.
  6. A's stale update completes afterward and becomes the final RIB state.

For example, session B may be Down while a stale shutdown = false write
from A leaves routes through the peer enabled. Because B already believes
it synchronized shutdown = true, no further write is guaranteed until
another state transition occurs.

The inverse is also possible: a stale shutdown = true write can suppress
routes belonging to a new Up session.

Impact

The forwarding state can permanently disagree with the currently visible
BFD session until another BFD transition happens.

Proposed direction

Give peer removal an awaitable, session-level shutdown operation that:

  1. stops and awaits the driver and egress tasks;
  2. closes the state channel;
  3. allows any current blocking RIB operation to complete;
  4. awaits the RIB task;
  5. awaits the listener task.

The same peer must not be re-added until shutdown of the prior generation
has completed. Alternatively, serialize RIB updates through a
generation-aware per-peer worker that rejects writes from superseded
sessions.

Testing

Add a fake RIB sink with barriers that can:

  1. block an old session's write;
  2. remove and recreate the session;
  3. allow the new session's write to complete;
  4. release the old write;
  5. verify that the old generation cannot overwrite the new state.

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 by tracing Session::drop, RibTask, spawn_blocking, and the peer-removal shutdown handle to identify which tasks and RIB operations are awaited. Build the fake RIB sink with barriers described in the issue, then remove and recreate a peer while an old write is blocked. Done means shutdown completes before re-addition, or superseded generations cannot overwrite the new RIB state.

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.