Repository navigation
usb: a writable FAT32 user disk as a third LUN - #92
Conversation
Upstream asks for this five times (sipeed/NanoKVM#829, #745, #574, #356, #104): corporate USB policies and endpoint security refuse a device called "sipeed NanoKVM", and the identity was hard-coded in S03usbdev and S03usbhid. The USB settings page gets a USB identity section: the factory identity, a "generic composite device" preset (the factory IDs with the names Generic / USB Composite Device), or the four fields by hand. No preset carries another vendor's ID; a custom ID is the user's own, and the page says what it breaks: the serial console helper's udev rule on Linux hosts names the IDs. The serial number stays the chip UID (that rule is bound to it), bcdDevice stays the mode flag. The identity is part of the composition. The scripts read /boot/usb.vid, usb.pid (as before), usb.manufacturer and usb.product (new) at every start. The recovery init reads the manufacturer and product files already; it takes the IDs from its own usb.idVendor/usb.idProduct, and it lives in the checksummed initramfs, so aligning it waits for a platform build (the docs say so). The four files join the apply transaction: snapshotted, written by content (absent for the factory value), restored on failure, and the identity the gadget presents is read back from configfs in the verification. It is in the revision, so a stale tab cannot change it unseen. PUT takes an optional identity object; a client that sends none keeps the one in place. Tests: ID and string validation, defaults and the files an identity needs, reading /boot (a damaged file is the factory value), verification against a fake configfs, the transaction writing, keeping on rollback and removing the files; the web presets, matching and validation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Upstream asks for a small writable drive next to the virtual CD for BIOS update files and profiles (sipeed/NanoKVM#426, #477); the virtual storage had one LUN for images and, with USB serial, the read-only helper LUN. The user disk is a FAT32 image on the data partition (/data/.nanokvm/user-disk.img, label NKVMDATA), created from the USB settings page at 64 MiB to 4 GiB: a temporary file sized with truncate, BusyBox mkfs.vfat (-F 32, one sector per cluster up to 256 MiB so FAT32 keeps its 65525 clusters), then renamed into place, so a failed format leaves the old image. Turning it on is a composition change: /boot/usb.userdisk names the image, S03usbdev adds lun.2 before linking the function (removable, writable, "NanoKVM User Disk"), the marker joins the apply transaction with the identity files, the verification wants lun.2 to match, and it is in the revision. It needs the virtual disk function and costs no endpoints. Formatting anew and deleting are refused while it is on. The Images menu skips hidden directories under /data, and imageFile refuses paths through them, so the image cannot be mounted on lun.0 or deleted from there. Tests: creating under a temporary name and the size bounds, the old image kept when formatting fails, the state from the marker and the LUN, a real mkfs.vfat and mdir where available (CI has dosfstools and mtools); the marker written and removed by the transaction, the validation, the revision, the lun.2 check in verification; the web composition helpers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
dormancygrace
left a comment
dormancygrace
left a comment
There was a problem hiding this comment.
Review of fffdc00: found the issue below. Please address it before merging. Reviewed source changes and available CI; no device changes were made.
| } | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| if userDiskEnabled() || userDiskAttached() { |
There was a problem hiding this comment.
[P1] Serialize image replacement/removal with USB composition changes
This check is protected only by storage.Service.mu, while SetUSBComposition uses virtualDeviceMutex. With an existing disk turned off, start reformatting it in tab A and enable it in tab B while createUserDisk is building the temporary image. B attaches the existing inode and succeeds; A then renames the new image over it. The host keeps writing to the now-unlinked old image, so its writes disappear on detach even though both requests can report success. The same check/use race allows deletion after a concurrent attach. This is worse than the missing-file/rollback case mentioned in the PR description. I reproduced the format case with a temporary-directory Go test: the open LUN handle remained the old 64 MiB inode while the canonical image became 128 MiB and enabled/attached both remained true. Use shared exclusion for create/delete and composition attach/detach across the full operation; a second unlocked existence check is insufficient.
There was a problem hiding this comment.
Agreed, and the delete path had the same gap. Fixed in d6db8aa.
A dedicated lock (storage.userDiskMu) now spans the whole operation on both sides:
CreateUserDiskandDeleteUserDiskhold it while they check "enabled or attached" and replace or remove the file.applyLiveUSBCompositiontakes it whenever the current or the new composition has the user disk, for the whole apply, and re-checks that the image exists under it. It usesTryLockbecause it runs under the HID lock, which must not stall the keyboard and mouse for a multi-GiB format. A refused apply answers -6 ("being created or removed"), a vanished image -4, and the USB page says so.
Tests, under -race: your interleaving (a format in progress refuses the composition lock; a re-format started while the composition holds it waits, then refuses with the disk enabled and the attached inode unchanged), and the apply refusing both attach and detach while the image is being made.
There was a problem hiding this comment.
Rechecked d6db8aa: fixed. Create/delete and live USB composition changes now share exclusion over the full operation, including the file-existence check and apply/rollback. Ran go test -race -tags teststub,nomsgpack ./service/storage ./service/vm successfully, including the format-vs-attach regressions. This finding is resolved.
Review: create/delete checked "enabled or attached" under the storage
service's mutex only, while a composition change runs under the HID lock.
Re-formatting with the disk off in one tab and turning it on in another
could attach the old inode and then rename the new image over it: the
host kept writing to an unlinked file, both requests reported success.
Deletion had the same check/use gap.
A dedicated lock (storage.userDiskMu) now covers the whole operation on
both sides. Create and delete hold it while they check the state and
replace or remove the file. applyLiveUSBComposition takes it with TryLock
whenever the current or the new composition has the user disk, for the
whole apply, and re-checks that the image exists under it; it does not
wait because it runs under the HID lock, which must not stall keyboard
and mouse for a multi-GiB format. A refused apply answers -6 ("being
created or removed"), a vanished image -4; the USB page says so.
Tests: the review's interleaving (a format in progress refuses the
composition lock; a re-format started while the composition holds it
waits, then refuses with the disk enabled and the attached inode
unchanged) and the apply refusing attach and detach while the image is
being made, under -race.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Upstream asks for a small writable drive next to the virtual CD, for BIOS update files and profiles (sipeed/NanoKVM#426, #477). The virtual storage had one LUN for images and, with USB serial, the read-only helper LUN. Stacked on #82 (
claude/usb-identity): both editS03usbdev,usbCompositionFlagsand the USB settings page.What the user gets
Settings → USB, a User disk card: create the image at 64 MiB … 4 GiB (FAT32, label
NKVMDATA), turn it on, and the computer sees a third removable drive "NanoKVM User Disk" next to the virtual CD/disk. Format anew or delete while it is off. The card says when the drive needs the virtual disk function, and that turning it on or off reconnects the USB devices.How
server/service/storage/userdisk.go: the image lives in/data/.nanokvm/user-disk.img. Create = a temporary file sized withTruncate, BusyBoxmkfs.vfat -F 32 -s 1|8 -n NKVMDATA(one 512-byte sector per cluster up to 256 MiB, so FAT32 keeps its 65525 clusters; dosfstools on the build hosts needs the same flags), rename into place; a failed format leaves the old image.GET /api/storage/user-disk,POST …/create {sizeMiB},POST …/delete; create and delete are refused while the disk is on or attached, because replacing a file under a live LUN leaves the host with a stale view./data, andimageFilerefuses paths through them, so the image is never offered forlun.0nor deletable from there.PUT /api/vm/device/virtualtakes an optionaluserDisk(omitted keeps it; on needs the disk function and an existing image)./boot/usb.userdisknames the image and joins the apply transaction next to the identity files (written by content, absent when off, restored on failure); it is in therevision;verifyUSBCompositionAtwantslun.2to exist exactly when the user disk is on.S03usbdev: while the marker names an existing file,lun.2is created before the function is linked (the kernel refuses a LUN change on a linked function) and set up after: removable, writable, "NanoKVM User Disk". A LUN kept from the previous start still has its file set, andro/cdromare refused (EBUSY) while it does, so both the helper LUN and the user disk eject first; this also removes the "write error: Resource busy" line the helper LUN printed on every restart.Checked
lun.2is not "attached"), a realmkfs.vfat+mdirlisting the label where available (CI installs dosfstools and mtools); the marker written and removed by the transaction, the validation (user disk without the disk function), the revision, thelun.2check both ways.go vet,go test -tags teststub ./service/vm/ ./service/storage/,go build ./....tools/test-usb-profiles.sh,tools/test-usb-rebind.sh, shellcheck.tsc --noEmit, eslint, prettier, the composition and i18n tests.truncate -s 64Mon the exFAT data partition allocates at once; BusyBoxmkfs.vfat -F 32 -s 1lays out FAT32 with 131072 clusters; marker written, newS03usbdevstop+start→lun.0 lun.1 lun.2,lun.2ro=0 removable=1 inquiry "NanoKVM User Disk", helper LUN untouched, no "Resource busy"; on the Windows hostGet-Diskshows "NanoKVM User Disk" 64 MiB online andGet-VolumetheNKVMDATAFAT32 volume asE:; a file written from Windows, the disk turned off, the image loop-mounted on the NanoKVM: the file is there with its content. The stand was left as it was (console composition, helper LUN, no image).Notes for deployment and review
S03usbdevaccepts only/data/.nanokvm/user-disk.imgin the marker (literal compare), so an edited/boot/usb.userdiskcannot put any other file on the card on a writable USB drive; the Go side fences the same path.lun.2, and the old script does not remove one either (seen on the stand: turning the disk off with main's script leftlun.2behind until the gadget was rebuilt). On a device running this PR the off path removes it.SetUSBCompositionchecks that the image exists before taking the composition lock, without the storage service's mutex: adeleteracing an enable can leave a marker that names a missing file. The script then skips the LUN at boot and the verification fails the apply and rolls it back; not engineered around.mkfs.vfat -F 32 -s Non the device: 64 and 256 MiB → 1 sector per cluster (131072 / 524288 clusters), 512 MiB and 1 GiB → 8 (131072 / 262144), allFAT32; the same flags give dosfstools on the build hosts the same layout.Not covered
🤖 Generated with Claude Code