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

[Jan Kara]

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:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/pidfs.c                                             | 8 +++++++-
>  tools/testing/selftests/pidfd/pidfd_file_handle_test.c | 1 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index aaa609ddab044..c20ffd747ff51 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -939,12 +939,18 @@ 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.
>  	 */
>  	oflags &= ~O_LARGEFILE;
> -	return dentry_open(path, oflags | O_RDWR, current_cred());
> +	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;
>  }
>  
>  static const struct export_operations pidfs_export_operations = {
> diff --git a/tools/testing/selftests/pidfd/pidfd_file_handle_test.c b/tools/testing/selftests/pidfd/pidfd_file_handle_test.c
> index 68918734dcf32..1e03ae9575fe6 100644
> --- a/tools/testing/selftests/pidfd/pidfd_file_handle_test.c
> +++ b/tools/testing/selftests/pidfd/pidfd_file_handle_test.c
> @@ -373,6 +373,7 @@ TEST_F(file_handle, open_by_handle_at_valid_flags)
>  				  O_CLOEXEC |
>  				  O_EXCL);
>  	ASSERT_GE(pidfd, 0);
> +	ASSERT_NE(fcntl(pidfd, F_GETFL) & PIDFD_THREAD, 0);
>  
>  	ASSERT_EQ(fstat(pidfd, &st2), 0);
>  	ASSERT_TRUE(st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR