Re: [PATCH 1/3] pidfs: fix PIDFD_THREAD flag loss when opening pidfds via file handles

[Jan Kara]

On Mon 20-04-26 15:32:35, Christian Brauner wrote:
> pidfs_export_open() calls dentry_open() which internally calls
> do_dentry_open() that strips O_EXCL from f_flags. Since PIDFD_THREAD is
> defined as O_EXCL, thread pidfds opened via open_by_handle_at() silently
> lose their thread-specific scope.
> 
> pidfs_alloc_file() already handles this by explicitly restoring the
> PIDFD_THREAD flag after dentry_open() returns. Factor the common
> dentry_open() + flag restoration into a shared pidfs_open_file() helper
> and use it from both call sites.
> 
> Without this fix, a pidfd obtained via open_by_handle_at() with O_EXCL
> will have PIDTYPE_TGID scope instead of PIDTYPE_PID scope, causing
> pidfd_send_signal() to deliver signals to the entire thread group
> instead of the specific thread.
> 
> Fixes: 30915e955528 ("pidfs: convert to path_from_stashed() helper")
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/pidfs.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index e3825ee246be..11eb53f3e50a 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -897,14 +897,28 @@ static int pidfs_export_permission(struct handle_to_path_ctx *ctx,
>  	return 0;
>  }
>  
> +/*
> + * Open a pidfs dentry. Pidfds are always O_RDWR and PIDFD_THREAD (O_EXCL)
> + * must be restored after dentry_open() as do_dentry_open() strips it.
> + */
> +static struct file *pidfs_open_file(const struct path *path, unsigned int flags)
> +{
> +	struct file *f;
> +
> +	flags |= O_RDWR;
> +	f = dentry_open(path, flags, current_cred());
> +	if (!IS_ERR(f))
> +		f->f_flags |= (flags & PIDFD_THREAD);
> +	return f;
> +}
> +
>  static struct file *pidfs_export_open(const struct path *path, unsigned int oflags)
>  {
>  	/*
> -	 * Clear O_LARGEFILE as open_by_handle_at() forces it and raise
> -	 * O_RDWR as pidfds always are.
> +	 * Clear O_LARGEFILE as open_by_handle_at() forces it.
>  	 */
>  	oflags &= ~O_LARGEFILE;
> -	return dentry_open(path, oflags | O_RDWR, current_cred());
> +	return pidfs_open_file(path, oflags);
>  }
>  
>  static const struct export_operations pidfs_export_operations = {
> @@ -1086,7 +1100,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;
>  
> @@ -1104,13 +1117,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 explicitly as do_dentry_open() strips it. */
> -	if (!IS_ERR(pidfd_file))
> -		pidfd_file->f_flags |= (flags & PIDFD_THREAD);
> -
> -	return pidfd_file;
> +	return pidfs_open_file(&path, flags);
>  }
>  
>  void __init pidfs_init(void)
> 
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR