systemd icon indicating copy to clipboard operation
systemd copied to clipboard

core/mount: unmount credential directory with relevant mount unit

Open yuwata opened this issue 10 months ago • 4 comments

Replaces 1e1225614ca1106116dcad9fb37aaeb6106408ab.

When exec_context_destroy_credentials() -> umount2() succeeds, the relevant .mount unit may be already already in deactivating but umount command may not be invoked yet. In such case, the later invoked umount command will fail.

To avoid such simultaneous unmounting of the credential directory, let's unmount it with the relevant .mount unit if exists, and call umount2() only when that fails or no relevant .mount unit exists.

Hopefully, this avoids an extra read of mountinfo file.

Prompted by https://github.com/systemd/systemd/issues/31337#issuecomment-2025848069.

yuwata avatar Mar 29 '24 02:03 yuwata

So there seems to be a race between rmdir through mount unit and sd-mkdcred for next service instance...

YHNdnzj avatar Apr 04 '24 09:04 YHNdnzj

In the light of #32799, the case where cred mount units race with unmounting through exec_context_destroy_credentials should be drastically decreased in number. Hence, I wonder if it's time to drop the mount_invalidate_state_by_path thing on its own.

The new logic in this PR is still nice to have IMO, but it requires bigger rework. Now that the original issue should be gone, let's simplify the existing logic first?

YHNdnzj avatar May 14 '24 09:05 YHNdnzj

so here's an idea: maybe we should replace the indiviudal creds mounts with a single fs shared for all services, where each service gets their own subdir.

what's nice about the current scheme is that we can populate the tmpfs and mark it read-only when done.

I guess we could implement similar behaviour if we have a shred cred mount: i.e. create a dir under a random name, then populate it. Then mark all files with the "immutable" flag (tmpfs supports that these days). Finally, when done, rename it to the final name.

This would probably come pretty close to the current behaviour (i.e. immutable files as replacement for MS_RDONLY), and be more efficient? and it wouldn't litter the system with so many mounts anymore.

(just an idea)

poettering avatar May 15 '24 18:05 poettering

so here's an idea: maybe we should replace the indiviudal creds mounts with a single fs shared for all services, where each service gets their own subdir.

Hmm, I think a great property of using individual mount is that one day we can use mount_beneath to implement cred refreshing on service reload.

YHNdnzj avatar May 15 '24 19:05 YHNdnzj