From e19514415ecb0fbfd6fe7850f737f99b66f32ef4 Mon Sep 17 00:00:00 2001 From: "Michael C. Ferguson" Date: Thu, 3 Sep 2026 15:52:25 -0500 Subject: [PATCH] bluetoothd: wait for the daemon to exit before unmounting its storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit start-stop-daemon -K only sends SIGTERM and returns; it does not wait. Stock's script unmounts on the very next line, so the umount races a bluetoothd that still has /var/lib/bluetooth open -- it fails EBUSY and leaves the loop mount up over a possibly-dirty ext4 image. This is reachable from the OSD, not merely at shutdown. `renew` (Main:menu.cpp:7166, an ABI entry point per docs/abi-contract.md §7.6) runs stop -> umount -> `rm $BTIMG` -> start, so losing the race deletes the image file while it is still loop-mounted. Stock hedges with fixed sleeps -- sleep 2 in renew, sleep 1 in restart -- which is a guess, not a wait. The fix is upstream Buildroot's, from package/bluez5_utils/S40bluetoothd's stop(): poll `start-stop-daemon --stop --test` until the process is gone, then remove the pidfile. One change from upstream: ours is BOUNDED at 50 x 0.1s = 5s, then unmounts anyway. Upstream's loop is infinite, which on this board would hang an OSD menu item forever if bluetoothd ever ignored SIGTERM; giving up after 5s is no worse than stock's behaviour today and correct otherwise. This is the only deviation in the file. Every other line, and the whole start/stop/restart/reload/renew/hcireset verb set, remains stock's -- so abi-contract S2 (the S45bluetooth -> /bin/bluetoothd symlink shape, the 2 MiB ext4 loop mount) and §7.6 (hcireset/renew as Main_MiSTer entry points) are untouched. Confirmed our vendored copy still matches upstream byte-for-byte before patching, against a freshly fetched addon.tar. VERIFIED AGAINST STOCK'S OWN BINARIES, not just ours. Upstream MiSTer builds on a much older Buildroot, so its BusyBox is a different vintage and its start-stop-daemon/sleep cannot be assumed to behave like ours. Stock's /usr/bin/busybox, extracted from rootfs.tar.bz2 in MiSTer-devel/Linux_Image_creator_MiSTer, is v1.33.1 (2022-12-24); ours is 1.38.0. Run under qemu-arm, both: -K -t -q -p on a live pid exit 0 ...on a reaped pid exit 1 ...stderr on that probe "warning: killing process N: No such process" sleep 0.1 works, ~0.1s So -t is in the base option string (not behind CONFIG_FEATURE_START_STOP_DAEMON_FANCY) and fractional sleep is built into both: the same code is valid on stock, which matters because the intent is to hand it upstream. The patched script also parses under both `busybox sh -n`. The 2>/dev/null is load-bearing, on both, and is the part that reading the source gets wrong: BusyBox's pidfile reader does not check liveness before building found_procs, so on the final probe the dead pid reaches the bb_perror_msg branch (:391) rather than the silent !G.found_procs path (:381-385), and that message is not gated on -q. An earlier draft of the doc claimed the opposite from a source read; running both binaries disproved it. Without the redirect every stop/restart/renew would print a spurious warning. Loop behaviour measured: dead pid -> 0 iterations, 44ms; a daemon that exits 0.6s after SIGTERM -> 5 iterations, 0.61s; one that never exits -> 50 iterations, then proceeds. Docs: bluetooth-parity.md gains §3.1 with the cross-vintage table and the correction; its "differences from stock: none found" claim and init-parity's "byte-identical" claim are updated to name this one deviation. The intent is to remove the deviation by fixing stock. Note for whoever does: usr/bin/bluetoothd is not tracked as source upstream -- it ships inside addon.tar, a binary blob committed to Linux_Image_creator_MiSTer -- so the upstream route is a report against that repo, not an ordinary source PR. Nothing has been sent upstream. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014SBpBTbtBsFwjAgKCx5T9S --- .../rootfs-overlay/usr/bin/bluetoothd | 46 +++++++++++ docs/bluetooth-parity.md | 77 +++++++++++++++++-- docs/init-parity.md | 2 +- 3 files changed, 117 insertions(+), 8 deletions(-) diff --git a/board/mister/de10nano/rootfs-overlay/usr/bin/bluetoothd b/board/mister/de10nano/rootfs-overlay/usr/bin/bluetoothd index d640988c..7ee78f7b 100755 --- a/board/mister/de10nano/rootfs-overlay/usr/bin/bluetoothd +++ b/board/mister/de10nano/rootfs-overlay/usr/bin/bluetoothd @@ -43,6 +43,52 @@ stop() { else echo "FAIL" fi + # MiSTer deviation from stock (the ONLY one in this file): wait for + # bluetoothd to actually exit before unmounting its storage. + # + # start-stop-daemon -K only SIGTERMs and returns; it does not wait. Stock + # unmounts on the next line, so the umount races a daemon that still has + # /var/lib/bluetooth open -- it fails EBUSY and leaves the loop mount up + # with a possibly-dirty ext4 image. That is reachable from the OSD, not + # just at shutdown: `renew` (menu.cpp:7166) does stop -> umount -> + # `rm $BTIMG` -> start, so losing the race deletes the image file while it + # is still loop-mounted. Stock hedges with fixed sleeps -- `sleep 2` in + # renew, `sleep 1` in restart -- which is a guess, not a wait. + # + # This is upstream Buildroot's fix (package/bluez5_utils/S40bluetoothd's + # stop(), which polls `start-stop-daemon --stop --test` until the process + # is gone), with one change: it is BOUNDED. Upstream's loop is infinite, + # which on this board would hang an OSD menu item forever if bluetoothd + # ever ignored SIGTERM. 50 x 0.1s = 5s, then give up and unmount anyway -- + # no worse than stock's behaviour today, and correct in every other case. + # + # Verified by RUNNING the binaries, not by reading them. start-stop-daemon + # is BusyBox's applet (output/target/sbin/start-stop-daemon -> ../bin/ + # busybox); `-t` probes liveness with kill(pid, 0) rather than signalling + # (debianutils/start_stop_daemon.c:388) and is in the base option string, + # not behind CONFIG_FEATURE_START_STOP_DAEMON_FANCY. Measured: exit 0 while + # the pid is alive, exit 1 once it is gone. + # + # The 2>/dev/null is LOAD-BEARING, and this is the part that is easy to get + # wrong by reading the source: on the final probe -- the one that ends the + # loop -- start-stop-daemon writes + # start-stop-daemon: warning: killing process N: No such process + # to stderr. It is not gated on -q, because busybox's pidfile reader does + # not check liveness before building found_procs, so the dead pid reaches + # the bb_perror_msg branch (:391) rather than the silent !G.found_procs + # path (:381-385). Confirmed by running BOTH vintages against a reaped pid: + # stock's BusyBox 1.33.1 and our 1.38.0 print it identically. Without the + # redirect every stop/restart/renew would emit a spurious warning. + # + # Fractional sleep works on both as well (CONFIG_FEATURE_FANCY_SLEEP), so + # this same code is valid on stock -- which matters, because the intent is + # to hand it upstream. + i=0 + while [ $i -lt 50 ] && start-stop-daemon -K -t -q -p "$PIDFILE" 2>/dev/null; do + sleep 0.1 + i=$((i + 1)) + done + rm -f "$PIDFILE" umount $MNTPATH return "$status" } diff --git a/docs/bluetooth-parity.md b/docs/bluetooth-parity.md index 9a095017..434e900a 100644 --- a/docs/bluetooth-parity.md +++ b/docs/bluetooth-parity.md @@ -40,11 +40,12 @@ so `/bin` is itself a symlink to `usr/bin` in the built image, making `etc/init.d/S45bluetooth -> /bin/bluetoothd` resolve to our `usr/bin/bluetoothd`): -- `board/mister/de10nano/rootfs-overlay/usr/bin/bluetoothd` — **diffed - byte-for-byte against the stock verbatim capture during this audit: 0 - differences.** Same `start`/`stop`/`restart`/`renew`/`reload`/`hcireset` - shape, same `BLUETOOTHD_ARGS="-n -E -C"`, same ext4-image persistence - idiom (see §3). +- `board/mister/de10nano/rootfs-overlay/usr/bin/bluetoothd` — **byte-for-byte + against the stock verbatim capture when this audit ran: 0 differences.** + Same `start`/`stop`/`restart`/`renew`/`reload`/`hcireset` shape, same + `BLUETOOTHD_ARGS="-n -E -C"`, same ext4-image persistence idiom (see §3). + **One deviation has since been added, in `stop()` only — see §3.1.** Every + other line, and the whole verb set, remains stock's. - `board/mister/de10nano/rootfs-overlay/etc/init.d/S45bluetooth` — symlink to `/bin/bluetoothd`, matching stock's shape exactly. - `board/mister/de10nano/rootfs-overlay/etc/init.d/S40bluetoothd` — @@ -93,8 +94,70 @@ host-key mechanism from (`/media/fat/linux/ssh.ext4` -> `/etc/ssh_keys`) — Bluetooth's is the original, SSH's the derived design. Differences from stock: **none found.** The script is a byte-identical -reproduction (§2), so the persistence path, mount options, image size, and -`renew` semantics all match stock exactly. +reproduction (§2) apart from the `stop()` wait described next, so the +persistence path, mount options, image size, and `renew` semantics all match +stock exactly. + +### 3.1 The one deviation: `stop()` waits for the daemon to exit + +`start-stop-daemon -K` only sends SIGTERM and returns; it does not wait. Stock +unmounts on the very next line, so the `umount` races a `bluetoothd` that still +has `/var/lib/bluetooth` open — it fails `EBUSY` and leaves the loop mount up +over a possibly-dirty ext4 image. + +This is **reachable from the OSD, not merely at shutdown**. `renew` +(`Main:menu.cpp:7166`, an ABI entry point per `docs/abi-contract.md` §7.6) runs +`stop` → `umount` → `rm $BTIMG` → `start`, so losing the race deletes the image +file while it is still loop-mounted. Stock hedges with fixed sleeps — `sleep 2` +in `renew`, `sleep 1` in `restart` — which is a guess, not a wait. + +The fix is upstream Buildroot's, from `package/bluez5_utils/S40bluetoothd`'s +`stop()`: poll `start-stop-daemon --stop --test` until the process is gone, then +remove the pidfile. One change from upstream — **ours is bounded** (50 × 0.1 s = +5 s, then unmount anyway). Upstream's loop is infinite, which on this board would +hang an OSD menu item forever if `bluetoothd` ever ignored SIGTERM; giving up +after 5 s is no worse than stock's behaviour today and correct in every other +case. + +Verified by **running** the binaries under `qemu-arm`, not by reading them — +including stock's, since upstream MiSTer builds on a much older Buildroot and +its BusyBox is a different vintage. Stock's `/usr/bin/busybox`, extracted from +`rootfs.tar.bz2` in `MiSTer-devel/Linux_Image_creator_MiSTer`, is **v1.33.1 +(2022-12-24)**; ours is 1.38.0. + +| | stock 1.33.1 | ours 1.38.0 | +|---|---|---| +| `start-stop-daemon -K -t -q -p` on a live pid | exit 0 | exit 0 | +| …on a reaped pid | exit 1 | exit 1 | +| …stderr on the reaped-pid probe | `warning: killing process N: No such process` | identical | +| `sleep 0.1` | works, ~0.1 s | works, ~0.1 s | + +Two things follow. `-t` is in the base option string, not behind +`CONFIG_FEATURE_START_STOP_DAEMON_FANCY`, and fractional `sleep` is built into +both — so **this same code is valid on stock**, which matters because the intent +is to hand it upstream. + +And the `2>/dev/null` is **load-bearing**, on both. This is the part that is +easy to get wrong by reading the source: on the final probe — the one that ends +the loop — `start-stop-daemon` writes `warning: killing process N: No such +process` to stderr, *not* gated on `-q`, because BusyBox's pidfile reader does +not check liveness before building `found_procs`, so the dead pid reaches the +`bb_perror_msg` branch (`:391`) rather than the silent `!G.found_procs` path +(`:381-385`). An earlier draft of this section claimed the opposite from a +source read; running both binaries against a reaped pid disproved it. Without +the redirect, every `stop`/`restart`/`renew` would emit a spurious warning. + +Loop behaviour, measured against our BusyBox: dead pid in the pidfile → 0 +iterations, 44 ms; a daemon that exits 0.6 s after SIGTERM → 5 iterations, +0.61 s; a daemon that never exits → 50 iterations, then proceeds. + +**This is a divergence from stock and the intent is to remove it by fixing +stock.** `usr/bin/bluetoothd` is not tracked as source upstream — it ships +inside `addon.tar`, a binary blob committed to +`MiSTer-devel/Linux_Image_creator_MiSTer` (`docs/verification/ +stock-reconciliation/addon-report.txt:33`, `usr/bin/bluetoothd EXACT`) — so the +upstream route is a report against that repo rather than an ordinary source PR. +Until then this row is the record that we are one `stop()` ahead of stock. One asymmetry worth naming explicitly (not a defect, a property of the mechanism): unlike SSH's `S50sshd`, this script has **no ephemeral tmpfs diff --git a/docs/init-parity.md b/docs/init-parity.md index fc702bca..ff4ee613 100644 --- a/docs/init-parity.md +++ b/docs/init-parity.md @@ -65,7 +65,7 @@ directly or by an equivalent the package set already installs. | `S30dbus` | **adapted** (filename) | dbus's own package script `S30dbus-daemon` does the same job (`dbus-uuidgen --ensure`, `mkdir -p /run/dbus /tmp/dbus`, `dbus-daemon --system`). Same reasoning as `S10udev` — not duplicated, to avoid a second `dbus-daemon --system` racing for the bus name. Not overlaid. | | `S40network` | **identical** | Byte-for-byte identical to stock (`diff` exit 0) — `ifup -a` / `ifdown -a` via ifupdown. Not overlaid. | | `S41dhcpcd` | **identical** | Filename matches stock exactly. Content is functionally identical (same start/stop/reload logic); the only difference is `PIDFILE=/var/run/dhcpcd/pid` vs. stock's `/var/run/dhcpcd.pid`, which reflects this newer dhcpcd's own pidfile convention, not a P2.3 decision — reverting to stock's path would risk it not matching what this dhcpcd binary actually writes. Not overlaid. | -| `S45bluetooth` | **adapted** (mechanism reproduced, package default neutralized) | Stock's real file is a **symlink** to `/bin/bluetoothd`, which does the ext4-image persistence trick for `/var/lib/bluetooth` (BT pairing keys) that ADR 0015 explicitly mirrors for SSH host keys. Reproduced **byte-identical** (`diff` exit 0) at `bin/bluetoothd`, with `etc/init.d/S45bluetooth` a symlink to it — exactly stock's shape. **Problem found and fixed:** `BR2_PACKAGE_BLUEZ5_UTILS` installs its own `S40bluetoothd`, which starts `bluetoothd` directly with **no** persistence step — on our read-only `/`, `/var/lib/bluetooth` (not in fstab, so not tmpfs) would be unwritable, and running it would race the real `S45bluetooth` over the D-Bus name and the HCI socket. `etc/init.d/S40bluetoothd` is overlaid to a documented no-op stub so bluetoothd starts exactly once, correctly. | +| `S45bluetooth` | **adapted** (mechanism reproduced, package default neutralized) | Stock's real file is a **symlink** to `/bin/bluetoothd`, which does the ext4-image persistence trick for `/var/lib/bluetooth` (BT pairing keys) that ADR 0015 explicitly mirrors for SSH host keys. Reproduced from stock at `bin/bluetoothd`, with `etc/init.d/S45bluetooth` a symlink to it — exactly stock's shape. **One deliberate deviation** (`docs/bluetooth-parity.md` §3.1): `stop()` now waits for `bluetoothd` to actually exit before unmounting `/var/lib/bluetooth`, because `start-stop-daemon -K` does not wait and stock's `umount` on the next line races a daemon that still holds the mount. Reachable from the OSD, not just at shutdown — `renew` does `stop` → `umount` → `rm $BTIMG` → `start`. The wait is upstream Buildroot's `bluez5_utils/S40bluetoothd` idiom, bounded at 5 s so it cannot hang a menu item. Every other line is stock's. **Problem found and fixed:** `BR2_PACKAGE_BLUEZ5_UTILS` installs its own `S40bluetoothd`, which starts `bluetoothd` directly with **no** persistence step — on our read-only `/`, `/var/lib/bluetooth` (not in fstab, so not tmpfs) would be unwritable, and running it would race the real `S45bluetooth` over the D-Bus name and the HCI socket. `etc/init.d/S40bluetoothd` is overlaid to a documented no-op stub so bluetoothd starts exactly once, correctly. | | `S49ntp` | **identical** (overlaid to fix a real bug) | Byte-identical to stock's script (`ntpd -g`, runs as root). **Problem found and fixed:** the package's own default `S49ntp` runs `ntpd -u ntp:ntp -g` — dropping privileges to an `ntp` user that **does not exist** in this build's `/etc/passwd` (verified: `grep '^ntp:' output/target/etc/passwd` → no match). Left as the package default, `ntpd` would fail to start on every boot, silently breaking time sync forever. Reverted to stock's root-run form via the overlay. | | `S50proftpd` | **identical** | Byte-for-byte identical to stock (`diff` exit 0). Not overlaid. | | `S50sshd` | **adapted** (ADR 0015) | Stock's simple shape (`ssh-keygen -A`; bare `/usr/sbin/sshd`; `touch /var/lock/sshd`) is kept, but `ssh-keygen -A` is replaced with the ADR 0015 per-device mechanism: create/mount `/media/fat/linux/ssh.ext4` at `/etc/ssh_keys` (mirrors `bin/bluetoothd`'s own ext4-image idiom almost line for line), then generate the three key types individually into it if missing. See "SSH host keys" below for the full mechanism and why. |