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

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 5889 bytes --]

On Fri, Nov 28, 2025 at 04:28:20PM -0500, Demi Marie Obenour wrote:
> On 11/28/25 16:08, Alyssa Ross wrote:
> > Demi Marie Obenour <demiobenour@gmail.com> writes:
> >
> >> On 11/28/25 15:41, Alyssa Ross wrote:
> >>> Demi Marie Obenour <demiobenour@gmail.com> writes:
> >>>
> >>>> On 11/28/25 08:47, Alyssa Ross wrote:
> >>>>> Demi Marie Obenour <demiobenour@gmail.com> writes:
> >>>>>
> >>>>>> 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.
> >>
> >> Whoops!
> >>
> >>>>>> +  # 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.
> >>>
> >>> Okay, but why do we need to block systemd-sysupdate from writing to this
> >>> directory, but not to anywhere else on the system?
> >>
> >> We don't.  I'm fine with a writable mount.  Either should work.
> >
> > Then let's keep things simple and drop the unshare and mount.  We can
> > deprivilege it properly in the future.
>
> The mount is absolutely necessary: it ensures that the update is
> available where the .transfer file says it will be.  It just doesn't
> need to be read-only.
>
> The unshare is there to allow trivial cleanup by the kernel once
> systemd-sysupdate exits.

Ah, then I was misreading.

If the mount has to be there, it might as well be read-only.

With the outdated comment removed:

Reviewed-by: Alyssa Ross <hi@alyssa.is>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]