Re: [PATCH v4 1/2] Build verity images in rootfs Nix derivation

[Alyssa Ross <hi@alyssa.is>]

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

Whoops, just realised I replied to v4 and not v5.  You can ignore this,
and I'll resend any comments that are still relevant to v5.

Alyssa Ross <hi@alyssa.is> writes:

> Demi Marie Obenour <demiobenour@gmail.com> writes:
>
>> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
>> index 27a7c689c39bf9bc93b5ba33ce661be7e47b67f1..055185064d84d9450c2076fdeb410b21d00f1d40 100644
>> --- a/host/rootfs/Makefile
>> +++ b/host/rootfs/Makefile
>> @@ -1,12 +1,12 @@
>>  # SPDX-License-Identifier: EUPL-1.2+
>>  # SPDX-FileCopyrightText: 2021-2024 Alyssa Ross <hi@alyssa.is>
>> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
>>  
>>  .POSIX:
>>  
>>  include ../../lib/common.mk
>>  include file-list.mk
>> -
>> -dest = build/rootfs.erofs
>> +ROOT_FS_DIR = build
>>  
>>  DIRS = \
>>  	dev \
>> @@ -46,15 +46,27 @@ FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>>  
>>  BUILD_FILES = build/etc/s6-rc
>>  
>> -$(dest): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk
>> -	set -euo pipefail; \
>> +build/verity-timestamp: $(ROOT_FS)
>> +	$(VERITYSETUP) format \
>> +		--root-hash-file $(ROOT_FS_VERITY_ROOTHASH) \
>> +		-- $(ROOT_FS) $(ROOT_FS_VERITY)
>> +	# Add trailing newline
>> +	echo >> $(ROOT_FS_VERITY_ROOTHASH)
>
> Why do we need to do this?
>
> (Emacs would also rather your comments were not indented, so they're
> interpreted by Make as comments rather than being passed on to the
> shell.)
>
>> +	touch -- $(ROOT_FS_DIR)/verity-timestamp
>
> This should be build/verity-timestamp (like the rule), or even better $@.
>
>> +
>> +# This rule produces three files but Make only (portably)
>> +# supports one output per rule.  Instead of resorting to temporary
>> +# files, a timestamp file is created as the last step.  The actual
>> +# outputs are produced as side-effects.
>
> Is this comment supposed to be on the previous rule?
>
>> +$(ROOT_FS): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk
>> +	mkdir -p $(ROOT_FS_DIR) && \
>>  	{ \
>>  	    cat $(PACKAGES_FILE) ;\
>>  	    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 $@
>> +	} | ../../scripts/make-erofs.sh $(ROOT_FS)
>
> Why change this?
>
>>  
>>  build/fifo:
>>  	mkdir -p build
>> @@ -83,25 +95,10 @@ clean:
>>  	rm -rf build
>>  .PHONY: clean
>>  
>> -# veritysetup format produces two files, but Make only (portably)
>> -# supports one output per rule, so we combine the two outputs then
>> -# define two more rules to separate them again.
>> -build/rootfs.verity: $(dest)
>> -	$(VERITYSETUP) format $(dest) build/rootfs.verity.superblock.tmp \
>> -	    | awk -F ':[[:blank:]]*' '$$1 == "Root hash" {print $$2; exit}' \
>> -	    > build/rootfs.verity.roothash.tmp
>> -	cat build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp \
>> -	    > $@
>> -	rm build/rootfs.verity.roothash.tmp build/rootfs.verity.superblock.tmp
>> -build/rootfs.verity.roothash: build/rootfs.verity
>> -	head -n 1 build/rootfs.verity > $@
>> -build/rootfs.verity.superblock: build/rootfs.verity
>> -	tail -n +2 build/rootfs.verity > $@
>> -
>> -build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk build/rootfs.verity.superblock build/rootfs.verity.roothash $(dest)
>> +build/live.img: ../../scripts/format-uuid.sh ../../scripts/make-gpt.sh ../../scripts/sfdisk-field.awk $(ROOT_FS_DIR)/verity-timestamp $(ROOT_FS)
>
> Here you're also still referring to $(ROOT_FS_DIR)/verity-timestamp
> rather than build/verity-timestamp.
>
>>  	../../scripts/make-gpt.sh $@.tmp \
>> -	    build/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=build/rootfs.verity.roothash bs=32 skip=1 count=1 status=none)") \
>> -	    $(dest):root:$$(../../scripts/format-uuid.sh "$$(head -c 32 build/rootfs.verity.roothash)")
>> +	    $(ROOT_FS)/rootfs.verity.superblock:verity:$$(../../scripts/format-uuid.sh "$$(dd if=$(ROOT_FS_VERITY_ROOTHASH) bs=32 skip=1 count=1 status=none)") \
>> +	    $(ROOT_FS)/rootfs:root:$$(../../scripts/format-uuid.sh "$$(head -c 32 $(ROOT_FS_VERITY_ROOTHASH)")
>
> This can't be right, can it?  $(ROOT_FS) is a file.
>
>>  	mv $@.tmp $@
>>  
>>  debug:
>> @@ -111,7 +108,7 @@ debug:
>>  	    $(VMLINUX)
>>  .PHONY: debug
>>  
>> -run: build/live.img build/rootfs.verity.roothash
>> +run: build/live.img
>
> I'd still prefer we kept the explicit dependency, even though we will
> get it via build/live.img as well.
>
>>  	@set -x && \
>>  	ext="$$(mktemp build/spectrum-rootfs-extfs.XXXXXXXXXX.img)" && \
>>  	truncate -s 10G "$$ext" && \
>> @@ -132,7 +129,7 @@ run: build/live.img build/rootfs.verity.roothash
>>  	    -device virtconsole,chardev=virtiocon0 \
>>  	    -drive file=build/live.img,if=virtio,format=raw,readonly=on \
>>  	    -drive file=/proc/self/fd/3,if=virtio,format=raw \
>> -	    -append "earlycon console=hvc0 roothash=$$(< build/rootfs.verity.roothash) intel_iommu=on nokaslr" \
>> +	    -append "earlycon console=hvc0 roothash=$$(< $(ROOT_FS_VERITY_ROOTHASH)) intel_iommu=on nokaslr" \
>>  	    -device virtio-keyboard \
>>  	    -device virtio-mouse \
>>  	    -device virtio-gpu \
>> diff --git a/host/rootfs/default.nix b/host/rootfs/default.nix
>> index 0ac70c7c077c0656c5820a5d8b3c7ce0e7c78e54..1578155fa0fb9a4df3fb4884e21ed7d8d8f821dc 100644
>> --- a/host/rootfs/default.nix
>> +++ b/host/rootfs/default.nix
>> @@ -138,7 +138,7 @@ stdenvNoCC.mkDerivation {
>>    };
>>    sourceRoot = "source/host/rootfs";
>>  
>> -  nativeBuildInputs = [ erofs-utils spectrum-build-tools s6-rc ];
>> +  nativeBuildInputs = [ cryptsetup erofs-utils spectrum-build-tools s6-rc ];
>>  
>>    env = {
>>      PACKAGES = runCommand "packages" {} ''
>> @@ -147,7 +147,9 @@ stdenvNoCC.mkDerivation {
>>      '';
>>    };
>>  
>> -  makeFlags = [ "dest=$(out)" ];
>> +  # The Makefile uses $(ROOT_FS_DIR), not $(dest), so it can share code
>> +  # with other Makefiles that also use this variable.
>> +  makeFlags = [ "ROOT_FS_DIR=$(out)" ];
>>  
>>    dontInstall = true;
>>  
>> diff --git a/host/rootfs/shell.nix b/host/rootfs/shell.nix
>> index 1bf61bebf418333624e799cc8ca231f5783206f4..6df2f575fdfc7cdf8067ccfdb5fecaad9f6ea5e6 100644
>> --- a/host/rootfs/shell.nix
>> +++ b/host/rootfs/shell.nix
>> @@ -12,7 +12,7 @@ rootfs.overrideAttrs (
>>  
>>  {
>>    nativeBuildInputs = nativeBuildInputs ++ [
>> -    btrfs-progs cryptsetup jq netcat qemu_kvm reuse util-linux
>> +    btrfs-progs jq netcat qemu_kvm reuse util-linux
>>    ];
>>  
>>    env = env // {
>> diff --git a/lib/common.mk b/lib/common.mk
>> index 277c3544036d9a9057f8ba4ad37fe2207548cc59..d1cc4d0514070cc3f418c4d1b7e929abd40d985c 100644
>> --- a/lib/common.mk
>> +++ b/lib/common.mk
>> @@ -11,6 +11,10 @@ GDB = gdb
>>  MCOPY = mcopy
>>  MKFS_FAT = mkfs.fat
>>  MMD = mmd
>> +ROOT_FS = $(ROOT_FS_DIR)/rootfs
>
> Would be nice for this to keep its file extension.
>
>> +ROOT_FS_IMAGES = $(ROOT_FS) $(ROOT_FS_VERITY_ROOTHASH) $(ROOT_FS_VERITY)
>
> I'm not sure "IMAGES" makes sense as a name for this.  A verity roothash
> is not an image.  ROOT_FS_FILES?
>
> Alternative naming scheme idea, that avoids mistaking ROOT_FS for the
> directory like has happened above:
>
> ROOT_FS (for the directory), ROOT_FS_IMAGE, ROOT_FS_VERITY,
> ROOT_FS_VERITY_ROOTHASH.

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