Re: [PATCH] pidfs: preserve thread pidfds reopened by file handle

[Jan Kara]

On Wed 22-07-26 14:07:37, Christian Brauner wrote:
> On 2026-07-16 13:11 +0200, Jan Kara wrote:
> > On Thu 16-07-26 13:27:22, Li Chen wrote:
> > > PIDFD_THREAD shares O_EXCL. do_dentry_open() clears O_EXCL after
> > > pidfs_export_open() validates the flags, so open_by_handle_at()
> > > silently turns a thread pidfd into a process pidfd.
> > > 
> > > Restore PIDFD_THREAD on the opened file, matching pidfs_alloc_file(),
> > > and cover the restored flag with F_GETFL.
> > > 
> > > Signed-off-by: Li Chen <me@linux.beauty>
> > 
> > Makes sense. Just looking at this code, what about PIDFD_AUTOKILL (aka
> > O_TRUNC?)? Reading the commit introducing it 07c3ef58223e it appears it was
> > never meant to be directly requested by userspace so probably
> > pidfs_export_open() needs to reject it. But that's unrelated issue to your
> > patch so feel free to add:
> 
> Good point. But pidfs_export_open() is guarded by
> pidfs_export_permission() which does:
> 
> #define VALID_FILE_HANDLE_OPEN_FLAGS \
> 	(O_RDONLY | O_WRONLY | O_RDWR | O_NONBLOCK | O_CLOEXEC | O_EXCL)
> 
> 	if (oflags & ~(VALID_FILE_HANDLE_OPEN_FLAGS | O_LARGEFILE))
> 		return -EINVAL;
> 
> and so is safe. But I've added the following patch on top which also
> splats if that is passed!
> 
> From 99a859485394eca8cf6271de09ae72ac3b846bda Mon Sep 17 00:00:00 2001
> From: Christian Brauner <brauner@xxxxxxxxxx>
> Date: Wed, 22 Jul 2026 13:56:28 +0200
> Subject: [PATCH] pidfs: add pidfs_dentry_open() helper
> 
> Both pidfs_alloc_file() and pidfs_export_open() need to force O_RDWR
> and reapply the pidfd flags that do_dentry_open() strips. Move the
> common logic into a helper.
> 
> PIDFD_AUTOKILL is now part of the restore mask in the file handle path
> as well, but pidfs_export_permission() rejects O_TRUNC, so this is a
> no-op there. But warn nonetheless.
> 
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>

Nice! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index c20ffd747ff5..695215aa2a58 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -915,6 +915,20 @@ static struct dentry *pidfs_fh_to_dentry(struct super_block *sb,
>  	return path.dentry;
>  }
>  
> +static struct file *pidfs_dentry_open(const struct path *path,
> +				      unsigned int flags,
> +				      const struct cred *cred)
> +{
> +	struct file *file;
> +
> +	/* pidfds are always O_RDWR. */
> +	file = dentry_open(path, flags | O_RDWR, cred);
> +	/* do_dentry_open() strips O_EXCL and O_TRUNC. */
> +	if (!IS_ERR(file))
> +		file->f_flags |= flags & (PIDFD_THREAD | PIDFD_AUTOKILL);
> +	return file;
> +}
> +
>  /*
>   * Make sure that we reject any nonsensical flags that users pass via
>   * open_by_handle_at(). Note that PIDFD_THREAD is defined as O_EXCL, and
> @@ -939,18 +953,14 @@ static int pidfs_export_permission(struct handle_to_path_ctx *ctx,
>  
>  static struct file *pidfs_export_open(const struct path *path, unsigned int oflags)
>  {
> -	struct file *file;
> -
>  	/*
> -	 * Clear O_LARGEFILE as open_by_handle_at() forces it and raise
> -	 * O_RDWR as pidfds always are.
> +	 * Opening via file handle may never raise PIDFD_AUTOKILL. That can
> +	 * only be done at task creation!
>  	 */
> -	oflags &= ~O_LARGEFILE;
> -	file = dentry_open(path, oflags | O_RDWR, current_cred());
> -	/* do_dentry_open() strips O_EXCL, which encodes PIDFD_THREAD. */
> -	if (!IS_ERR(file))
> -		file->f_flags |= oflags & PIDFD_THREAD;
> -	return file;
> +	if (WARN_ON_ONCE(oflags & PIDFD_AUTOKILL))
> +		return ERR_PTR(-EINVAL);
> +	/* Clear O_LARGEFILE as open_by_handle_at() forces it. */
> +	return pidfs_dentry_open(path, oflags & ~O_LARGEFILE, current_cred());
>  }
>  
>  static const struct export_operations pidfs_export_operations = {
> @@ -1114,7 +1124,6 @@ static struct file_system_type pidfs_type = {
>  
>  struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
>  {
> -	struct file *pidfd_file;
>  	struct path path __free(path_put) = {};
>  	int ret;
>  
> @@ -1132,16 +1141,7 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
>  	VFS_WARN_ON_ONCE(!pid->attr);
>  
>  	flags &= ~PIDFD_STALE;
> -	flags |= O_RDWR;
> -	pidfd_file = dentry_open(&path, flags, current_cred());
> -	/*
> -	 * Raise PIDFD_THREAD and PIDFD_AUTOKILL explicitly as
> -	 * do_dentry_open() strips O_EXCL and O_TRUNC.
> -	 */
> -	if (!IS_ERR(pidfd_file))
> -		pidfd_file->f_flags |= (flags & (PIDFD_THREAD | PIDFD_AUTOKILL));
> -
> -	return pidfd_file;
> +	return pidfs_dentry_open(&path, flags, current_cred());
>  }
>  
>  void __init pidfs_init(void)
> -- 
> 2.53.0
> 
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR