I have been looking through this:

https://gitlab.com/postmarketOS/pmaports/-/tree/systemd/systemd/systemd

With my upstream systemd hat on I just wanted to mention two things:

https://gitlab.com/postmarketOS/pmaports/-/blob/systemd/systemd/systemd/0011-src-basic-missing.h-check-for-missing-strndupa.patch

You should be able to drop the patch entirely, we don't use strndupa() anymore. (Might need to backport 10d50d9eac0ed94576928a23e3fa525ddc49c2aa from upstream possibly on some not completely recent version, since for a short time it was reintroduced again by accident).

https://gitlab.com/postmarketOS/pmaports/-/blob/systemd/systemd/systemd/0016-don-t-pass-AT_SYMLINK_NOFOLLOW-flag-to-faccessat.patch

This one you should just kill too. It's a crazy patch anyway, and all kernels since 2020 have a new syscall that addresses this properly and musl is using this too afaik. Hence the patch can just be dropped.

It appears cloud-hypervisor implements the same as firecracker. If qemu did too, there'd be three implementations of the same logic, yay!

So a little bird tells me that libcryptsetup should suspend the DM device with DM_SUSPEND_LOCKFS_FLAG and without DM_SUSPEND_NOFLUSH_FLAG. Right now this is not what libcryptsetup does. With that the suspend should result in a full sync/fsfreeze. That should mean that if userspace follows up with a BLKFLSBUF then we are sure that everything was written to disk before the suspend, and we also know that nothing new could have been read off disk polluting the buffer cache with unencrypted data, and that anything already in the buffer cache would be dropped from it.

Yes, please, it's a general recommendation: props should be kept on udev devices on all "positive" events, not just some.

Yeah, it's probably something to do in a separate PR. I got confused because I only looked at the two linked commits but not actually at the commit of this PR (which doesn't actually mention the ACTION itself, unlike the linked commits).

Please change ACTION=="add|change" → ACTION!="remove".

We generally want that flags/props are not dropped if you do a "udevadm trigger" call at any time. Hence in most cases you want to denylist the single "negative" event, instead of allowlisting a small subset of the "positive" events. (by positive/negative I mean that the device exists or not after the event is seen)

Yeah, that looks excellent, and is exactly what I'd need! Thanks so much!

awesome!

I really dislike the interposition stuff I must say. I always find that a hack, not clean. I made it working for me, but I'd be happy to swap it out for something else.

so, a crypt_token_set_external_path() would be still very very welcome.

Hmm, so, what about a different approach: allow apps to change the path? i.e. add crypt_token_set_external_path()? would that be acceptable? for me that would be equally useful and i don't need to write any LD_PRELOAD tool and that mess?

I sympathize a lot with not having a config file. In systemd we consider env vars as these hidden, unstable hacks to tweak behaviour mostly for debugging purposes of our tools. For example they aren't documented in the man pages, but in a separate doc that makes clear they do not come with the same guarantees and are considered "secondary" parameterization, not for clean code paths.

You are not the first, though. NIX is already patching the token path, for debugging you can use LD_PRELOAD trick to overwrite crypt_token_external_path() (we use it in tests https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/tests/fake_token_path.c).

Well, that's a complicated hack and ultimately just means you set an env var for this after all, just a super powerful one.

This makes it easier to debug cryptsetup LUKS2 token modules, as one can direct the tools to look for the modules in a separate directory.

Fixes: #846

Lennart Poettering (54f630a9) at 24 Oct 18:25

... and 3 more commits

To make it easier to debug cryptsetup token plugins it would be great if crypt_token_external_path() would use some env var if it is set as primary place to look for the token modules in.

i.e. all i want is pretty much that crypt_token_external_path() becomes:

const char *crypt_token_external_path(void)
{
        return external_tokens_enabled ? (secure_getenv("CRYPTSETUP_TOKENS_PATH") ?: EXTERNAL_LUKS2_TOKENS_PATH) : NULL;
}

that would make it tremendously easier to debug cryptsetup token logic, since one wouldn't have to install the modules every time.

Lennart Poettering (20bfec91) at 16 Oct 17:16

Right now libcryptsetup returns ENODEV in case crypt_activate_by_signed_key() is called for a signed dm-verity block device. It returns ENODEV in some other cases too though, for example if we concurrently set up the same verity device from the same file twice.

This is pretty annoying since we'd like to handle such errors differntly: in the latter case we'd rather retry a couple of times. In the key missing case we'd however completely fail and give the user a friendly log message about the key missing in the keyring. Right now we cannot do that however.

Hence, any chance that this can be fixed in libcryptsetup? if crypt_activateby_signed_key() would return ENOKEY in this case this would be ideal to us.

Also please add "Fixes: #841 (closed)." line at the end of commit message.

done.

done

Lennart Poettering (20bfec91) at 16 Oct 12:38

Lennart Poettering (0006da4b) at 16 Oct 12:35