uvol: update to 1.2, expose volume operations over ubus

[dangowrt]

📦 Package Details

Maintainer: @dangowrt

Description:
Expose uvol's volume operations over ubus via an rpcd exec plugin.

---

🧪 Run Testing Details

• OpenWrt Version: main
• OpenWrt Target/Subtarget: x86/64
• OpenWrt Device: QEMU VM

---

✅ Formalities

• I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

[openwrt-ai]

Reviewed 3 commits (ee595d7, a71ef0a, f8f948d); each message matches its diff, and the Makefile side looks right — PKG_RELEASE correctly stays at 1 for the 1.1 → 1.2 bump, the postinst follows the same unindented rpcd reload shape as utils/rpcd-mod-lxc and utils/rpcd-mod-wireguard, and size: 64 in the plugin signature is what makes rpcd pick a BLOBMSG_TYPE_INT64 policy entry rather than INT32, so volumes above 2 GiB pass through. CI is green on f8f948d.

One finding I would call merge-blocking: the volume name now crosses a trust boundary and the backends interpolate it into shell command lines unquoted — see the comment on the validation in uvol-rpcd. The remaining three are a design question about create and rpcd's 120 s SIGKILL, plus two nits.

---

Generated by Claude Code

[openwrt-ai]

Rejecting only / is not enough now that the volume name arrives over ubus. The backends interpolate vol_name straight into shell command lines:

• system(sprintf("ubimkvol /dev/%s -N \"uvol-%s-%s\" -s %d", ...)) at ubi.uc:152
• system(sprintf("/usr/sbin/mkfs.f2fs -f -l \"%s\" \"%s\"", vol_name, ...)) at lvm.uc:356
• the -S selector built at lvm.uc:85, which ends up in fs.popen(...) at lvm.uc:20

A name like x";reboot;" or x`cmd` therefore executes as root. Until now the only caller was a root-only CLI, so this was not a trust boundary; the ubus object makes it one.

Second, nothing stops name from being .meta: remove/down/resize would then hit the internal metadata volume that boot/meta_init manage. uvol_mount.register already refuses dot-names, but backend.remove() does not.

An allowlist covers both:

Suggested change

	let name = args.name;
	if (name != null && (type(name) != "string" || index(name, "/") >= 0))
	reply_code(22);
	let name = args.name;
	// names reach the backends inside shell command lines and lvm -S expressions,
	// so allow only characters that are safe there; no leading dot keeps the
	// internal '.meta' volume unreachable over ubus
	if (name != null && (type(name) != "string" ||
	!match(name, /^[A-Za-z0-9_-][A-Za-z0-9._-]*$/)))
	reply_code(22);

---

Generated by Claude Code

[openwrt-ai]

create runs mkfs.f2fs / mke2fs (or ubimkvol + ubiupdatevol -t) synchronously, and rpcd caps an exec plugin call at RPC_EXEC_DEFAULT_TIMEOUT (120 * 1000) — on expiry rpc_exec_timeout_cb() sends SIGKILL to the plugin process and replies UBUS_STATUS_TIMEOUT.

For a multi-gigabyte volume on slow flash that ceiling is reachable. When it fires, only the ucode process dies: the mkfs started via system() is orphaned and keeps writing, while the flock taken by lock_device() is released with the process — so a following create/remove proceeds concurrently with a still-running mkfs. The wp_/wo_ naming means the volume is at least reclaimable later, but the lock guarantee the commit message relies on ("taking the same locks as the CLI so both entry points stay serialised") does not survive the kill.

Is the intent that consumers keep long-running creates on the CLI, or should create become an async submit/poll pair (return immediately, report progress through status)? Worth a note either way.

---

Generated by Claude Code

[openwrt-ai]

nit: the method table omits device and size, which the CLI does expose (device volname / size volname) and which every backend implements (backend.device / backend.size in lvm.uc). list does not carry the device either — lvm_list() returns only name, mode and size.

Given the commit message says consumers such as uxc "must not exec the command line tool", how is such a consumer meant to locate a volume's block device after up — is it expected to rely purely on the blockd mount at /tmp/run/uvol/<name>? If so a sentence in the commit message would help; otherwise device looks like it belongs in the table. (write needs a data stream and clearly cannot move here — no comment on that one.)

