[GIT PULL] vfs pidfd

[GIT PULL] vfs pidfd

[Christian Brauner]

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/20240221103200.165d8cd5@canb.auug.org.au

[2] linux-next: manual merge of the vfs-brauner tree with the cifs tree
    https://lore.kernel.org/linux-next/20240226110343.28e340eb@canb.auug.org.au

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:

  git@gitolite.kernel.org: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

Re: [GIT PULL] vfs pidfd

[pr-tracker-bot]

The pull request you sent on Fri,  8 Mar 2024 11:13:50 +0100:

> git@gitolite.kernel.org: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

Re: [GIT PULL] vfs pidfd

[Linus Torvalds]

On Fri, 8 Mar 2024 at 02:14, Christian Brauner <brauner@kernel.org> 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

Re: [GIT PULL] vfs pidfd

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On Mon, Mar 11, 2024 at 01:05:06PM -0700, Linus Torvalds wrote:
> On Fri, 8 Mar 2024 at 02:14, Christian Brauner <brauner@kernel.org> 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?

[-- Attachment #2: 0001-pidfs-remove-config-option.patch --]
[-- Type: text/x-diff, Size: 5463 bytes --]

From ce1c50a3d8d569be338f2a06f5e8470603038363 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Tue, 12 Mar 2024 10:39:44 +0100
Subject: [PATCH] pidfs: remove config option

Enable pidfs unconditionally. There's no real reason not do to it. One
of the really nice properties of pidfs is that we have unique inode
numbers for the system lifetime which allows userspace to do a bunch of
elegant things. So we should retain that property. So on arches where
inode number in the kernel are only 32bit we simply use get_next_ino()
and print the full value into fdinfo. On 64bit we do it cleanly and put
this into stat where it belongs.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/Kconfig            |  7 ------
 fs/pidfs.c            | 50 ++++++-------------------------------------
 include/linux/pid.h   |  4 +---
 include/linux/pidfs.h |  1 -
 kernel/pid.c          |  4 ----
 5 files changed, 7 insertions(+), 59 deletions(-)

diff --git a/fs/Kconfig b/fs/Kconfig
index f3dbd84a0e40..89fdbefd1075 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -174,13 +174,6 @@ source "fs/proc/Kconfig"
 source "fs/kernfs/Kconfig"
 source "fs/sysfs/Kconfig"
 
-config FS_PID
-	bool "Pseudo filesystem for process file descriptors"
-	depends on 64BIT
-	default y
-	help
-	  Pidfs implements advanced features for process file descriptors.
-
 config TMPFS
 	bool "Tmpfs virtual memory file system support (former shm fs)"
 	depends on SHMEM
diff --git a/fs/pidfs.c b/fs/pidfs.c
index 8fd71a00be9c..677fa2f1bbbb 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -16,17 +16,6 @@
 
 #include "internal.h"
 
-static int pidfd_release(struct inode *inode, struct file *file)
-{
-#ifndef CONFIG_FS_PID
-	struct pid *pid = file->private_data;
-
-	file->private_data = NULL;
-	put_pid(pid);
-#endif
-	return 0;
-}
-
 #ifdef CONFIG_PROC_FS
 /**
  * pidfd_show_fdinfo - print information about a pidfd
@@ -89,6 +78,9 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 		for (i = ns->level + 1; i <= pid->level; i++)
 			seq_put_decimal_ll(m, "\t", pid->numbers[i].nr);
 	}
+#endif
+#if BITS_PER_LONG == 32
+	seq_put_decimal_ll(m, "\nPidfsId:\t", pid->ino);
 #endif
 	seq_putc(m, '\n');
 }
@@ -120,7 +112,6 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
 }
 
 static const struct file_operations pidfs_file_operations = {
-	.release	= pidfd_release,
 	.poll		= pidfd_poll,
 #ifdef CONFIG_PROC_FS
 	.show_fdinfo	= pidfd_show_fdinfo,
@@ -131,14 +122,9 @@ struct pid *pidfd_pid(const struct file *file)
 {
 	if (file->f_op != &pidfs_file_operations)
 		return ERR_PTR(-EBADF);
-#ifdef CONFIG_FS_PID
 	return file_inode(file)->i_private;
-#else
-	return file->private_data;
-#endif
 }
 
-#ifdef CONFIG_FS_PID
 static struct vfsmount *pidfs_mnt __ro_after_init;
 
 /*
@@ -200,6 +186,9 @@ static void pidfs_init_inode(struct inode *inode, void *data)
 	inode->i_mode |= S_IRWXU;
 	inode->i_op = &pidfs_inode_operations;
 	inode->i_fop = &pidfs_file_operations;
+#if BITS_PER_LONG == 32
+	inode->i_ino = get_next_ino();
+#endif
 }
 
 static void pidfs_put_data(void *data)
@@ -261,30 +250,3 @@ void __init pidfs_init(void)
 	if (IS_ERR(pidfs_mnt))
 		panic("Failed to mount pidfs pseudo filesystem");
 }
-
-bool is_pidfs_sb(const struct super_block *sb)
-{
-	return sb == pidfs_mnt->mnt_sb;
-}
-
-#else /* !CONFIG_FS_PID */
-
-struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
-{
-	struct file *pidfd_file;
-
-	pidfd_file = anon_inode_getfile("[pidfd]", &pidfs_file_operations, pid,
-					flags | O_RDWR);
-	if (IS_ERR(pidfd_file))
-		return pidfd_file;
-
-	get_pid(pid);
-	return pidfd_file;
-}
-
-void __init pidfs_init(void) { }
-bool is_pidfs_sb(const struct super_block *sb)
-{
-	return false;
-}
-#endif
diff --git a/include/linux/pid.h b/include/linux/pid.h
index c79a0efd0258..ae0c0fd943c4 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -55,10 +55,8 @@ struct pid
 	refcount_t count;
 	unsigned int level;
 	spinlock_t lock;
-#ifdef CONFIG_FS_PID
 	struct dentry *stashed;
-	unsigned long ino;
-#endif
+	u64 ino;
 	/* lists of tasks that use this pid */
 	struct hlist_head tasks[PIDTYPE_MAX];
 	struct hlist_head inodes;
diff --git a/include/linux/pidfs.h b/include/linux/pidfs.h
index 40dd325a32a6..75bdf9807802 100644
--- a/include/linux/pidfs.h
+++ b/include/linux/pidfs.h
@@ -4,6 +4,5 @@
 
 struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags);
 void __init pidfs_init(void);
