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.

Sign up for GitHub

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

Jump to bottom

logind: use pidfd to refer to session leader #28938

Merged
merged 1 commit into from Oct 26, 2023
Merged

logind: use pidfd to refer to session leader #28938

merged 1 commit into from Oct 26, 2023

Conversation

msekletar
Copy link
Contributor

No description provided.

@msekletar
Copy link
Contributor Author

Another piece in the pidfdize eveything puzzle.

@bluca @poettering I'd like to get your thought on API and approach before adding docs, tests...

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 23, 2023
View reviewed changes
src/login/logind-dbus.c Fixed Show fixed Hide fixed
@yuwata yuwata added the logind label Aug 23, 2023
poettering
poettering reviewed Aug 23, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
poettering
poettering reviewed Aug 23, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
poettering
poettering reviewed Aug 23, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
poettering
poettering reviewed Aug 23, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
poettering
poettering reviewed Aug 23, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
poettering
poettering reviewed Aug 23, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
poettering
poettering reviewed Aug 23, 2023
View reviewed changes
src/login/pam_systemd.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member

so this looks OK, but is of course incomplete: logind passes the "leader" pid on to pid1 via the scope concept. But pid1 doesn't track scope processes via pidfd yet. Which is something we really should add.

So this is still racy, though the race is certainly smaller now. And I think it's OK to merge even if the fix in PID 1 is not coming soon.

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Aug 23, 2023
bluca
bluca reviewed Aug 27, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
bluca
bluca reviewed Aug 27, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
@bluca
Copy link
Member

bluca commented Aug 27, 2023

Another piece in the pidfdize eveything puzzle.

@bluca @poettering I'd like to get your thought on API and approach before adding docs, tests...

Yeah I think this is a great starting point and we should add this interface even if the story is not complete end-to-end

poettering
poettering reviewed Aug 30, 2023
View reviewed changes
src/login/logind-dbus.c Outdated Show resolved Hide resolved
poettering
poettering reviewed Aug 30, 2023
View reviewed changes
src/login/pam_systemd.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member

Now that #29142 has been merged this should be reworked around the new "PidRef" structure.

@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Oct 24, 2023
@msekletar
Copy link
Contributor Author

@poettering Updated. I fixed everything mentioned above and I've also added fallback in pam_systemd but I am not sure about the approach taken (helper struct in order to avoid passing a lot of parameters around). PTAL.

bluca
bluca reviewed Oct 24, 2023
View reviewed changes
src/login/pam_systemd.c Outdated Show resolved Hide resolved
@bluca

This comment was marked as resolved.

Sign in to view
@bluca bluca added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed please-review PR is ready for (re-)review by a maintainer labels Oct 24, 2023
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Oct 25, 2023
@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed documentation please-review PR is ready for (re-)review by a maintainer labels Oct 25, 2023
keszybz
keszybz reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

LGTM.

man/org.freedesktop.login1.xml Outdated Show resolved Hide resolved
@keszybz keszybz added needs-rebase and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Oct 26, 2023
@msekletar
logind: introduce CreateSessionWithPIDFD()
08edb48 
This new D-Bus API uses pidfd to refer to the session leader. Also,
pam_systemd will try to make use of it when pidfd support is available.
@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Oct 26, 2023
@bluca bluca merged commit 76f2191 into systemd:main Oct 26, 2023
47 of 48 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Oct 26, 2023
@bluca bluca removed the rc-blocker 🚧 PRs and Issues tagged this way are blocking the upcoming rc release! label Mar 26, 2024
@mrc0mmand mrc0mmand added login and removed logind labels Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keszybz keszybz keszybz left review comments

@bluca bluca bluca approved these changes

@poettering poettering poettering left review comments

@YHNdnzj YHNdnzj YHNdnzj left review comments

@DaanDeMeyer DaanDeMeyer DaanDeMeyer approved these changes

Assignees
No one assigned
Labels
documentation login util-lib
Milestone
v255
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@msekletar @poettering @bluca @keszybz @DaanDeMeyer @YHNdnzj @mrc0mmand @yuwata