Re: [PATCH v5 11/13] Support updates via systemd-sysupdate

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 11920 bytes --]

On 11/28/25 08:47, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
> 
>> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
>> index a6d9f23e9f5277b7c79a53105eb2dfe1bab1451e..74ff64019560aae6387df0e1b3409bc174251bdb 100644
>> --- a/host/rootfs/Makefile
>> +++ b/host/rootfs/Makefile
>> @@ -10,6 +10,7 @@ include file-list.mk
>>  ROOT_FS = build
>>  
>>  DIRS = \
>> +	boot \
>>  	dev \
>>  	etc/s6-linux-init/env \
>>  	etc/s6-linux-init/run-image/configs \
>> @@ -33,13 +34,15 @@ DIRS = \
>>  	etc/s6-linux-init/run-image/vm/by-id \
>>  	etc/s6-linux-init/run-image/vm/by-name \
>>  	ext \
>> +	home \
>>  	proc \
>>  	run \
>> -	sys
>> +	sys \
>> +	tmp
>>  
>>  FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>>  
>> -BUILD_FILES = build/etc/s6-rc
>> +BUILD_FILES = build/etc/s6-rc build/etc/os-release build/etc/update-url
>>  
>>  # This rule produces three files but Make only (portably)
>>  # supports one output per rule.  Instead of resorting to temporary
>> @@ -59,12 +62,22 @@ $(ROOT_FS_IMAGE): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_
>>  	mkdir -p $(ROOT_FS) && \
>>  	{ \
>>  	    cat $(PACKAGES_FILE) ;\
>> +	    printf '%s\n%s\n' "$$UPDATE_SIGNING_KEY" /etc/systemd/import-pubring.gpg; \
> 
> Inconsistent use of shell variable instead of make macro.
> 
>>  	    for file in $(FILES) $(LINKS); do printf '%s\n%s\n' $$file "$${file#image/}"; done ;\
>>  	    for file in $(BUILD_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\
>>  	    printf 'build/empty\n%s\n' $(DIRS) ;\
>>  	    printf 'build/fifo\n%s\n' $(FIFOS) ;\
>>  	} | ../../scripts/make-erofs.sh $@
>>  
>> +build/etc/update-url:
>> +	mkdir -p build/etc
>> +	# might have metacharacters, so avoid interpolation
>> +	printf %s\\n "$${UPDATE_URL:?'update URL empty or missing'}" > build/etc/update-url
> 
> I'm learning so many shell parameter expansions I didn't know from you :)
> 
>> diff --git a/host/rootfs/image/usr/bin/spectrum-update b/host/rootfs/image/usr/bin/spectrum-update
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..613b43570d0538fce20296ccb1de2a6364e0df55
>> --- /dev/null
>> +++ b/host/rootfs/image/usr/bin/spectrum-update
>> @@ -0,0 +1,92 @@
>> +#!/bin/execlineb -WS1
>> +# SPDX-License-Identifier: EUPL-1.2+
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>> +
>> +if { mkdir -p -m 0700 /run/updater }
>> +
>> +# Take a global lock to avoid races.
>> +s6-setlock /run/update-lock
>> +
>> +foreground { redirfd -w 2 /dev/null rmdir -- $1 }
>> +if { umask 0077 mkdir -p -- $1 }
>> +cd $1
>> +foreground {
>> +  # If this exists already that is okay.
>> +  foreground { redirfd -w 2 /dev/null btrfs subvolume create -- shared }
>> +
>> +  # Delete any stale temporary files.  Delete any existing signature
>> +  # files.  If the VM is still running (it should not be), the VM might
>> +  # have write access to the directory.  However, updates-dir-check is
>> +  # safe against that.
>> +  if { updates-dir-check cleanup shared }
>> +
>> +  if {
>> +    foreground {
>> +      # TODO: suppress only "subvolume does not exist" errors.
>> +      redirfd -w 2 /dev/null
>> +      btrfs subvolume delete snapshot
>> +    }
>> +    rm -f snapshot
>> +  }
>> +
>> +  backtick -E update_vm_id {
>> +    backtick -E id_path { readlink /run/vm/by-name/sys.appvm-systemd-sysupdate }
>> +    basename -- $id_path
>> +  }
>> +
>> +  # $fsdir is read-only to the guest, but read-write to the host.
>> +  # Directories bind-mounted into it are read-write to the guest.
>> +  # See etc/s6-linux-init/run-image/service/vhost-user-fs/template/run
>> +  # for details.
>> +
> 
> This still refers to a non-existent variable.
> 
>> +  # Set up /etc with what the VM needs.  The VM will overlay this
>> +  # on its own /etc.
>> +  #
>> +  # In the future, this should use a bind mount instead of copying
>> +  # into a tmpfs.  However, this would significantly complicate the
>> +  # cleanup code.  Deleting fs/etc would require undoing the bind
>> +  # mounts instead of rm -rf.  Once this code is in a separate mount
>> +  # namespace, the copies should be replaced by bind mounts.
>> +  if {
>> +    if { rm -rf -- /run/vm/by-id/${update_vm_id}/fs/etc }
>> +    umask 022
>> +    if { mkdir -p -- /run/vm/by-id/${update_vm_id}/fs/updates /run/vm/by-id/${update_vm_id}/fs/etc/systemd }
>> +    if { cp -R -- /etc/vm-sysupdate.d /etc/update-url /run/vm/by-id/${update_vm_id}/fs/etc }
>> +    cp -- /etc/systemd/import-pubring.gpg /run/vm/by-id/${update_vm_id}/fs/etc/systemd
>> +  }
>> +
>> +  # If the directory is already mounted, unmount it.  This prevents a
>> +  # confusing error from mount.
>> +  foreground { redirfd -w 2 /dev/null umount -- /run/vm/by-id/${update_vm_id}/fs/updates }
>> +
>> +  # Share the update directory with the VM.
>> +  if { mount --bind -- shared /run/vm/by-id/${update_vm_id}/fs/updates }
>> +
>> +  # Start the update VM.
>> +  if { vm-start $update_vm_id }
>> +
>> +  # Wait for the VM to exit.
>> +  # TODO: This is racy.  If the update finishes before this code runs,
>> +  # the s6-svwait call will fail.
>> +  if { s6-svwait -D /run/service/vmm/instance/${update_vm_id} }
>> +
>> +  # Remove the bind mount.
>> +  if { umount -- /run/vm/by-id/${update_vm_id}/fs/updates }
>> +
>> +  # Ensure that the VM cannot change the directory
>> +  # while systemd-sysupdate is using it.
>> +  if { btrfs subvolume snapshot -- shared snapshot }
>> +
>> +  # Validate the update directory.  Delete any stale temporary files.
>> +  # Check that a signature file was downloaded.
>> +  if { updates-dir-check check snapshot }
>> +
>> +  unshare --mount
>> +  if { mount --bind -o ro -- snapshot /run/updater }
>> +
>> +  /usr/lib/systemd/systemd-sysupdate update
> 
> Why not just make a readonly snapshot?
> (btrfs subvolume snapshot -r)

The checker will delete any temporary files it comes across, so it
needs write access.  A snapshot is much heavier than a bind mount
and isn't automatically cleaned up.

>> diff --git a/vm/app/systemd-sysupdate/default.nix b/vm/app/systemd-sysupdate/default.nix
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..69be0bab500ea2ea6cb3b6d71edbf1a3e7bddbba
>> --- /dev/null
>> +++ b/vm/app/systemd-sysupdate/default.nix
>> @@ -0,0 +1,26 @@
>> +# SPDX-License-Identifier: MIT
>> +# SPDX-FileCopyrightText: 2023 Alyssa Ross <hi@alyssa.is>
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>> +
>> +import ../../../lib/call-package.nix (
>> +{ callSpectrumPackage, curl, lib, src
>> +, runCommand, systemd, writeScript
>> +}:
>> +
>> +let
>> +  downloadUpdate = builtins.path {
>> +    name = "download-update";
>> +    path = ./download-update;
>> +  };
> 
> builtins.path is overkill here surely, as opposed to just writing
> ${./download-update} below?

${./download-update} includes the working directory in the Nix store
hash, which means that renaming your source tree forces an unnecessary
rebuild.  builtins.path is the standard way to avoid this.

>> +in
>> +
>> +callSpectrumPackage ../../make-vm.nix {} {
>> +  providers.net = [ "sys.netvm" ];
>> +  type = "nix";
>> +  run = writeScript "run-script" ''
>> +#!/usr/bin/env -S execlineb -WS0
> 
> #!/bin/execlineb -WS0 would be fine — we know that'll exist in the VM.

Will fix.

>> diff --git a/vm/app/systemd-sysupdate/download-update b/vm/app/systemd-sysupdate/download-update
>> new file mode 100755
>> index 0000000000000000000000000000000000000000..eada41c6c8ad5edcedd9f4d76b76492e0b8be826
>> --- /dev/null
>> +++ b/vm/app/systemd-sysupdate/download-update
>> @@ -0,0 +1,68 @@
>> +#!/usr/bin/env -S execlineb -WS0
>> +# SPDX-License-Identifier: EUPL-1.2+
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>> +export LC_ALL C
>> +export LANGUAGE C
>> +if { mount -toverlay -olowerdir=/run/virtiofs/virtiofs0/etc:/etc -- overlay /etc }
>> +backtick tmpdir { mktemp -d /tmp/sysupdate-XXXXXX }
>> +# Not a useless use of cat: if there are NUL bytes in the URL
>> +# busybox's awk might misbehave.
>> +backtick update_url { cat /etc/update-url }
>> +if {
>> +  backtick sed_rhs {
>> +    # Use awk to both validate the URL and to escape sed metacharacters.
>> +    # Reject URLs with control characters, query parameters, or fragments.
>> +    # They *cannot* work and so are rejected to produce better error messages.
>> +    #
>> +    # curl rejects control characters with "Malformed input to a URL function".
>> +    # Fragment specifiers ("#") and query parameters ("?") break concatenating
>> +    # /SHA256SUMS and /SHA256SUMS.sha256.asc onto the update URL.  Also, it is
>> +    # simpler to reject update URLs that contain whitespace than to try to
>> +    # escape them.
>> +    #
>> +    # Backslash needs to be escaped once for systemd-sysupdate and again for sed.
>> +    # Ampersand needs to be escaped once for sed.
>> +    awk "BEGIN {
>> +      update_url = ENVIRON[\"update_url\"];
>> +      if (update_url ~ /^[^\\001-\\040?#\\x7F]+$/) {
>> +        # Use & to avoid extra escaping (16 or 32 backslashes!)
>> +        # and a divergence between POSIX and GNU awk.
>> +        gsub(/\\\\/, \"&&&&\", update_url);
>> +        gsub(/&/, \"\\\\\\\\&\", update_url);
>> +        print update_url;
>> +        exit 0;
>> +      } else {
>> +        print ARGV[2] > \"/dev/stderr\";
>> +        exit 100;
>> +      }
>> +    }" -- $3
>> +    "Bad update URL from host: control characters, whitespace, query parameters, and fragment specifiers not allowed"
>> +  }
>> +  elglob -w -0 transfer_file_ /etc/vm-sysupdate.d/*.transfer
>> +  forx -E transfer_file { $transfer_file_ }
>> +  backtick target_basename {
>> +    basename -- $transfer_file
>> +  }
>> +  multisubstitute {
>> +    importas -iuS sed_rhs
>> +    importas -iuS target_basename
>> +    importas -iuS tmpdir
>> +    define sed_input $transfer_file
>> +  }
> 
> You could avoid some serial substitution here if you wanted, by not
> passing -E to forx:
> 
> forx transfer_file { $transfer_file_ }
> backtick target_basename {
>   importas -iuS transfer_file
>   basename -- $transfer_file
> }
> multisubstitute {
>   …
> }

I considered it and decided that the extra define in the
multisubstitute was cheaper than the extra importas process.

>> +  redirfd -w 1 ${tmpdir}/${target_basename}
>> +  sed -E -- "s#@UPDATE_URL@#${sed_rhs}#g" $sed_input
> 
> Using awk to escape stuff for sed seems a bit Rube Goldberg.  Would it
> make more sense to just do the replacement in the awk program?  Actually
> a lot of this might be nicer in awk than execline?  Feel free to tell me
> to leave it this way for now, though.

I'd prefer to leave it this way for now.  Maybe add a TODO to clean
this up.

>> +}
>> +multisubstitute {
>> +  importas -iuS update_url
>> +  importas -iuS CURL_PATH
>> +  importas -iuS SYSTEMD_SYSUPDATE_PATH
>> +  importas -iuS tmpdir
>> +}
>> +if { $SYSTEMD_SYSUPDATE_PATH --definitions=${tmpdir} update }
>> +# [ and ] are allowed in update URLs so that IPv6 addresses work, but
>> +# they cause globbing in the curl command-line tool by default.  Use --globoff
>> +# to disable this feature.
>> +if { $CURL_PATH -L --proto-redir =http,https --globoff
>> +     -o /run/virtiofs/virtiofs0/updates/SHA256SUMS -- ${update_url}/SHA256SUMS }
>> +$CURL_PATH -L --proto-redir =http,https --globoff
>> +     -o /run/virtiofs/virtiofs0/updates/SHA256SUMS.sha256.asc -- ${update_url}/SHA256SUMS.sha256.asc
> 
> Much easier to understand now.  Thanks!

You're welcome!
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]