-bool is_pidfs_sb(const struct super_block *sb);
 
 #endif /* _LINUX_PID_FS_H */
diff --git a/kernel/pid.c b/kernel/pid.c
index 99a0c5eb24b8..8ced4e208c22 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -66,13 +66,11 @@ int pid_max = PID_MAX_DEFAULT;
 
 int pid_max_min = RESERVED_PIDS + 1;
 int pid_max_max = PID_MAX_LIMIT;
-#ifdef CONFIG_FS_PID
 /*
  * Pseudo filesystems start inode numbering after one. We use Reserved
  * PIDs as a natural offset.
  */
 static u64 pidfs_ino = RESERVED_PIDS;
-#endif
 
 /*
  * PID-map pages start out as NULL, they get allocated upon
@@ -280,10 +278,8 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
 	spin_lock_irq(&pidmap_lock);
 	if (!(ns->pid_allocated & PIDNS_ADDING))
 		goto out_unlock;
-#ifdef CONFIG_FS_PID
 	pid->stashed = NULL;
 	pid->ino = ++pidfs_ino;
-#endif
 	for ( ; upid >= pid->numbers; --upid) {
 		/* Make the PID visible to find_pid_ns. */
 		idr_replace(&upid->ns->idr, pid, upid->nr);
-- 
2.43.0

Re: [GIT PULL] vfs pidfd

[Linus Torvalds]

On Tue, 12 Mar 2024 at 07:16, Christian Brauner <brauner@kernel.org> 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

Re: [GIT PULL] vfs pidfd

[Christian Brauner]

On Tue, Mar 12, 2024 at 09:23:55AM -0700, Linus Torvalds wrote:
> On Tue, 12 Mar 2024 at 07:16, Christian Brauner <brauner@kernel.org> 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?

Re: [GIT PULL] vfs pidfd

[Linus Torvalds]

On Tue, 12 Mar 2024 at 13:09, Christian Brauner <brauner@kernel.org> 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

Re: [GIT PULL] vfs pidfd

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On Tue, Mar 12, 2024 at 01:21:34PM -0700, Linus Torvalds wrote:
> On Tue, 12 Mar 2024 at 13:09, Christian Brauner <brauner@kernel.org> 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.

