Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logind: register PAM sessions via Varlink instead of D-Bus #35264

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

poettering
Copy link
Member

This makes things a bit faster (because it cuts down a bit on roundtrips) and prepares ground so that one day we can let logind run in earlier boot already, making it a bit less special.

communication between logind and pid1 is still dbus only, hence there's a lot of room for further improvement I guess.

@poettering poettering added this to the v258 milestone Nov 20, 2024
Comment on lines +102 to +104
_cleanup_close_ int fifo_fd = session_create_fifo(s);
if (fifo_fd < 0)
return fifo_fd;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, hmm, I'm quite inclined to just leave this whole fifo stuff behind when we're embracing the new varlink and pidfd world. We actually stop the session as soon as the session leader exits when listening on pidfd, so the fifo is useless here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, right now pidfd (and in particular SO_PEERPIDFD is not in our kernel baseline yet. I am not sure we can do this already.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to bump our baseline to v5.4 in v258, so I'd really appreciate it if we just send logind our pidfd, and drop the fifo concept altogether (I can handle the latter part later)

@poettering poettering force-pushed the pam-systemd-varlink branch 4 times, most recently from 4baee2d to 67591fd Compare November 22, 2024 23:21
needs-rebase and removed please-review PR is ready for (re-)review by a maintainer labels Dec 12, 2024
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed needs-rebase labels Dec 17, 2024
The new json_dispatch_const_path() is to json_dispatch_path() what
sd_json_dispatch_const_string() is to sd_json_dispatch_ string(), i.e.
doesn't implicitly strdup() the string, but gives you the pointer into
the JSON structure, and thus requires you to keep it pinned.
This combines getpeercred() and getpeerpidfd() and returns a PidRef
…ith fd passing enabled

Let's add a simple flag that enables fd passing for all connections of a
server. It's much easier to use this than to install a connect handler
which manually enables this for each connection.
We can pass a properly typed Manager object here, no reason to pass it
as void*.
Just some refactoring to make an overly large function a bit smaller.
…k codepaths

This separates the preparatory checks that generate D-Bus errors from
the code that actually allocates the session. This make the logic easier
to follow and prepares ground so that we can reuse the 2nd part later
when exposing session creation via Varlink.
This prepares ground so that later on we can reply with either D-Bus or
Varlink depending on the client's request.
For now this only covers CreateSession() and ReleaseSession(), i.e. the
two operations pam_systemd cares about.
This makes sure we now use Varlink per default as transport for
allocating sessions.

This reduces the time it takes to do one run0 cycle by roughly ~10% on my
completely synthetic test setup (assuming the target user's service
manager is already started)

The D-Bus codepaths are kept in place for two reasons:
* To make upgrades easy
* If the user actually sets resource properties on the PAM session we
  fall back to the D-Bus codepaths, as we currently have no way to
  encode the scope properties in JSON, this is only supported for D-Bus
  serialization.

The latter should be revisited once it is possible to allocate a scope
unit from PID1 via varlink.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants