From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id 60555EDE6; Sun, 14 Dec 2025 10:49:53 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 5B215EDDF; Sun, 14 Dec 2025 10:49:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-0.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_MISSING,SPF_HELO_PASS autolearn=unavailable autolearn_force=no version=4.0.1 Received: from fout-a7-smtp.messagingengine.com (fout-a7-smtp.messagingengine.com [103.168.172.150]) by atuin.qyliss.net (Postfix) with ESMTPS id B7CF2EDDD for ; Sun, 14 Dec 2025 10:49:49 +0000 (UTC) Received: from phl-compute-03.internal (phl-compute-03.internal [10.202.2.43]) by mailfout.phl.internal (Postfix) with ESMTP id C1611EC05DE; Sun, 14 Dec 2025 05:49:46 -0500 (EST) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-03.internal (MEProxy); Sun, 14 Dec 2025 05:49:46 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h=cc :cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm3; t=1765709386; x=1765795786; bh=ryF5+ftwnr Tcp2W0oFqG+uAKSTlSbxzeFs0WtJKp0ik=; b=cwsjGYZnu1RmvpWdneBcDdE1bn B5WBBIOKB9xF6DQCDB7BHxOOTHa1E4V5GFOjMH3uX3KXua2sE0thRwEDlq7km9MH qisDjhzyVVvbZSGKBP9DcO3s5ywbfe9WObaOvRxFOkEhvqh8M/ghy81aHLjn2yIb /VW+WatvzAeudDSJwE8wKXkVGgDnTQchP94YusaW8Uzqb2KwcIy//iZpM98we/cL pc2sRcFqnEqmADr8x5DoEozSad+9PWKVbbV1hKehENLGFujkDfFS7Pa9IadSfRcE 7kcDy3TnQkaYXViXF8mhIg+bSJuu41kd0BYzND9ZsgXq+fWltFkfA9r/gaIw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t= 1765709386; x=1765795786; bh=ryF5+ftwnrTcp2W0oFqG+uAKSTlSbxzeFs0 WtJKp0ik=; b=Bp3XQGr9DepwLuSduje2wJiJw2dvnIoRtpLHwJs3vQysJ3hEi+R 4V9HF9wYvwOzFSBXhtDxnEJZrCgv0oBGIw62S5WzhaURlyuqtnqQcw63gG/YHVGq 7CgxnqQ+w+07dyIv2Ba8BH+/5foOy6y9R480gJyg7N+tLr2k/Ntdy0q0zoqEdJMu siXiYNisec4+ZQZkTRZjQV/0AQHnPxfIcUNcxTqa9XOGMkCwCLCgtK9NNSADyeOh sjZjxv+9mA/bDwfcnHUVck2IidMhDrW0e01TWFwLSF3/dZ1ZTN+0cYiriEAW+HIe rjNGf7YDhSvFT3Dp9wU0lLTEBXLf6mATUOA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefgedrtddtgdeffeejfecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpuffrtefokffrpgfnqfghnecuuegr ihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenucfjug hrpefhvfevufgjfhffkfggtgesghdtreertddttdenucfhrhhomheptehlhihsshgrucft ohhsshcuoehhihesrghlhihsshgrrdhisheqnecuggftrfgrthhtvghrnhepieduffeuie elgfetgfdttddtkeekheekgfehkedufeevteegfeeiffetvdetueevnecuvehluhhsthgv rhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhephhhisegrlhihshhsrgdrih hspdhnsggprhgtphhtthhopedvpdhmohguvgepshhmthhpohhuthdprhgtphhtthhopegu vghmihhosggvnhhouhhrsehgmhgrihhlrdgtohhmpdhrtghpthhtohepuggvvhgvlhessh hpvggtthhruhhmqdhoshdrohhrgh X-ME-Proxy: Feedback-ID: i12284293:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 14 Dec 2025 05:49:46 -0500 (EST) Received: by fw12.qyliss.net (Postfix, from userid 1000) id 20A9E7BF8B71; Sun, 14 Dec 2025 11:49:40 +0100 (CET) From: Alyssa Ross To: Demi Marie Obenour Subject: Re: [PATCH v2 5/7] tools/vm-set-persist.c: init In-Reply-To: <219eac4a-133a-4d8c-b289-2da42274ce6e@gmail.com> References: <20251214014229.775825-2-hi@alyssa.is> <20251214014229.775825-10-hi@alyssa.is> <219eac4a-133a-4d8c-b289-2da42274ce6e@gmail.com> Date: Sun, 14 Dec 2025 11:49:38 +0100 Message-ID: <87a4zl2v99.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Message-ID-Hash: 2ATAZ643DV7XHPOEAUBCOYTJ237ZIKM3 X-Message-ID-Hash: 2ATAZ643DV7XHPOEAUBCOYTJ237ZIKM3 X-MailFrom: hi@alyssa.is X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: devel@spectrum-os.org X-Mailman-Version: 3.3.9 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Demi Marie Obenour writes: > On 12/13/25 20:42, Alyssa Ross wrote: >> diff --git a/tools/vm-set-persist.c b/tools/vm-set-persist.c >> new file mode 100644 >> index 00000000..ac759504 >> --- /dev/null >> +++ b/tools/vm-set-persist.c >> @@ -0,0 +1,179 @@ >> +// SPDX-FileCopyrightText: 2025 Alyssa Ross >> +// SPDX-License-Identifier: EUPL-1.2+ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +// No until musl declares stx_mnt_id. >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +// Including trailing NUL bytes. >> +static const int MNT_ROOT_MAX_LEN =3D 43; >> +static const int SOURCE_MAX_LEN =3D 28; >> + >> +static void set_mount_namespace(const char vm_id[static 1]) >> +{ >> + char ns_path[28]; >> + int r =3D snprintf(ns_path, sizeof ns_path, >> + "/run/vm/by-id/%s/ns/mnt", vm_id); >> + >> + if (r =3D=3D -1) >> + err(EXIT_FAILURE, "snprintf"); >> + if ((size_t)r >=3D sizeof ns_path) >> + errx(EXIT_FAILURE, "VM ID unexpectedly long"); >> + >> + if ((r =3D open(ns_path, O_RDONLY | O_CLOEXEC)) =3D=3D -1) >> + err(EXIT_FAILURE, "open"); >> + if (setns(r, CLONE_NEWNS) =3D=3D -1) >> + err(EXIT_FAILURE, "setns"); >> + close(r); >> +} >> + >> +static void do_statx(const char path[static 1], >> + mode_t mode[static 1], uint64_t mnt_id[static 1]) >> +{ >> + struct statx stx; >> + >> + if (syscall(__NR_statx, AT_FDCWD, path, AT_SYMLINK_NOFOLLOW, >> + STATX_MODE | STATX_MNT_ID_UNIQUE, &stx) =3D=3D -1) >> + err(EXIT_FAILURE, "statx"); > > Here (and below), I recommend using wrapper functions around syscalls > instead of raw syscalls. This makes it much easier to check the > types used against the manpage. I doubt this code is going to change much before the next musl release (which will bring wrappers for most of these) anyway. > This is also missing casts to long. If casts to long are necessary every time you call syscall() in C, fixing that is going to need to start with musl, which doesn't seem to think that's necessary. >> + if (!(stx.stx_attributes & STATX_ATTR_MOUNT_ROOT)) { >> + if (stx.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT) >> + errx(EXIT_FAILURE, >> + "VM disk-backed directory not mounted"); >> + >> + errx(EXIT_FAILURE, "statx didn't return STATX_ATTR_MOUNT_ROOT"); >> + } >> + >> + if (!(stx.stx_mask & STATX_MNT_ID_UNIQUE)) >> + errx(EXIT_FAILURE, "statx didn't return STATX_MNT_ID_UNIQUE"); >> + if (!(stx.stx_mask & STATX_MODE)) >> + errx(EXIT_FAILURE, "statx didn't return STATX_MODE"); >> + >> + *mode =3D stx.stx_mode; >> + *mnt_id =3D stx.stx_mnt_id; >> +} >> + >> +static int do_mount(const char source[static 1]) >> +{ >> + int mnt, fs =3D syscall(__NR_fsopen, "btrfs", FSOPEN_CLOEXEC); >> + if (fs =3D=3D -1) >> + err(EXIT_FAILURE, "fsopen"); >> + if (syscall(__NR_fsconfig, fs, FSCONFIG_SET_STRING, >> + "source", source, 0) =3D=3D -1) > > This might be too paranoid, but if possible I would use openat2() > with RESOLVE_NO_SYMLINKS to get a file descriptor to the source, > and then use /proc/thread-self/fd/FD_NUMBER here. If the kernel gives us a symlink for some reason we probably want to follow it. Swapping out the path would require root. >> + err(EXIT_FAILURE, "FSCONFIG_SET_STRING source"); >> + if (syscall(__NR_fsconfig, fs, FSCONFIG_SET_FLAG, >> + "rw", nullptr, 0) =3D=3D -1) >> + err(EXIT_FAILURE, "FSCONFIG_SET_FLAG rw"); >> + if (syscall(__NR_fsconfig, fs, FSCONFIG_CMD_CREATE, >> + nullptr, nullptr, 0) =3D=3D -1) >> + err(EXIT_FAILURE, "FSCONFIG_CMD_CREATE"); >> + if ((mnt =3D syscall(__NR_fsmount, fs, FSMOUNT_CLOEXEC, >> + MOUNT_ATTR_NOSUID | MOUNT_ATTR_NOSYMFOLLOW | >> + MOUNT_ATTR_NOEXEC | MOUNT_ATTR_NODEV)) =3D=3D -1) >> + err(EXIT_FAILURE, "fsmount"); >> + close(fs); >> + return mnt; >> +} >> + >> +static void do_statmount(uint64_t mnt_id, >> + char mnt_root[static MNT_ROOT_MAX_LEN], >> + char source[static SOURCE_MAX_LEN]) >> +{ >> + int r; >> + char sm_buf[sizeof(struct statmount) + >> + MNT_ROOT_MAX_LEN + SOURCE_MAX_LEN]; >> + struct statmount *sm =3D (struct statmount *)sm_buf; >> + struct mnt_id_req req =3D { >> + .size =3D sizeof req, >> + .mnt_id =3D mnt_id, >> + .param =3D STATMOUNT_MNT_ROOT | STATMOUNT_SB_SOURCE, >> + }; >> + >> + if (syscall(__NR_statmount, &req, sm, sizeof sm_buf, 0) =3D=3D -1) >> + err(EXIT_FAILURE, "statmount"); >> + >> + r =3D snprintf(mnt_root, MNT_ROOT_MAX_LEN, "%s", sm->str + sm->mnt_roo= t); >> + if (r =3D=3D -1) >> + err(EXIT_FAILURE, "snprintf"); >> + if (r >=3D MNT_ROOT_MAX_LEN) >> + errx(EXIT_FAILURE, "unexpectedly long mnt_root"); >> + >> + r =3D snprintf(source, SOURCE_MAX_LEN, "%s", sm->str + sm->sb_source); >> + if (r =3D=3D -1) >> + err(EXIT_FAILURE, "snprintf"); >> + if (r >=3D SOURCE_MAX_LEN) >> + errx(EXIT_FAILURE, "unexpectedly long sb_source"); >> +} > > Here and elsewhere, I suggest a wrapper around snprintf(). There's a readability cost to unfamiliar wrappers that I don't think is justified here. Maybe if we were doing this a lot more. >> +static void do_rename(int mnt, const char dir_name[static 1], >> + const char old_name[static 1], >> + const char new_name[static 1], mode_t mode) >> +{ >> + struct open_how how =3D { >> + .flags =3D O_PATH | O_CLOEXEC | O_DIRECTORY | O_NOFOLLOW, >> + .resolve =3D RESOLVE_NO_MAGICLINKS | RESOLVE_IN_ROOT | >> + RESOLVE_NO_SYMLINKS | RESOLVE_NO_XDEV, >> + }; >> + int dir =3D syscall(__NR_openat2, mnt, dir_name, &how, sizeof how); >> + if (dir =3D=3D -1) >> + err(EXIT_FAILURE, "openat2"); >> + >> + if (syscall(__NR_mkdirat, dir, new_name, mode) =3D=3D -1) >> + err(EXIT_FAILURE, "mkdirat"); >> + if (syscall(__NR_renameat2, dir, old_name, dir, new_name, >> + RENAME_EXCHANGE) =3D=3D -1) >> + err(EXIT_FAILURE, "renameat2"); >> +} >> + >> +int main(int argc, char *argv[]) >> +{ >> + int mnt; >> + mode_t mode; >> + uint64_t mnt_id; >> + char *disk_path, *dir_name, *old_name, *new_name, >> + mnt_root[MNT_ROOT_MAX_LEN], source[SOURCE_MAX_LEN]; >> + >> + if (argc !=3D 3) { >> + fprintf(stderr, "Usage: vm-set-persist ID INSTANCE\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (strchr(argv[1], '/')) >> + errx(EXIT_FAILURE, "invalid VM ID"); > > I'd check for ".", "..", empty string, and NAME_MAX (255) as well. Any of those will just mean the program fails. Same goes for /, of course, but that's more likely to happen by accident I think. >> + if (strchr(argv[2], '/')) >> + errx(EXIT_FAILURE, "invalid persistent directory name"); >> + if (asprintf(&disk_path, "/run/fs/%s/disk", argv[1]) =3D=3D -1) >> + err(EXIT_FAILURE, "asprintf"); >> + if (asprintf(&new_name, "persist.%s", argv[2]) =3D=3D -1) >> + err(EXIT_FAILURE, "asprintf"); > > I'd check that this doesn't go beyond NAME_MAX. > >> + set_mount_namespace(argv[1]); >> + >> + do_statx(disk_path, &mode, &mnt_id); >> + do_statmount(mnt_id, mnt_root, source); >> + >> + if (!(dir_name =3D strdup(mnt_root))) >> + err(EXIT_FAILURE, "strdup"); >> + dir_name =3D dirname(dir_name); >> + old_name =3D basename(mnt_root); >> + >> + mnt =3D do_mount(source); >> + >> + do_rename(mnt, dir_name, old_name, new_name, mode); >> +} > --=20 > Sincerely, > Demi Marie Obenour (she/her/hers) --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQQGoGac7QfI+H5ZtFCZddwkt31pFQUCaT6WQgAKCRCZddwkt31p FTq9AQCLSYqbU/W2KQC3+Wzgb7gDUL1BKmRFGHpbgt7IlMshFwD+INmqAE3s8hvj in4uMpPYO/NQL8eDhikZugLxlatndgU= =my7N -----END PGP SIGNATURE----- --=-=-=--