[-- Attachment #2: 0001-pidfs-remove-config-option.patch --]
[-- Type: text/x-diff, Size: 14078 bytes --]

From 6c72192dca868a9e04d23bbcfe0db1e35b7dd71f Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Tue, 12 Mar 2024 10:39:44 +0100
Subject: [PATCH] pidfs: remove config option

As Linus suggested this enables pidfs unconditionally. A key property to
retain is the ability to compare pidfds by inode number (cf. [1]).
That's extremely helpful just as comparing namespace file descriptors by
inode number is. They are used in a variety of scenarios where they need
to be compared, e.g., when receiving a pidfd via SO_PEERPIDFD from a
socket to trivially authenticate a the sender and various other
use-cases.

For 64bit systems this is pretty trivial to do. For 32bit it's slightly
more annoying as we discussed but we simply add a dumb ida based
allocator that gets used on 32bit. This gives the same guarantees about
inode numbers on 64bit without any overflow risk. Practically, we'll
never run into overflow issues because we're contstrained by the number
of processes that can exist on 32bit and by the number of open files
that can exist on a 32bit system. On 64bit none of this matters and
things are very simple.

If 32bit also needs the uniqueness guarantee they can simply parse the
contents of /proc/<pid>/fd/<nr>. The uniqueness guarantees have a
variety of use-cases. One of the most obvious ones is that they will
make pidfiles (or "pidfdfiles", I guess) reliable as the unique
identifier can be placed into there that won't be reycled. Also a
frequent request.

Note, I took the chance and simplified path_from_stashed() even further.
Instead of passing the inode number explicitly to path_from_stashed() we
let the filesystem handle that internally. So path_from_stashed() ends
up even simpler than it is now. This is also a good solution allowing
the cleanup code to be clean and consistent between 32bit and 64bit. The
cleanup path in prepare_anon_dentry() is also switched around so we put
the inode before the dentry allocation. This means we only have to call
the cleanup handler for the filesystem's inode data once and can rely
->evict_inode() otherwise.

Aside from having to have a bit of extra code for 32bit it actually ends
up a nice cleanup for path_from_stashed() imho.

Tested on both 32 and 64bit including error injection.

Link: https://github.com/systemd/systemd/pull/31713 [1]
Link: https://lore.kernel.org/r/20240312-dingo-sehnlich-b3ecc35c6de7@brauner
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/Kconfig            |   7 ---
 fs/internal.h         |   6 +--
 fs/libfs.c            |  33 +++++++-------
 fs/nsfs.c             |  11 +++--
 fs/pidfs.c            | 101 ++++++++++++++++++++----------------------
 include/linux/pid.h   |   6 +--
 include/linux/pidfs.h |   1 -
 kernel/pid.c          |   6 ---
 8 files changed, 78 insertions(+), 93 deletions(-)

diff --git a/fs/Kconfig b/fs/Kconfig
index f3dbd84a0e40..89fdbefd1075 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -174,13 +174,6 @@ source "fs/proc/Kconfig"
 source "fs/kernfs/Kconfig"
 source "fs/sysfs/Kconfig"
 
-config FS_PID
-	bool "Pseudo filesystem for process file descriptors"
-	depends on 64BIT
-	default y
-	help
-	  Pidfs implements advanced features for process file descriptors.
-
 config TMPFS
 	bool "Tmpfs virtual memory file system support (former shm fs)"
 	depends on SHMEM
diff --git a/fs/internal.h b/fs/internal.h
index 7d3edcdf59cc..0a54b6dfaea2 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -312,8 +312,8 @@ struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap);
 void mnt_idmap_put(struct mnt_idmap *idmap);
 struct stashed_operations {
 	void (*put_data)(void *data);
-	void (*init_inode)(struct inode *inode, void *data);
+	int (*init_inode)(struct inode *inode, void *data);
 };
-int path_from_stashed(struct dentry **stashed, unsigned long ino,
-		      struct vfsmount *mnt, void *data, struct path *path);
+int path_from_stashed(struct dentry **stashed, struct vfsmount *mnt, void *data,
+		      struct path *path);
 void stashed_dentry_prune(struct dentry *dentry);
