Re: [PATCH 4/4] pidfs: implement fh_to_dentry

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2024-11-01 at 13:54 +0000, Erin Shepherd wrote:
> This enables userspace to use name_to_handle_at to recover a pidfd
> to a process.
> 
> We stash the process' PID in the root pid namespace inside the handle,
> and use that to recover the pid (validating that pid->ino matches the
> value in the handle, i.e. that the pid has not been reused).
> 
> We use the root namespace in order to ensure that file handles can be
> moved across namespaces; however, we validate that the PID exists in
> the current namespace before returning the inode.
> 
> Signed-off-by: Erin Shepherd <erin.shepherd@e43.eu>
> ---
>  fs/pidfs.c | 50 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index c8e7e9011550..2d66610ef385 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -348,23 +348,59 @@ static const struct dentry_operations pidfs_dentry_operations = {
>  	.d_prune	= stashed_dentry_prune,
>  };
>  
> -static int pidfs_encode_fh(struct inode *inode, __u32 *fh, int *max_len,
> +#define PIDFD_FID_LEN 3
> +
> +struct pidfd_fid {
> +	u64 ino;
> +	s32 pid;
> +} __packed;
> +
> +static int pidfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>  			   struct inode *parent)
>  {
>  	struct pid *pid = inode->i_private;
> -	
> -	if (*max_len < 2) {
> -		*max_len = 2;
> +	struct pidfd_fid *fid = (struct pidfd_fid *)fh;
> +
> +	if (*max_len < PIDFD_FID_LEN) {
> +		*max_len = PIDFD_FID_LEN;
>  		return FILEID_INVALID;
>  	}
>  
> -	*max_len = 2;
> -	*(u64 *)fh = pid->ino;
> -	return FILEID_KERNFS;
> +	fid->ino = pid->ino;
> +	fid->pid = pid_nr(pid);

I worry a little here. A container being able to discover its pid in
the root namespace seems a little sketchy. This makes that fairly
simple to figure out.

Maybe generate a random 32 bit value at boot time, and then xor that
into this? Then you could just reverse that and look up the pid.

> +	*max_len = PIDFD_FID_LEN;
> +	return FILEID_INO64_GEN;
> +}
> +
> +static struct dentry *pidfs_fh_to_dentry(struct super_block *sb,
> +					 struct fid *gen_fid,
> +					 int fh_len, int fh_type)
> +{
> +	int ret;
> +	struct path path;
> +	struct pidfd_fid *fid = (struct pidfd_fid *)gen_fid;
> +	struct pid *pid;
> +
> +	if (fh_type != FILEID_INO64_GEN || fh_len < PIDFD_FID_LEN)
> +		return NULL;
> +
> +	pid = find_get_pid_ns(fid->pid, &init_pid_ns);
> +	if (!pid || pid->ino != fid->ino || pid_vnr(pid) == 0) {
> +		put_pid(pid);
> +		return NULL;
> +	}
> +
> +	ret = path_from_stashed(&pid->stashed, pidfs_mnt, pid, &path);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	mntput(path.mnt);
> +	return path.dentry;
>  }
>  
>  static const struct export_operations pidfs_export_operations = {
>  	.encode_fh = pidfs_encode_fh,
> +	.fh_to_dentry = pidfs_fh_to_dentry,
>  };
>  
>  static int pidfs_init_inode(struct inode *inode, void *data)

-- 
Jeff Layton <jlayton@kernel.org>