install: follow config_path symlink

[rimmington]

Under NixOS, the config_path /etc/systemd/system is a symlink to /etc/static/systemd/system. Commands such as systemctl list-unit-files and systemctl is-enabled did not work as the symlink was not followed.

This does not affect how symlinks are treated within the config_path directory.

This fixes NixOS/nixpkgs#1083.

[rimmington]

CC @mjhoy

[poettering]

Sorry. But this isnt right. we use symlinks for service enablement configuration. Thus when enumerating the installed unit files we dont follow symlinks on purpose: we are interested in the actually installed unit names and not under the names they were enabled under... just dropping o_follow really is too simple.

[mjhoy]

@poettering But in this case isn't config_path separate from service enablement configuration? I don't think this affects how the unit files themselves are opened

[poettering]

ah, uh, I missed that this was about the find_symlinks() code, so yeah this doesn't affect how the unit files themselves are opened.

However, the problem I see with this is that dropping O_NOFOLLOW means the kernel will follow symlinks and not take our optionally specified root dir into account while doing so... And that's a bigger problem... Right now, if you use "systemctl --root=... list-unit-files" you get a failure with such a symlinked dir, but with your patch applied, then we might end up reading the host's unit files, not the --root= ones....

Note sure what to suggest about that...

[Mathnerd314]

Maybe: open the path with O_NOFOLLOW, check for ELOOP on error, and, if that was the error, then read the symlink's target, prefix the root directory to that target, and open the result, in a loop until opening no longer gives ELOOP.

[DamienCassou]

What is the status of this PR? NixOS still suffers from this problem.

[rimmington]

Apologies for the delay, I've been quite busy and this slipped my mind.

In the course of implementing @Mathnerd314's suggestion, I've run across some unexpected behaviour: with systemd 230, systemctl list-unit-files --root=... doesn't appear to use find_symlinks(). In fact, it doesn't appear to use O_NOFOLLOW when opening any directory under the specified root, according to strace. This was observed of systemd 230 in NixOS (ie. with one small, irrelevant patch). In contrast, systemd 225 on my Ubuntu box does use O_NOFOLLOW.

I'm not sure if this is expected behaviour and am not familiar enough with the systemd codebase to check, but it's made regression testing difficult. I'm still trying to track down where this all happens.

[rimmington]

@poettering Additional testing in an Ubuntu 16.04 VM (systemd 229) and NixOS unstable (systemd 230) indicates that systemctl list-unit-files --root=... actually follows a symlinked <root>/etc/systemd/system path. I think the culprit is unit_file_get_list(), which calls opendir() on the results from lookup_paths_init(). systemd doesn't appear to use find_symlinks() when given --root=....

Given the above, I believe this PR does not introduce any regression re --root.

[rimmington]

It also occurs to me that, even given O_NOFOLLOW, "Symbolic links in earlier components of the pathname will still be followed" (man 2 open). This could of course cause problems with --root, but perhaps the fix is worse than the problem.

[keszybz]

@rimmington You're right, current code is completely confused:

$ strace -efile ./systemctl list-unit-files --root=/var/lib/machines/fedora-rawhide/
...
open("/var/lib/machines/fedora-rawhide/etc/systemd/system", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC) = 4
readlinkat(AT_FDCWD, "/var/lib/machines/fedora-rawhide/etc/systemd/system/syslog.service", "/usr/lib/systemd/system/rsyslog."..., 99) = 39
openat(4, "multi-user.target.wants", O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC) = 5
...

We should probably fix that, and then check that the patch in this PR does not regress things and apply it.

[keszybz]

Hm, no, actually this seems correct: the second path in readlinkat is the output parameter.

[keszybz]

OK, so this only changes the way we open config_path, and we don't really care if a config directory is a symlink or not. We only care whether a unit file in that config path is a symlink.