diff --git a/fs/libfs.c b/fs/libfs.c
index 65322e11bcda..686dbf6bb0c0 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1989,34 +1989,40 @@ static inline struct dentry *get_stashed_dentry(struct dentry *stashed)
 }
 
 static struct dentry *prepare_anon_dentry(struct dentry **stashed,
-					  unsigned long ino,
 					  struct super_block *sb,
 					  void *data)
 {
 	struct dentry *dentry;
 	struct inode *inode;
 	const struct stashed_operations *sops = sb->s_fs_info;
-
-	dentry = d_alloc_anon(sb);
-	if (!dentry)
-		return ERR_PTR(-ENOMEM);
+	int ret;
 
 	inode = new_inode_pseudo(sb);
 	if (!inode) {
-		dput(dentry);
+		sops->put_data(data);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	inode->i_ino = ino;
 	inode->i_flags |= S_IMMUTABLE;
 	inode->i_mode = S_IFREG;
 	simple_inode_init_ts(inode);
-	sops->init_inode(inode, data);
+
+	ret = sops->init_inode(inode, data);
+	if (ret < 0) {
+		iput(inode);
+		return ERR_PTR(ret);
+	}
 
 	/* Notice when this is changed. */
 	WARN_ON_ONCE(!S_ISREG(inode->i_mode));
 	WARN_ON_ONCE(!IS_IMMUTABLE(inode));
 
+	dentry = d_alloc_anon(sb);
+	if (!dentry) {
+		iput(inode);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	/* Store address of location where dentry's supposed to be stashed. */
 	dentry->d_fsdata = stashed;
 
@@ -2050,7 +2056,6 @@ static struct dentry *stash_dentry(struct dentry **stashed,
 /**
  * path_from_stashed - create path from stashed or new dentry
  * @stashed:    where to retrieve or stash dentry
- * @ino:        inode number to use
  * @mnt:        mnt of the filesystems to use
  * @data:       data to store in inode->i_private
  * @path:       path to create
@@ -2065,8 +2070,8 @@ static struct dentry *stash_dentry(struct dentry **stashed,
  *
  * Return: On success zero and on failure a negative error is returned.
  */
-int path_from_stashed(struct dentry **stashed, unsigned long ino,
-		      struct vfsmount *mnt, void *data, struct path *path)
+int path_from_stashed(struct dentry **stashed, struct vfsmount *mnt, void *data,
+		      struct path *path)
 {
 	struct dentry *dentry;
 	const struct stashed_operations *sops = mnt->mnt_sb->s_fs_info;
@@ -2079,11 +2084,9 @@ int path_from_stashed(struct dentry **stashed, unsigned long ino,
 	}
 
 	/* Allocate a new dentry. */
-	dentry = prepare_anon_dentry(stashed, ino, mnt->mnt_sb, data);
-	if (IS_ERR(dentry)) {
-		sops->put_data(data);
+	dentry = prepare_anon_dentry(stashed, mnt->mnt_sb, data);
+	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
-	}
 
 	/* Added a new dentry. @data is now owned by the filesystem. */
 	path->dentry = stash_dentry(stashed, dentry);
diff --git a/fs/nsfs.c b/fs/nsfs.c
index 7aaafb5cb9fc..07e22a15ef02 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c
@@ -56,7 +56,7 @@ int ns_get_path_cb(struct path *path, ns_get_path_helper_t *ns_get_cb,
 	if (!ns)
 		return -ENOENT;
 
-	return path_from_stashed(&ns->stashed, ns->inum, nsfs_mnt, ns, path);
+	return path_from_stashed(&ns->stashed, nsfs_mnt, ns, path);
 }
 
 struct ns_get_path_task_args {
@@ -101,8 +101,7 @@ int open_related_ns(struct ns_common *ns,
 		return PTR_ERR(relative);
 	}
 
-	err = path_from_stashed(&relative->stashed, relative->inum, nsfs_mnt,
-				relative, &path);
+	err = path_from_stashed(&relative->stashed, nsfs_mnt, relative, &path);
 	if (err < 0) {
 		put_unused_fd(fd);
 		return err;
@@ -199,11 +198,15 @@ static const struct super_operations nsfs_ops = {
 	.show_path = nsfs_show_path,
 };
 
-static void nsfs_init_inode(struct inode *inode, void *data)
+static int nsfs_init_inode(struct inode *inode, void *data)
 {
+	struct ns_common *ns = data;
+
 	inode->i_private = data;
 	inode->i_mode |= S_IRUGO;
 	inode->i_fop = &ns_file_operations;
+	inode->i_ino = ns->inum;
+	return 0;
 }
 
 static void nsfs_put_data(void *data)
diff --git a/fs/pidfs.c b/fs/pidfs.c
index 8fd71a00be9c..a63d5d24aa02 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -16,17 +16,6 @@
 
 #include "internal.h"
 
-static int pidfd_release(struct inode *inode, struct file *file)
-{
-#ifndef CONFIG_FS_PID
-	struct pid *pid = file->private_data;
-
-	file->private_data = NULL;
-	put_pid(pid);
-#endif
-	return 0;
-}
-
 #ifdef CONFIG_PROC_FS
 /**
  * pidfd_show_fdinfo - print information about a pidfd
@@ -120,7 +109,6 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
 }
 
 static const struct file_operations pidfs_file_operations = {
-	.release	= pidfd_release,
 	.poll		= pidfd_poll,
 #ifdef CONFIG_PROC_FS
 	.show_fdinfo	= pidfd_show_fdinfo,
@@ -131,16 +119,45 @@ struct pid *pidfd_pid(const struct file *file)
 {
 	if (file->f_op != &pidfs_file_operations)
 		return ERR_PTR(-EBADF);
-#ifdef CONFIG_FS_PID
 	return file_inode(file)->i_private;
-#else
-	return file->private_data;
-#endif
 }
 
-#ifdef CONFIG_FS_PID
 static struct vfsmount *pidfs_mnt __ro_after_init;
 
+#if BITS_PER_LONG == 32
+/*
+ * Provide a fallback mechanism for 32-bit systems so processes remain
+ * reliably comparable by inode number even on those systems.
+ */
+static DEFINE_IDA(pidfd_inum_ida);
+
+static int pidfs_inum(struct pid *pid, unsigned long *ino)
+{
+	int ret;
+
+	ret = ida_alloc_range(&pidfd_inum_ida, RESERVED_PIDS + 1,
+			      UINT_MAX, GFP_ATOMIC);
+	if (ret < 0)
+		return -ENOSPC;
+
+	*ino = ret;
+	return 0;
+}
+
+static inline void pidfs_free_inum(unsigned long ino)
+{
+	if (ino > 0)
+		ida_free(&pidfd_inum_ida, ino);
+}
+#else
+static inline int pidfs_inum(struct pid *pid, unsigned long *ino)
+{
+	*ino = pid->ino;
+	return 0;
+}
+#define pidfs_free_inum(ino) ((void)(ino))
+#endif
+
 /*
  * The vfs falls back to simple_setattr() if i_op->setattr() isn't
  * implemented. Let's reject it completely until we have a clean
@@ -173,6 +190,7 @@ static void pidfs_evict_inode(struct inode *inode)
 
 	clear_inode(inode);
 	put_pid(pid);
+	pidfs_free_inum(inode->i_ino);
 }
 
 static const struct super_operations pidfs_sops = {
@@ -183,8 +201,10 @@ static const struct super_operations pidfs_sops = {
 
 static char *pidfs_dname(struct dentry *dentry, char *buffer, int buflen)
 {
-	return dynamic_dname(buffer, buflen, "pidfd:[%lu]",
-			     d_inode(dentry)->i_ino);
+	struct inode *inode = d_inode(dentry);
+	struct pid *pid = inode->i_private;
+
+	return dynamic_dname(buffer, buflen, "pidfd:[%llu]", pid->ino);
 }
 
 static const struct dentry_operations pidfs_dentry_operations = {
@@ -193,13 +213,19 @@ static const struct dentry_operations pidfs_dentry_operations = {
 	.d_prune	= stashed_dentry_prune,
 };
 
-static void pidfs_init_inode(struct inode *inode, void *data)
+static int pidfs_init_inode(struct inode *inode, void *data)
 {
 	inode->i_private = data;
 	inode->i_flags |= S_PRIVATE;
 	inode->i_mode |= S_IRWXU;
 	inode->i_op = &pidfs_inode_operations;
 	inode->i_fop = &pidfs_file_operations;
+	/*
+	 * Inode numbering for pidfs start at RESERVED_PIDS + 1. This
+	 * avoids collisions with the root inode which is 1 for pseudo
+	 * filesystems.
+	 */
+	return pidfs_inum(data, &inode->i_ino);
 }
 
 static void pidfs_put_data(void *data)
@@ -240,13 +266,7 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
 	struct path path;
 	int ret;
 
-	/*
-	* Inode numbering for pidfs start at RESERVED_PIDS + 1.
-	* This avoids collisions with the root inode which is 1
-	* for pseudo filesystems.
-	 */
-	ret = path_from_stashed(&pid->stashed, pid->ino, pidfs_mnt,
-				get_pid(pid), &path);
+	ret = path_from_stashed(&pid->stashed, pidfs_mnt, get_pid(pid), &path);
 	if (ret < 0)
 		return ERR_PTR(ret);
 
@@ -261,30 +281,3 @@ void __init pidfs_init(void)
 	if (IS_ERR(pidfs_mnt))
 		panic("Failed to mount pidfs pseudo filesystem");
 }
-
-bool is_pidfs_sb(const struct super_block *sb)
-{
-	return sb == pidfs_mnt->mnt_sb;
-}
-
-#else /* !CONFIG_FS_PID */
-
-struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
-{
-	struct file *pidfd_file;
-
-	pidfd_file = anon_inode_getfile("[pidfd]", &pidfs_file_operations, pid,
-					flags | O_RDWR);
-	if (IS_ERR(pidfd_file))
-		return pidfd_file;
-
-	get_pid(pid);
-	return pidfd_file;
-}
-
-void __init pidfs_init(void) { }
-bool is_pidfs_sb(const struct super_block *sb)
-{
-	return false;
-}
-#endif
diff --git a/include/linux/pid.h b/include/linux/pid.h
index c79a0efd0258..a3aad9b4074c 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -45,6 +45,8 @@
  * find_pid_ns() using the int nr and struct pid_namespace *ns.
  */
 
+#define RESERVED_PIDS 300
+
 struct upid {
 	int nr;
 	struct pid_namespace *ns;
@@ -55,10 +57,8 @@ struct pid
 	refcount_t count;
 	unsigned int level;
 	spinlock_t lock;
-#ifdef CONFIG_FS_PID
 	struct dentry *stashed;
-	unsigned long ino;
-#endif
+	u64 ino;
 	/* lists of tasks that use this pid */
 	struct hlist_head tasks[PIDTYPE_MAX];
 	struct hlist_head inodes;
diff --git a/include/linux/pidfs.h b/include/linux/pidfs.h
index 40dd325a32a6..75bdf9807802 100644
--- a/include/linux/pidfs.h
+++ b/include/linux/pidfs.h
@@ -4,6 +4,5 @@
 
 struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags);
 void __init pidfs_init(void);
-bool is_pidfs_sb(const struct super_block *sb);
 
 #endif /* _LINUX_PID_FS_H */
diff --git a/kernel/pid.c b/kernel/pid.c
index 99a0c5eb24b8..da76ed1873f7 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -62,17 +62,13 @@ struct pid init_struct_pid = {
 
 int pid_max = PID_MAX_DEFAULT;
 
-#define RESERVED_PIDS		300
-
 int pid_max_min = RESERVED_PIDS + 1;
 int pid_max_max = PID_MAX_LIMIT;
-#ifdef CONFIG_FS_PID
 /*
  * Pseudo filesystems start inode numbering after one. We use Reserved
  * PIDs as a natural offset.
  */
 static u64 pidfs_ino = RESERVED_PIDS;
-#endif
 
 /*
  * PID-map pages start out as NULL, they get allocated upon
@@ -280,10 +276,8 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
 	spin_lock_irq(&pidmap_lock);
 	if (!(ns->pid_allocated & PIDNS_ADDING))
 		goto out_unlock;
-#ifdef CONFIG_FS_PID
 	pid->stashed = NULL;
 	pid->ino = ++pidfs_ino;
-#endif
 	for ( ; upid >= pid->numbers; --upid) {
 		/* Make the PID visible to find_pid_ns. */
 		idr_replace(&upid->ns->idr, pid, upid->nr);
-- 
2.43.0

Re: [GIT PULL] vfs pidfd

[Linus Torvalds]

On Wed, 13 Mar 2024 at 10:10, Christian Brauner <brauner@kernel.org> 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