core: handle lookup paths being symlinks by andir · Pull Request #20479 · systemd/systemd

andir · 2021-08-18T23:28:26Z

With a recent change paths leaving the statically known lookup paths
would be treated differently then those that remained within those. That
was done (AFAIK) to consistently handle alias names. Unfortunately that
means that on some distributions, especially those where /etc/ consists
mostly of symlinks, would trigger that new detection for every single
unit in /etc/systemd/system. The reason for that is that the units
directory itself is already a symlink.

This is related to the issue discussed in #20003. cc @keszybz.

I haven't fully analyzed all the changes that went into the original change but this change seems to do what is required to make this edge case work (again). Let me know if there is another approach I could take. Unfortunately havin /etc/systemd/system not as a symlink isn't workable.

src/basic/unit-file.c

This updates systemd to version v249.4 from version v247.6. Besides the many new features that can be found in the upstream repository they also introduced a bunch of cleanup which ended up requiring a few more patches on our side. a) 0022-core-Handle-lookup-paths-being-symlinks.patch: The way symlinked units were handled was changed in such that the last name of a unit file within one of the unit directories (/run/systemd/system, /etc/systemd/system, ...) is used as the name for the unit. Unfortunately that code didn't take into account that the unit directories themselves could already be symlinks and thus caused all our units to be recognized slightly different. There is an upstream PR for this new patch: systemd/systemd#20479 b) The way the APIVFS is setup has been changed in such a way that we now always have /run. This required a few changes to the confinement tests which did assert that they didn't exist. Instead of adding another patch we can just adopt the upstream behavior. An empty /run doesn't seem harmful. As part of this work I refactored the confinement test just a little bit to allow better debugging of test failures. Previously it would just fail at some point and it wasn't obvious which of the many commands failed or what the unexpected string was. This should now be more obvious. c) Again related to the confinement tests the way a file was tested for being accessible was optimized. Previously systemd would in some situations open a file twice during that check. This was reduced to one operation but required the procfs to be mounted in a units namespace. An upstream bug was filed and fixed. We are now carrying the essential patch to fix that issue until it is backported to a new release (likely only version 250). The good part about this story is that upstream systemd now has a test case that looks very similar to one of our confinement tests. Hopefully that will lead to less friction in the long run. systemd/systemd#20514 systemd/systemd#20515 d) Previously we could grep for dlopen( somewhat reliably but now upstream started using a wrapper around dlopen that is most of the time used with linebreaks. This makes using grep not ergonomic anymore. With this bump we are grepping for anything that looks like a dynamic library name (in contrast to a dlopen(3) call) and replace those instead. That seems more robust. Time will tell if this holds. I tried using coccinelle to patch all those call sites using its tooling but unfornately it does stumble upon the _cleanup_ annotations that are very common in the systemd code. e) We now have some machinery for libbpf support in our systemd build. That being said it doesn't actually work as generating some skeletons doesn't work just yet. It fails with the below error message and is disabled by default (in both minimal and the regular build). > FAILED: src/core/bpf/socket_bind/socket-bind.skel.h > /build/source/tools/build-bpf-skel.py --clang_exec /nix/store/x1bi2mkapk1m0zq2g02nr018qyjkdn7a-clang-wrapper-12.0.1/bin/clang --llvm_strip_exec /nix/store/zm0kqan9qc77x219yihmmisi9g3sg8ns-llvm-12.0.1/bin/llvm-strip --bpftool_exec /nix/store/l6dg8jlbh8qnqa58mshh3d8r6999dk0p-bpftools-5.13.11/bin/bpftool --arch x86_64 ../src/core/bpf/socket_bind/socket-bind.bpf.c src/core/bpf/socket_bind/socket-bind.skel.h > libbpf: elf: socket_bind_bpf is not a valid eBPF object file > Error: failed to open BPF object file: BPF object format invalid > Traceback (most recent call last): > File "/build/source/tools/build-bpf-skel.py", line 128, in <module> > bpf_build(args) > File "/build/source/tools/build-bpf-skel.py", line 92, in bpf_build > gen_bpf_skeleton(bpftool_exec=args.bpftool_exec, > File "/build/source/tools/build-bpf-skel.py", line 63, in gen_bpf_skeleton > skel = subprocess.check_output(bpftool_args, universal_newlines=True) > File "/nix/store/81lwy2hfqj4c1943b1x8a0qsivjhdhw9-python3-3.9.6/lib/python3.9/subprocess.py", line 424, in check_output > return run(*popenargs, stdout=PIPE, timeout=timeout, check=True, > File "/nix/store/81lwy2hfqj4c1943b1x8a0qsivjhdhw9-python3-3.9.6/lib/python3.9/subprocess.py", line 528, in run > raise CalledProcessError(retcode, process.args, > subprocess.CalledProcessError: Command '['/nix/store/l6dg8jlbh8qnqa58mshh3d8r6999dk0p-bpftools-5.13.11/bin/bpftool', 'g', 's', '../src/core/bpf/socket_bind/socket-bind.bpf.o']' returned non-zero exit status 255. > [102/1457] Compiling C object src/journal/libjournal-core.a.p/journald-server.c.oapture output)put)ut) > ninja: build stopped: subcommand failed. f) We do now have support for TPM2 based disk encryption in our systemd build. The actual bits and pieces to make use of that are missing but there are various ongoing efforts in that direction. There is also the story about systemd in our initrd to enable this being used for root volumes. None of this will yet work out of the box but we can start improving on that front. g) FIDO2 support was added systemd and consequently we can now use that. Just with TPM2 there hasn't been any integration work with NixOS and instead this just adds that capability to work on that. Co-Authored-By: Jörg Thalheim <[email protected]>