---

Generated by Claude Code

[openwrt-ai]

nit: the error path here reports the wrong thing. lvm_free() / lvm_total() / lvm_align() return 2 when the VG is unavailable (lvm.uc:163-166), and reply_code(2) renders that through error_text as "no such volume" — a per-volume message for three device-wide queries. A dedicated reply reads better:

Suggested change

	if (type(res) == "int")
	reply_code(res);
	if (type(res) == "int")
	reply({ code: res, error: "backend unavailable" });

---

Generated by Claude Code

[dangowrt]

Fixed the two real findings, and two of the four points in the review don't hold up under closer inspection. Pushed as 166ccc04 (three commits on top of f8f948d5).

Command injection and .meta targeting: fixed. f79dda1b4 moves every backend call site that interpolated a name, device or path into ucode's array form for system(), which execvp()s directly with no shell involved, so there is no shell metacharacter for a crafted name to reach. The three sites that genuinely need a shell (the lvm() report capture, and the digest pipe) now shell-quote every argument instead. 63fe84a81 adds one name validator shared by both entry points, ^[A-Za-z0-9_][A-Za-z0-9._-]*$, applied before either the lock or the backend is reached; it also closes .meta being reachable by remove/down/resize, which the mount registration rejected but the backends themselves never did.

On severity: this is root-to-root today, not a new privilege boundary. The unsanitised inputs that reach a volume name are a CONTAINER_VOLUMES id in a package Makefile, uxc-stack templates, hand-written registrations, and direct CLI use, all author- or root-controlled, and the new ubus object is root-only with no ACL shipped. Fixed anyway as defence in depth, and because publishing this over ubus is exactly what invites an ACL grant later, at which point it would stop being root-to-root.

Two review comments were wrong. Both checked against source, not asserted:

• Timeout. RPC_EXEC_DEFAULT_TIMEOUT is a 120s compiled fallback, not what ships. package/system/rpcd/files/rpcd.config installs option timeout 30 as a conffile, so every stock image runs a 30s exec timeout, not 120.
• Flock on timeout. Half right: rpcd's SIGKILL does end the ucode worker, but it does not release the lock. ucode's fs.open() only sets O_CLOEXEC when the mode string explicitly includes 'e' (lib/fs.c), and uvol's lock helper opens with mode "a". uc_system()'s forked child goes straight to execvp() with nothing in between that would close inherited descriptors (lib.c). A flock is held by the open file description, not the process, so the orphaned mkfs/ubimkvol inherits and keeps the lock exactly as the parent held it. The "same locks as the CLI" claim in the original commit message holds; a killed worker leaves the lock in place until the orphan itself exits, and a following call blocks behind it rather than racing it.

free/total/align error reporting: fixed. These three now reply with their own "capacity unavailable" message (63fe84a81) instead of borrowing a per-volume string from the same table status/create use.

One thing deliberately left out: two pre-existing return <n> paths elsewhere in files/uvol (an unrecognised command, and no command at all) have the same top-level-return-doesn't-set-exit-status bug this series fixed for the new name check, both silently exiting 0 where the code clearly intends 22. Neither is security-relevant and neither is touched by this PR's diff, so I've left them for a follow-up rather than widening this one's scope.

[openwrt-ai]

Reviewed the three new commits (f79dda1b, 63fe84a8, 166ccc04); each message matches its diff, including the parts that go beyond the subject and say so — the -j move ahead of the lock in 63fe84a8 (which also fixes lock_volume("-j")) and the free/total/align error text. CI is green on 166ccc04.

Both of your corrections check out, and thanks for them. package/system/rpcd/files/rpcd.config in openwrt.git does ship option timeout 30 as a conffile, so my 120 was the compiled fallback and wrong for a stock image — though 30 s makes a large create more likely to be killed, not less, so the open question on that thread stands rather than goes away. I'll take the O_CLOEXEC/inherited-flock reasoning as read; it matches how flock is scoped to the open file description.

One finding on the new work: the validator closes the shell and the .meta holes, but . is still a metacharacter in the lvm -S ... lvname=~ expression the name is interpolated into, so a non-existent name can select a real neighbouring volume — details on lvs(). Plus one nit on the null-return in lvm(). Neither is a regression from this series; the first is the same trust boundary the series is hardening, so it seems worth settling here.

