Hey Linus,
/* Summary */
This contains updates for pidfds:
* Until now pidfds could only be created for thread-group leaders but
not for threads. There was no technical reason for this. We simply had
no users that needed support for this. Now we do have users that need
support for this.
This introduces a new PIDFD_THREAD flag for pidfd_open(). If that flag
is set pidfd_open() creates a pidfd that refers to a specific thread.
In addition, we now allow clone() and clone3() to be called with
CLONE_PIDFD | CLONE_THREAD which wasn't possible before.
A pidfd that refers to an individual thread differs from a pidfd that
refers to a thread-group leader:
(1) Pidfs are pollable. A task may poll a pidfd and get notified when
the task has exited.
For thread-group leader pidfds the polling task is woken if the
thread-group is empty. In other words, if the thread-group leader
task exits when there are still threads alive in its thread-group
the polling task will not be woken when the thread-group leader
exits but rather when the last thread in the thread-group exits.
For thread-specific pidfds the polling task is woken if the thread
exits.
(2) Passing a thread-group leader pidfd to pidfd_send_signal() will
generate thread-group directed signals like kill(2) does.
Passing a thread-specific pidfd to pidfd_send_signal() will
generate thread-specific signals like tgkill(2) does.
The default scope of the signal is thus determined by the type of
the pidfd.
Since use-cases exist where the default scope of the provided
pidfd needs to be overriden the following flags are added to
pidfd_send_signal():
- PIDFD_SIGNAL_THREAD
Send a thread-specific signal.
- PIDFD_SIGNAL_THREAD_GROUP
Send a thread-group directed signal.
- PIDFD_SIGNAL_PROCESS_GROUP
Send a process-group directed signal.
The scope change will only work if the struct pid is actually used
for this scope. For example, in order to send a thread-group
directed signal the provided pidfd must be used as a thread-group
leader and similarly for PIDFD_SIGNAL_PROCESS_GROUP the struct pid
must be used as a process group leader.
* Move pidfds from the anonymous inode infrastructure to a tiny
pseudo filesystem. This will unblock further work that we weren't able
to do simply because of the very justified limitations of anonymous
inodes. Moving pidfds to a tiny pseudo filesystem allows for statx on
pidfds to become useful for the first time. They can now be compared
by inode number which are unique for the system lifetime.
Instead of stashing struct pid in file->private_data we can now stash
it in inode->i_private. This makes it possible to introduce concepts
that operate on a process once all file descriptors have been closed.
A concrete example is kill-on-last-close. Another side-effect is that
file->private_data is now freed up for per-file options for pidfds.
Now, each struct pid will refer to a different inode but the same
struct pid will refer to the same inode if it's opened multiple times.
In contrast to now where each struct pid refers to the same inode.
The tiny pseudo filesystem is not visible anywhere in userspace
exactly like e.g., pipefs and sockfs. There's no lookup, there's no
complex inode operations, nothing. Dentries and inodes are always
deleted when the last pidfd is closed.
We allocate a new inode and dentry for each struct pid and we reuse
that inode and dentry for all pidfds that refer to the same struct
pid. The code is entirely optional and fairly small. If it's not
selected we fallback to anonymous inodes. Heavily inspired by nsfs.
The dentry and inode allocation mechanism is moved into generic
infrastructure that is now shared between nsfs and pidfs. The
path_from_stashed() helper must be provided with a stashing location,
an inode number, a mount, and the private data that is supposed to be
used and it will provide a path that can be passed to dentry_open().
The helper will try retrieve an existing dentry from the provided
stashing location. If a valid dentry is found it is reused. If not a
new one is allocated and we try to stash it in the provided location.
If this fails we retry until we either find an existing dentry or the
newly allocated dentry could be stashed. Subsequent openers of the
same namespace or task are then able to reuse it.
* Currently it is only possible to get notified when a task has exited,
i.e., become a zombie and userspace gets notified with EPOLLIN. We now
also support waiting until the task has been reaped, notifying
userspace with EPOLLHUP.
* Ensure that ESRCH is reported for getfd if a task is exiting instead
of the confusing EBADF.
* Various smaller cleanups to pidfd functions.
/* Testing */
clang: Debian clang version 16.0.6 (19)
gcc: (Debian 13.2.0-7) 13.2.0
All patches are based on v6.8-rc1 and have been sitting in linux-next.
No build failures or warnings were observed.
/* Conflicts */
Merge conflicts with other trees
================================
[1] linux-next: manual merge of the vfs-brauner tree with the mm tree
https://lore.kernel.org/linux-next/[email protected]
[2] linux-next: manual merge of the vfs-brauner tree with the cifs tree
https://lore.kernel.org/linux-next/[email protected]
Merge conflicts with mainline
=============================
No known conflicts.
The following changes since commit 6613476e225e090cc9aad49be7fa504e290dd33d:
Linux 6.8-rc1 (2024-01-21 14:11:32 -0800)
are available in the Git repository at:
[email protected]:pub/scm/linux/kernel/git/vfs/vfs tags/vfs-6.9.pidfd
for you to fetch changes up to e9c5263ce16d96311c118111ac779f004be8b473:
libfs: improve path_from_stashed() (2024-03-01 22:31:40 +0100)
Please consider pulling these changes from the signed vfs-6.9.pidfd tag.
Thanks!
Christian
----------------------------------------------------------------
vfs-6.9.pidfd
----------------------------------------------------------------
Christian Brauner (9):
pidfd: allow to override signal scope in pidfd_send_signal()
pidfd: move struct pidfd_fops
pidfd: add pidfs
libfs: add path_from_stashed()
nsfs: convert to path_from_stashed() helper
pidfs: convert to path_from_stashed() helper
libfs: improve path_from_stashed() helper
libfs: add stashed_dentry_prune()
libfs: improve path_from_stashed()
Oleg Nesterov (11):
pidfd: cleanup the usage of __pidfd_prepare's flags
pidfd: don't do_notify_pidfd() if !thread_group_empty()
pidfd: implement PIDFD_THREAD flag for pidfd_open()
pidfd_poll: report POLLHUP when pid_task() == NULL
pidfd: kill the no longer needed do_notify_pidfd() in de_thread()
pid: kill the obsolete PIDTYPE_PID code in transfer_pid()
pidfd: change do_notify_pidfd() to use __wake_up(poll_to_key(EPOLLIN))
pidfd: exit: kill the no longer used thread_group_exited()
pidfd: clone: allow CLONE_THREAD | CLONE_PIDFD together
signal: fill in si_code in prepare_kill_siginfo()
pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
Tycho Andersen (2):
pidfd: getfd should always report ESRCH if a task is exiting
selftests: add ESRCH tests for pidfd_getfd()
Wang Jinchao (1):
fork: Using clone_flags for legacy clone check
fs/Kconfig | 7 +
fs/Makefile | 2 +-
fs/exec.c | 1 -
fs/internal.h | 7 +
fs/libfs.c | 142 +++++++++++
fs/nsfs.c | 121 +++-------
fs/pidfs.c | 290 +++++++++++++++++++++++
include/linux/ns_common.h | 2 +-
include/linux/pid.h | 10 +-
include/linux/pidfs.h | 9 +
include/linux/proc_ns.h | 2 +-
include/linux/sched/signal.h | 2 -
include/uapi/linux/magic.h | 1 +
include/uapi/linux/pidfd.h | 8 +-
init/main.c | 2 +
kernel/exit.c | 31 +--
kernel/fork.c | 147 ++----------
kernel/nsproxy.c | 2 +-
kernel/pid.c | 57 +++--
kernel/signal.c | 110 ++++++---
tools/testing/selftests/pidfd/pidfd_getfd_test.c | 31 ++-
21 files changed, 686 insertions(+), 298 deletions(-)
create mode 100644 fs/pidfs.c
create mode 100644 include/linux/pidfs.h
The pull request you sent on Fri, 8 Mar 2024 11:13:50 +0100:
> [email protected]:pub/scm/linux/kernel/git/vfs/vfs tags/vfs-6.9.pidfd
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/b5683a37c881e2e08065f1670086e281430ee19f
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On Fri, 8 Mar 2024 at 02:14, Christian Brauner <[email protected]> wrote:
>
> * Move pidfds from the anonymous inode infrastructure to a tiny
> pseudo filesystem. This will unblock further work that we weren't able
> to do simply because of the very justified limitations of anonymous
> inodes. Moving pidfds to a tiny pseudo filesystem allows for statx on
> pidfds to become useful for the first time. They can now be compared
> by inode number which are unique for the system lifetime.
So I obviously pulled this already, but I did have one question - we
don't make nsfs conditional, and I'm not convinced we should make
pidfs conditional either.
I think (and *hope*) all the semantic annoyances got sorted out, and I
don't think there are any realistic size advantages to not enabling
CONFIG_FS_PID.
Is there some fundamental reason for that config entry to exist?
Linus
On Mon, Mar 11, 2024 at 01:05:06PM -0700, Linus Torvalds wrote:
> On Fri, 8 Mar 2024 at 02:14, Christian Brauner <[email protected]> wrote:
> >
> > * Move pidfds from the anonymous inode infrastructure to a tiny
> > pseudo filesystem. This will unblock further work that we weren't able
> > to do simply because of the very justified limitations of anonymous
> > inodes. Moving pidfds to a tiny pseudo filesystem allows for statx on
> > pidfds to become useful for the first time. They can now be compared
> > by inode number which are unique for the system lifetime.
>
> So I obviously pulled this already, but I did have one question - we
> don't make nsfs conditional, and I'm not convinced we should make
> pidfs conditional either.
>
> I think (and *hope*) all the semantic annoyances got sorted out, and I
> don't think there are any realistic size advantages to not enabling
> CONFIG_FS_PID.
>
> Is there some fundamental reason for that config entry to exist?
No, the size of struct pid was the main reason but I don't think it
matters. A side-effect was that we could easily enforce 64bit inode
numbers. But realistically it's trivial enough to workaround. Here's a
patch for what I think is pretty simple appended. Does that work?
On Tue, 12 Mar 2024 at 07:16, Christian Brauner <[email protected]> wrote:
>
> No, the size of struct pid was the main reason but I don't think it
> matters. A side-effect was that we could easily enforce 64bit inode
> numbers. But realistically it's trivial enough to workaround. Here's a
> patch for what I think is pretty simple appended. Does that work?
This looks eminently sane to me. Not that I actually _tested_it, but
since my testing would have compared it to my current setup (64-bit
and CONFIG_FS_PID=y) any testing would have been pointless because
that case didn't change.
Looking at the patch, I do wonder how much we even care about 64-bit
inodes. I'd like to point out how 'path_from_stashed()' only takes a
'unsigned long ino' anyway, and I don't think anything really cares
about either the high bits *or* the uniqueness of that inode number..
And similarly, i_ino isn't actually *used* for anything but naming to
user space.
So I'm not at all sure the whole 64-bit checks are worth it. Am I
missing something else?
Linus
On Tue, Mar 12, 2024 at 09:23:55AM -0700, Linus Torvalds wrote:
> On Tue, 12 Mar 2024 at 07:16, Christian Brauner <[email protected]> wrote:
> >
> > No, the size of struct pid was the main reason but I don't think it
> > matters. A side-effect was that we could easily enforce 64bit inode
> > numbers. But realistically it's trivial enough to workaround. Here's a
> > patch for what I think is pretty simple appended. Does that work?
>
> This looks eminently sane to me. Not that I actually _tested_it, but
> since my testing would have compared it to my current setup (64-bit
> and CONFIG_FS_PID=y) any testing would have been pointless because
> that case didn't change.
>
> Looking at the patch, I do wonder how much we even care about 64-bit
> inodes. I'd like to point out how 'path_from_stashed()' only takes a
> 'unsigned long ino' anyway, and I don't think anything really cares
> about either the high bits *or* the uniqueness of that inode number..
>
> And similarly, i_ino isn't actually *used* for anything but naming to
> user space.
It's used to compare pidfs and someone actually already sent a pull
request for this to another project iirc. So it'd be good to keep that
property.
But if your point is that we don't care about this for 32bit then I do
agree. We could do away with the checks completely and just accept the
truncation for 32bit. If that's your point feel free to just remove the
32bit handling in the patch and apply it. Let me know. Maybe I
misunderstood.
>
> So I'm not at all sure the whole 64-bit checks are worth it. Am I
> missing something else?
On Tue, 12 Mar 2024 at 13:09, Christian Brauner <[email protected]> wrote:
>
> It's used to compare pidfs and someone actually already sent a pull
> request for this to another project iirc. So it'd be good to keep that
> property.
Hmm. If people really do care, I guess we should spend the effort on
making those things unique.
> But if your point is that we don't care about this for 32bit then I do
> agree. We could do away with the checks completely and just accept the
> truncation for 32bit. If that's your point feel free to just remove the
> 32bit handling in the patch and apply it. Let me know. Maybe I
> misunderstood.
I personally don't care about 32-bit any more, but it also feels wrong
to just say that it's ok depending on something on a 64-bit kernel,
but not a 32-bit one.
So let's go with your patch. It's not like it's a problem to spend the
(very little) extra effort to do a 64-bit inode number.
Linus
On Tue, Mar 12, 2024 at 01:21:34PM -0700, Linus Torvalds wrote:
> On Tue, 12 Mar 2024 at 13:09, Christian Brauner <[email protected]> wrote:
> >
> > It's used to compare pidfs and someone actually already sent a pull
> > request for this to another project iirc. So it'd be good to keep that
> > property.
>
> Hmm. If people really do care, I guess we should spend the effort on
> making those things unique.
So, I cleaned that patch and took the chance to simplify things a bit
more and now we give the same guarantees for 32bit and 64bit. We're
still removing a lot more code than we add. I provided a detailed
description and some already existing users.
If you're fine with it I would ask you to please just apply it or if you
prefer I can send a pull request tomorrow. I'm still stuck at the
doctor's office.
I spent most of my day compiling and test i386 kernel and userspace.
Surprisingly, trauma isn't the best teacher because I had completely
forgotten how horrible it all is and I'm glad I'm back in a world where
64bit is a thing.
On Wed, 13 Mar 2024 at 10:10, Christian Brauner <[email protected]> wrote:
>
> If you're fine with it I would ask you to please just apply it [..]
I'll take it directly, no problem.
Thanks,
Linus