polkitd: use PIDFDs if available to track processes - [merged]

[vmihalko]

In GitLab by @bluca on Apr 6, 2023, 02:15

Merges pidfd -> master

PIDs can be recycled, so when possible it is best to try and use PIDFDs, which are pinned. Change polkitd's unixprocess class so that, if the PIDFD syscall is available, it does not store a PID but only the PIDFD, and gets the PID when required on the fly (which will intentionally fail if the process has disappeared, so that we avoid recycling races).

In the future we will be able to get the PIDFD directly from D-Bus' GetConnectionCredentials() call, but for now get the FD from the PID. It does not completely close the window, but makes things significantly better already.

[vmihalko]

In GitLab by @bluca on Apr 6, 2023, 02:16

@jrybar split this out from https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/154 as it is useful by itself. It strictly improves the situation by taking a pidfd via pidfd_open. No external interface changes as the window is not fully closed until we can get a pidfd from D-Bus, but this is already useful by itself and by splitting it out it should be easier smaller and easier to review, I hope.

[vmihalko]

In GitLab by @bluca on Apr 13, 2023, 12:48

added 4 commits

• 83649e23...f8daee7c - 3 commits from branch polkit:master
• f15f861a - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on May 12, 2023, 19:47

added 3 commits

• f15f861a...4c6d183b - 2 commits from branch polkit:master
• a3916f42 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on May 24, 2023, 10:38

added 2 commits

• 5615ed04 - 1 commit from branch polkit:master
• d883272b - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on May 24, 2023, 15:12

added 2 commits

• 25eef55d - 1 commit from branch polkit:master
• 4b96e3c7 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on May 24, 2023, 15:13

@jrybar ping, any chance for a review? This PR does not require any new kernel interface and can be used without the other one.

[vmihalko]

In GitLab by @jrybar on May 24, 2023, 15:19

Hello Luca,
of course it has a chance and I will happily review this once I get to it. It's in the queue.

Nonetheless, thanks for your hard work and time!

[vmihalko]

In GitLab by @bluca on May 24, 2023, 15:21

Thanks!

[vmihalko]

In GitLab by @jrybar on Jun 30, 2023, 13:05

Hello Luca,
it is settled that this MR will be the highest priority after this summer's (probably mid-july) release of polkit.

[vmihalko]

In GitLab by @bluca on Jul 4, 2023, 20:53

added 2 commits

• 7ebf554e - 1 commit from branch polkit:master
• 04d8e5a0 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Jul 24, 2023, 11:54

added 2 commits

• f17b9ad5 - 1 commit from branch polkit:master
• ad559ed3 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Jul 30, 2023, 21:30

added 4 commits

• ad559ed3...c49c68e1 - 3 commits from branch polkit:master
• 402af760 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 1, 2023, 12:20

added 2 commits

• 4b7a5c35 - 1 commit from branch polkit:master
• 237850ee - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 2, 2023, 10:53

added 2 commits

• dae1bc5c - 1 commit from branch polkit:master
• 5c560b11 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 8, 2023, 15:04

@jrybar FYI the dbus-daemon change to add the new API has been merged: https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/398

[vmihalko]

In GitLab by @bluca on Aug 8, 2023, 21:00

added 3 commits

• 5c560b11...153cf209 - 2 commits from branch polkit:master
• 926efc87 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 11, 2023, 12:33

dbus-broker PR has been merged too, so now both D-Bus implementations will provide the required API in their next releases: https://github.com/bus1/dbus-broker/pull/312

[vmihalko]

In GitLab by @jrybar on Aug 16, 2023, 12:31

added 3 commits

• 926efc87...1d1cdb38 - 2 commits from branch polkit:master
• 520ae9fb - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 14:03

requested review from @jrybar

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 14:04

resolved all threads

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 14:12

Commented on src/polkit/polkitunixprocess.c line 290

Just a question: Since we're really just looking for "^Pid:", wouldn't it be faster to just g_strstr() on *contents?

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 14:12

Commented on src/polkit/polkitunixprocess.c line 58

just a typo :)

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 14:12

I like the code, neat&tidy.

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 14:12

approved this merge request

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 14:23

Commented on src/polkit/polkitunixprocess.c line 290

It seemed more readable and less error-prone to go line-by-line, so that we can pass an individual buffer to sscanff or example. It's all small in-memory buffers, so I can't imagine it would make a measurable difference?

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 14:23

Commented on src/polkit/polkitunixprocess.c line 58

changed this line in version 13 of the diff

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 14:23

added 1 commit

• 7f0d7923 - polkitd: use PIDFDs if available to track processes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 14:23

Commented on src/polkit/polkitunixprocess.c line 58

whops, fixed - given it's just one typo, I just amended and force pushed

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 14:26

Commented on src/polkit/polkitunixprocess.c line 290

It just seems like a "cannonball on a fly" case where we just trash everything except the one line. But I think I can live with it :D

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 14:46

Commented on src/polkit/polkitunixprocess.c line 290

Yeah it's usually 4/5 lines and a couple dozen characters, so it normally fits in a single cache line. If you want me to change it I can, otherwise are we good to merge?

$ cat /proc/self/fdinfo/0
pos:    0
flags:  02002002
mnt_id: 25
ino:    6
$ cat /proc/self/fdinfo/0 | wc
      4       8      41

[vmihalko]

In GitLab by @jrybar on Aug 17, 2023, 15:27

Commented on src/polkit/polkitunixprocess.c line 290

I want to try one final run on Rawhide and then it's set to go in.

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 16:02

Commented on src/polkit/polkitunixprocess.c line 290

thanks! I have rebased https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/154 on master, it's now ready