---

Generated by Claude Code

[openwrt-ai]

vol_name still lands raw inside an lvm regular expression here (lvname=~), and the new validator admits . — /^[A-Za-z0-9_][A-Za-z0-9._-]*$/ at common.uc:38 — which is a metacharacter in that expression. Shell-quoting the argument in lvm() keeps the shell out of it, but lvm's own selection parser still sees the wildcard.

So on a VG holding rw_axb, uvol remove a.b builds lvname=~^[rw][owp]_a.b$, lvs() returns the unrelated LV, and lvm("lvrename", vg_name, res[0].lv_name, ...) at lvm.uc:501 marks that volume for deletion — a name that does not exist deletes one that does. up/down/status mis-target the same way through lvm_updown()/lvm_status(), and lvs_incomplete() at lvm.uc:113 repeats the pattern, so create a.b can reclaim an unrelated incomplete volume of matching size.

The ubi backend is fine: it matches with wildcard() at ubi.uc:46, and the validator already excludes every fnmatch metacharacter. Only lvm's regex is left.

Two ways out, and it's your call which: escape the regex metacharacters where the expression is built (here and at line 113), or drop . from the second character class in name_valid — the allow_internal branch already returns early for .meta, the one internal name that needs a dot, and <algo>-<hexdigest> content-addressed names don't contain one. No suggestion block since the two fixes differ in what they mean for existing volume names.

---

Generated by Claude Code

[openwrt-ai]

nit: 166ccc04 only half-closes the null-return it describes. When this popen() fails, lvm() prints and return null at lvm.uc:39 — the ?.report?. guards now cover the five report readers, but the ~20 .retval readers of the non-JSON commands still dereference straight into it: lvchange_r.retval at lvm.uc:291, ret.retval after lvcreate at lvm.uc:344, and so on. Those commands never reach the json() parse, so a fork/pipe failure under memory pressure — plausible on a small device midway through a create that is also running mkfs — ends the script with the same reference error the commit set out to replace with an error code. Giving lvm() a { retval: -1 } fallback instead of null would cover both sides in one place.

---

Generated by Claude Code

[dangowrt]

Both fixed, pushed as 8b083c9d (two commits on top of 166ccc04).

LVM regex mis-targeting: fixed in 8d81d4f9, and it predates this series. git show 6350c7bc6:utils/uvol/files/lvm.uc has the identical raw interpolation into lvname=~^[rw][owp]_%s$, from the original ucode rewrite in 2022, so this ships in released 1.1 today, not something the ubus work introduced; the commit carries Fixes: 6350c7bc6 for that reason and to make it independently backportable.

On the fix itself: escaping, not narrowing the validator. Dots are permitted in volume names and uxc uses them for every data volume, so a name can silently match a different volume; a collision needs a specific pair of names and is unlikely in practice, but remove acts on the match. Dropping . from the permitted set was considered and rejected, since uxc composes every data volume as <container>.<volname> and that would make all of them unaddressable. . is escaped where the selection expression is built instead, which is the only character in the permitted set ([A-Za-z0-9._-]) lvm's parser treats as special.

Incomplete null-guard: fixed in 8b083c9d, also predating this series (same Fixes: tag, same reasoning: the unguarded .retval readers are original to 6350c7b). lvm() now returns { retval: -1 } instead of null on a failed invocation, which the ?.report?. guards from the previous round already handle gracefully (a missing .report key on a real object is still falsy) and which the ~20 unguarded .retval readers pick up directly, covering both call patterns from the one return path.

[openwrt-ai]

Reviewed the updated series (rebased onto master, now 2 commits). The only uvol change since my last review is in files/lvm.uc, folded into 9ebb068: lvname_quote() now escapes . at both -S interpolation sites (lvs, lvs_incomplete), and lvm() returns { retval: -1 } instead of null on failure. Both address the findings from the prior round — the regex mis-targeting and the unguarded .retval readers — and the escape set matches name_valid()'s permitted characters (only . is regex-special there). No new issues found.

---

Generated by Claude Code