Re: [PATCH 2/2] pidfd: add pidfdfs

Re: [PATCH 2/2] pidfd: add pidfdfs

[Geert Uytterhoeven]

 	Hi Christian,

On Tue, 13 Feb 2024, Christian Brauner wrote:
> This moves pidfds from the anonymous inode infrastructure to a tiny
> pseudo filesystem. This has been on my todo for quite a while as it 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:
>
> * statx() on pidfds becomes useful for the first time.
> * pidfds can be compared simply via statx() and then comparing inode
>  numbers.
> * pidfds have unique inode numbers for the system lifetime.
> * struct pid is now stashed in inode->i_private instead of
>  file->private_data. This means it is now possible to introduce
>  concepts that operate on a process once all file descriptors have been
>  closed. A concrete example is kill-on-last-close.
> * file->private_data is freed up for per-file options for pidfds.
> * 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. Even
>  if we were to move to anon_inode_create_getfile() which creates new
>  inodes we'd still be associating the same struct pid with multiple
>  different inodes.
> * Pidfds now go through the regular dentry_open() path which means that
>  all security hooks are called unblocking proper LSM management for
>  pidfds. In addition fsnotify hooks are called and allow for listening
>  to open events on pidfds.
>
> 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.
>
> The code is entirely optional and fairly small. If it's not selected we
> fallback to anonymous inodes. Heavily inspired by nsfs which uses a
> similar stashing mechanism just for namespaces.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Thanks for your patch, which is now commit cb12fd8e0dabb9a1
("pidfd: add pidfs") upstream.

> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -174,6 +174,12 @@ source "fs/proc/Kconfig"
> source "fs/kernfs/Kconfig"
> source "fs/sysfs/Kconfig"
>
> +config FS_PIDFD
> +	bool "Pseudo filesystem for process file descriptors"
> +	depends on 64BIT

I think it would have been good if this dependency would have been
explained in the commit message.  I.e. why does this depend on 64BIT?

What is the risk this will not stay entirely optional?  I.e. can it
become a requirement for modern userspace, and thus be used as a stick
to kill support for 32-bit architectures?

> +	help
> +	  Pidfdfs implements advanced features for process file descriptors.
> +
> config TMPFS
> 	bool "Tmpfs virtual memory file system support (former shm fs)"
> 	depends on SHMEM

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH 2/2] pidfd: add pidfdfs

[Christian Brauner]

> What is the risk this will not stay entirely optional?  I.e. can it
> become a requirement for modern userspace, and thus be used as a stick
> to kill support for 32-bit architectures?

Yeah, Linus has requested to remove the config option.
I'm about to send him a patch.