andir · 2021-09-27T19:58:15Z

@yuwata thanks for adding the labels. Is there anyone I could ping for a review?

poettering

i think @keszybz should also have a closer look, given he touched this code last

src/basic/unit-file.c

poettering · 2021-09-28T16:58:08Z

src/basic/unit-file.c

+         */
+        STRV_FOREACH(dir, (char**) lp->search_path) {
+                _cleanup_free_ char *resolved_dir = NULL;
+                r = strv_extend(&expanded_search_paths, *dir);


why include both the regular and the "expanded" ones? the latter should suffice, no?

poettering · 2021-09-28T17:00:04Z

src/basic/unit-file.c

+                        return log_oom();
+        }
+
        STRV_FOREACH(dir, (char**) lp->search_path) {


hmm, so why not just iterate through the reduced search path here?

i.e it appears to me the "expanded" search path should be called the "reduced" search path, and also maybe we should call strv_uniq() or so, to maybe strip duplicates. And then after all that we shoud operate purely on that and basically forget about original search path while building the map.

hmm, so why not just iterate through the reduced search path here?

I don't really know anymore. IIRC I was just proposing the minimal solution here to get some feedback on the approach. You might be correct that we can change all the search_path usages to the new list of paths. My only concern would be that in cases where e.g. /etc/systemd/system is already a symlink to another location we would then never see that path in systemctl {status,cat} <unit>? Arguably that would be the correct information but perhaps a bit misleading to the unaware?

I think we need both. I'm rebasing the patch now and I added an comment that describes this.

andir · 2021-11-13T10:15:52Z

How shall we continue here? Since the reviews have so far been mostly around minor details I am hesitant to put in more work until we hear from @keszybz on the general approach.

ReillyBrogan · 2022-02-17T02:42:23Z

It would be great to see this one get merged. Solus Linux has a /usr/lib -> /usr/lib64 symlink and has major issues without this patch.

poettering · 2022-04-12T15:01:53Z

Does this fix #22961?

@keszybz

keszybz · 2022-05-04T16:36:17Z

Sorry for the delay. I think this is the right thing to do. I probably doesn't help for #22961 though. I'm rebasing this now and will force-push in a sec.

src/basic/unit-file.c

andir

Thank you for picking up the PR (and getting it updated!).

keszybz · 2022-05-05T11:37:02Z

CI passed. I'll update with the typo fixed. Is it fine to merge this? If yes, I don't think we need for the CI again.

bluca · 2022-05-05T11:38:54Z

It looks good to me, not sure if you were waiting for a review from @poettering too

With a recent change paths leaving the statically known lookup paths would be treated differently then those that remained within those. That was done (AFAIK) to consistently handle alias names. Unfortunately that means that on some distributions, especially those where /etc/ consists mostly of symlinks, would trigger that new detection for every single unit in /etc/systemd/system. The reason for that is that the units directory itself is already a symlink. Rebased-by: Zbigniew Jędrzejewski-Szmek <[email protected]>

andir commented Aug 19, 2021

View reviewed changes

src/basic/unit-file.c Outdated Show resolved Hide resolved

andir force-pushed the resolve-symlink-unit-dirs branch from 2a2eb8e to 5f17b65 Compare August 20, 2021 08:19

andir changed the title ~~core: Handle lookup paths being symlinks~~ core: handle lookup paths being symlinks Aug 21, 2021

yuwata added the pid1 label Aug 22, 2021

flokli mentioned this pull request Aug 31, 2021

systemd v249 NixOS/nixpkgs#131618

Merged

2 tasks

yuwata added the needs-review label Sep 3, 2021

poettering requested changes Sep 28, 2021

View reviewed changes

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review labels Sep 28, 2021

poettering mentioned this pull request Sep 30, 2021

systemd doesn't start units in the default target #20003

Closed

poettering linked an issue Sep 30, 2021 that may be closed by this pull request

systemd doesn't start units in the default target #20003

Closed

andir mentioned this pull request Sep 30, 2021

Set maximum line length in editorconfig for C and XML files #20894

Merged

flokli mentioned this pull request Dec 7, 2021

systemd: 249.5 -> 249.7 & various fixes NixOS/nixpkgs#149153

Merged

13 tasks

anitazha requested a review from keszybz February 17, 2022 19:00

poettering mentioned this pull request Apr 12, 2022

systemd fails to enable certain units when /etc/systemd/system is a symlink #22961

Closed

poettering added the needs-rebase label Apr 12, 2022

keszybz force-pushed the resolve-symlink-unit-dirs branch from 5f17b65 to aabda8f Compare May 4, 2022 16:45

keszybz removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks needs-rebase labels May 4, 2022

yuwata reviewed May 4, 2022

View reviewed changes

src/basic/unit-file.c Show resolved Hide resolved

yuwata added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label May 4, 2022

keszybz force-pushed the resolve-symlink-unit-dirs branch from aabda8f to b145423 Compare May 4, 2022 20:09

keszybz added please-review and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels May 4, 2022

bluca reviewed May 5, 2022

View reviewed changes

src/basic/unit-file.c Outdated Show resolved Hide resolved

bluca approved these changes May 5, 2022

View reviewed changes

andir commented May 5, 2022

View reviewed changes

keszybz force-pushed the resolve-symlink-unit-dirs branch from b145423 to 87ccb72 Compare May 5, 2022 11:40

keszybz removed their request for review May 5, 2022 11:41

daandemeyer approved these changes May 5, 2022

View reviewed changes

keszybz merged commit 66c38cd into systemd:main May 5, 2022

mrc0mmand removed the please-review label May 5, 2022

Uh oh!

Conversation

andir commented Aug 18, 2021

Uh oh!

Uh oh!

andir commented Sep 27, 2021

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

poettering Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

poettering Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

andir Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

keszybz May 4, 2022

Choose a reason for hiding this comment

Uh oh!

andir commented Nov 13, 2021

Uh oh!

ReillyBrogan commented Feb 17, 2022

Uh oh!

poettering commented Apr 12, 2022

Uh oh!

keszybz commented May 4, 2022

Uh oh!

Uh oh!

Uh oh!

andir left a comment

Choose a reason for hiding this comment

Uh oh!

keszybz commented May 5, 2022

Uh oh!

bluca commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

8 participants