[RFC PATCH 0/2] API for exporting connectable file handles to userspace

[RFC PATCH 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

Jeff,

These patches bring the NFS connectable file handles feature to
userspace servers.

They rely on Christian's and Aleksa's changes recently merged to v6.12.

It may not be the best timing for posting RFC patches in the middle of
the merge window and during LPC, but at least this gives you a chance to
gossip about how bad an idea this is with folks ;)

I am aware of the usability implications with connectable file handles,
which are not consistent throughout the inode lifetime (i.e. when moved
to another parent), but the nfsd feature does exists and some users (me)
are interested in exporting this feature to userspace.

The API I chose for encoding conenctable file handles is pretty
conventional (AT_HANDLE_CONNECTABLE).

The API I chose for decoding a connected fd is a bit whacky, but if
you let it sink, it could make sense - my use case is to examine an
object's current path given a previously connectable encoded file handle.

By requesting to open an O_PATH fd, relative to an O_PATH mount_fd,
I would like to get an error (ESTALE) if the path connecting mount_fd
to the would-be-opened fd is unknown.

Thought and flames are welcome.

Thanks,
Amir.


Amir Goldstein (2):
  fs: name_to_handle_at() support for connectable file handles
  fs: open_by_handle_at() support for decoding connectable file handles

 fs/fhandle.c               | 85 +++++++++++++++++++++++++-------------
 include/uapi/linux/fcntl.h |  1 +
 2 files changed, 58 insertions(+), 28 deletions(-)

-- 
2.34.1

[RFC PATCH 1/2] fs: name_to_handle_at() support for connectable file handles

[Amir Goldstein]

nfsd encodes "connectable" file handles for the subtree_check feature.
So far, userspace nfs server could not make use of this functionality.

Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
When used, the encoded file handle is "connectable".

Note that decoding a "connectable" file handle with open_by_handle_at(2)
is not guarandteed to return a "connected" fd (i.e. fd with known path).
A new opt-in API would be needed to guarantee a "connected" fd.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fhandle.c               | 24 ++++++++++++++++++++----
 include/uapi/linux/fcntl.h |  1 +
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8cb665629f4a..956d9b25d4f7 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -31,6 +31,11 @@ static long do_sys_name_to_handle(const struct path *path,
 	if (!exportfs_can_encode_fh(path->dentry->d_sb->s_export_op, fh_flags))
 		return -EOPNOTSUPP;
 
+	/* Do not encode a connectable handle for a disconnected dentry */
+	if (fh_flags & EXPORT_FH_CONNECTABLE &&
+	    path->dentry->d_flags & DCACHE_DISCONNECTED)
+		return -EACCES;
+
 	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
 		return -EFAULT;
 
@@ -45,7 +50,7 @@ static long do_sys_name_to_handle(const struct path *path,
 	/* convert handle size to multiple of sizeof(u32) */
 	handle_dwords = f_handle.handle_bytes >> 2;
 
-	/* we ask for a non connectable maybe decodeable file handle */
+	/* Encode a possibly decodeable/connectable file handle */
 	retval = exportfs_encode_fh(path->dentry,
 				    (struct fid *)handle->f_handle,
 				    &handle_dwords, fh_flags);
@@ -109,15 +114,26 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 {
 	struct path path;
 	int lookup_flags;
-	int fh_flags;
+	int fh_flags = 0;
 	int err;
 
 	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
-		     AT_HANDLE_MNT_ID_UNIQUE))
+		     AT_HANDLE_MNT_ID_UNIQUE | AT_HANDLE_CONNECTABLE))
+		return -EINVAL;
+
+	/*
+	 * AT_HANDLE_FID means there is no intention to decode file handle
+	 * AT_HANDLE_CONNECTABLE means there is an intention to decode a
+	 * connected fd (with known path), so these flags are conflicting.
+	 */
+	if (flag & AT_HANDLE_CONNECTABLE && flag & AT_HANDLE_FID)
 		return -EINVAL;
+	else if (flag & AT_HANDLE_FID)
+		fh_flags |= EXPORT_FH_FID;
+	else if (flag & AT_HANDLE_CONNECTABLE)
+		fh_flags |= EXPORT_FH_CONNECTABLE;
 
 	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
-	fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0;
 	if (flag & AT_EMPTY_PATH)
 		lookup_flags |= LOOKUP_EMPTY;
 	err = user_path_at(dfd, name, lookup_flags, &path);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 87e2dec79fea..56ff2100e021 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -153,6 +153,7 @@
 					   object identity and may not be
 					   usable with open_by_handle_at(2). */
 #define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
+#define AT_HANDLE_CONNECTABLE	0x002	/* Request a connectable file handle */
 
 #if defined(__KERNEL__)
 #define AT_GETATTR_NOSEC	0x80000000
-- 
2.34.1

[RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Amir Goldstein]

Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2).
This was not allowed before, so we use it to enable a new API for
decoding "connectable" file handles that were created using the
AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2).

When mount fd is an O_PATH fd and decoding an O_PATH fd is requested,
use that as a hint to try to decode a "connected" fd with known path,
which is accessible (to capable user) from mount fd path.

Note that this does not check if the path is accessible to the calling
user, just that it is accessible wrt the mount namesapce, so if there
is no "connected" alias, or if parts of the path are hidden in the
mount namespace, open_by_handle_at(2) will return -ESTALE.

Note that the file handles used to decode a "connected" fd do not have
to be encoded with the AT_HANDLE_CONNECTABLE flag.  Specifically,
directory file handles are always "connectable", regardless of using
the AT_HANDLE_CONNECTABLE flag.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fhandle.c | 61 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 956d9b25d4f7..1fabfb79fd55 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 	return err;
 }
 
-static int get_path_from_fd(int fd, struct path *root)
+enum handle_to_path_flags {
+	HANDLE_CHECK_PERMS   = (1 << 0),
+	HANDLE_CHECK_SUBTREE = (1 << 1),
+};
+
+struct handle_to_path_ctx {
+	struct path root;
+	enum handle_to_path_flags flags;
+	unsigned int fh_flags;
+	unsigned int o_flags;
+};
+
+static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx)
 {
 	if (fd == AT_FDCWD) {
 		struct fs_struct *fs = current->fs;
 		spin_lock(&fs->lock);
-		*root = fs->pwd;
-		path_get(root);
+		ctx->root = fs->pwd;
+		path_get(&ctx->root);
 		spin_unlock(&fs->lock);
 	} else {
-		struct fd f = fdget(fd);
+		struct fd f = fdget_raw(fd);
 		if (!f.file)
 			return -EBADF;
-		*root = f.file->f_path;
-		path_get(root);
+		ctx->root = f.file->f_path;
+		path_get(&ctx->root);
+		/*
+		 * Use O_PATH mount fd and requested O_PATH fd as a hint for
+		 * decoding an fd with connected path, that is accessible from
+		 * the mount fd path.
+		 */
+		if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH)
+			ctx->flags |= HANDLE_CHECK_SUBTREE;
 		fdput(f);
 	}
 
 	return 0;
 }
 
-enum handle_to_path_flags {
-	HANDLE_CHECK_PERMS   = (1 << 0),
-	HANDLE_CHECK_SUBTREE = (1 << 1),
-};
-
-struct handle_to_path_ctx {
-	struct path root;
-	enum handle_to_path_flags flags;
-	unsigned int fh_flags;
-};
-
 static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
 {
 	struct handle_to_path_ctx *ctx = context;
@@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
 
 	if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
 		retval = 1;
-	WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
+	/*
+	 * exportfs_decode_fh_raw() does not call acceptable() callback with
+	 * a disconnected directory dentry, so we should have reached either
+	 * mount fd directory or sb root.
+	 */
+	if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
+		WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
 	dput(d);
 	return retval;
 }
@@ -265,8 +279,7 @@ static int do_handle_to_path(struct file_handle *handle, struct path *path,
  * filesystem but that only applies to procfs and sysfs neither of which
  * support decoding file handles.
  */
-static inline bool may_decode_fh(struct handle_to_path_ctx *ctx,
-				 unsigned int o_flags)
+static inline bool may_decode_fh(struct handle_to_path_ctx *ctx)
 {
 	struct path *root = &ctx->root;
 
@@ -276,7 +289,7 @@ static inline bool may_decode_fh(struct handle_to_path_ctx *ctx,
 	 *
 	 * There's only one dentry for each directory inode (VFS rule)...
 	 */
-	if (!(o_flags & O_DIRECTORY))
+	if (!(ctx->o_flags & O_DIRECTORY))
 		return false;
 
 	if (ns_capable(root->mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
@@ -303,13 +316,13 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 	int retval = 0;
 	struct file_handle f_handle;
 	struct file_handle *handle = NULL;
-	struct handle_to_path_ctx ctx = {};
+	struct handle_to_path_ctx ctx = { .o_flags = o_flags };
 
-	retval = get_path_from_fd(mountdirfd, &ctx.root);
+	retval = get_path_from_fd(mountdirfd, &ctx);
 	if (retval)
 		goto out_err;
 
-	if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx, o_flags)) {
+	if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx)) {
 		retval = -EPERM;
 		goto out_path;
 	}
-- 
2.34.1

Re: [RFC PATCH 1/2] fs: name_to_handle_at() support for connectable file handles

[Jeff Layton]

On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> nfsd encodes "connectable" file handles for the subtree_check feature.
> So far, userspace nfs server could not make use of this functionality.
> 
> Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
> When used, the encoded file handle is "connectable".
> 
> Note that decoding a "connectable" file handle with open_by_handle_at(2)
> is not guarandteed to return a "connected" fd (i.e. fd with known path).
> A new opt-in API would be needed to guarantee a "connected" fd.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/fhandle.c               | 24 ++++++++++++++++++++----
>  include/uapi/linux/fcntl.h |  1 +
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 8cb665629f4a..956d9b25d4f7 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -31,6 +31,11 @@ static long do_sys_name_to_handle(const struct path *path,
>  	if (!exportfs_can_encode_fh(path->dentry->d_sb->s_export_op, fh_flags))
>  		return -EOPNOTSUPP;
>  
> +	/* Do not encode a connectable handle for a disconnected dentry */
> +	if (fh_flags & EXPORT_FH_CONNECTABLE &&
> +	    path->dentry->d_flags & DCACHE_DISCONNECTED)
> +		return -EACCES;
> +

I'm not sure about EACCES here. That implies that if you had the right
creds then this would work. DCACHE_DISCONNECTED has nothing to do with
permissions though. Maybe -EINVAL instead since getting a disconnected
dentry here would imply that @path is somehow bogus?

Given how this function is used, will we ever see a disconnected dentry
here? The path comes from userland in this case, so I don't think it
can ever be disconnected. Maybe a WARN_ON_ONCE or pr_warn would be
appropriate in this case too?

>  	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
>  		return -EFAULT;
>  
> @@ -45,7 +50,7 @@ static long do_sys_name_to_handle(const struct path *path,
>  	/* convert handle size to multiple of sizeof(u32) */
>  	handle_dwords = f_handle.handle_bytes >> 2;
>  
> -	/* we ask for a non connectable maybe decodeable file handle */
> +	/* Encode a possibly decodeable/connectable file handle */
>  	retval = exportfs_encode_fh(path->dentry,
>  				    (struct fid *)handle->f_handle,
>  				    &handle_dwords, fh_flags);
> @@ -109,15 +114,26 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  {
>  	struct path path;
>  	int lookup_flags;
> -	int fh_flags;
> +	int fh_flags = 0;
>  	int err;
>  
>  	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> -		     AT_HANDLE_MNT_ID_UNIQUE))
> +		     AT_HANDLE_MNT_ID_UNIQUE | AT_HANDLE_CONNECTABLE))
> +		return -EINVAL;
> +
> +	/*
> +	 * AT_HANDLE_FID means there is no intention to decode file handle
> +	 * AT_HANDLE_CONNECTABLE means there is an intention to decode a
> +	 * connected fd (with known path), so these flags are conflicting.
> +	 */
> +	if (flag & AT_HANDLE_CONNECTABLE && flag & AT_HANDLE_FID)
>  		return -EINVAL;
> +	else if (flag & AT_HANDLE_FID)
> +		fh_flags |= EXPORT_FH_FID;
> +	else if (flag & AT_HANDLE_CONNECTABLE)
> +		fh_flags |= EXPORT_FH_CONNECTABLE;
>  
>  	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> -	fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0;
>  	if (flag & AT_EMPTY_PATH)
>  		lookup_flags |= LOOKUP_EMPTY;
>  	err = user_path_at(dfd, name, lookup_flags, &path);
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 87e2dec79fea..56ff2100e021 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -153,6 +153,7 @@
>  					   object identity and may not be
>  					   usable with open_by_handle_at(2). */
>  #define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
> +#define AT_HANDLE_CONNECTABLE	0x002	/* Request a connectable file handle */
>  
>  #if defined(__KERNEL__)
>  #define AT_GETATTR_NOSEC	0x80000000

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/2] fs: name_to_handle_at() support for connectable file handles

[Jeff Layton]

On Fri, 2024-09-20 at 09:13 +0200, Jeff Layton wrote:
> On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > nfsd encodes "connectable" file handles for the subtree_check feature.
> > So far, userspace nfs server could not make use of this functionality.
> > 
> > Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
> > When used, the encoded file handle is "connectable".
> > 
> > Note that decoding a "connectable" file handle with open_by_handle_at(2)
> > is not guarandteed to return a "connected" fd (i.e. fd with known path).
> > A new opt-in API would be needed to guarantee a "connected" fd.
> > 
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/fhandle.c               | 24 ++++++++++++++++++++----
> >  include/uapi/linux/fcntl.h |  1 +
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 8cb665629f4a..956d9b25d4f7 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -31,6 +31,11 @@ static long do_sys_name_to_handle(const struct path *path,
> >  	if (!exportfs_can_encode_fh(path->dentry->d_sb->s_export_op, fh_flags))
> >  		return -EOPNOTSUPP;
> >  
> > +	/* Do not encode a connectable handle for a disconnected dentry */
> > +	if (fh_flags & EXPORT_FH_CONNECTABLE &&
> > +	    path->dentry->d_flags & DCACHE_DISCONNECTED)
> > +		return -EACCES;
> > +
> 
> I'm not sure about EACCES here. That implies that if you had the right
> creds then this would work. DCACHE_DISCONNECTED has nothing to do with
> permissions though. Maybe -EINVAL instead since getting a disconnected
> dentry here would imply that @path is somehow bogus?
> 
> Given how this function is used, will we ever see a disconnected dentry
> here? The path comes from userland in this case, so I don't think it
> can ever be disconnected. Maybe a WARN_ON_ONCE or pr_warn would be
> appropriate in this case too?
> 

Oh, I guess you can get a disconnected dentry here.

You could call open_by_handle_at() for a directory fh, and then pass
that into name_to_handle_at().

Either way, this API scares me since it seems like it can just randomly
fail, depending on the state of the dcache. That's the worst-case
scenario for an API.

If you want to go through with this, you'll need to carefully document
what's required to make this work properly, as this has some
significant footguns.

 
> >  	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> >  		return -EFAULT;
> >  
> > @@ -45,7 +50,7 @@ static long do_sys_name_to_handle(const struct path *path,
> >  	/* convert handle size to multiple of sizeof(u32) */
> >  	handle_dwords = f_handle.handle_bytes >> 2;
> >  
> > -	/* we ask for a non connectable maybe decodeable file handle */
> > +	/* Encode a possibly decodeable/connectable file handle */
> >  	retval = exportfs_encode_fh(path->dentry,
> >  				    (struct fid *)handle->f_handle,
> >  				    &handle_dwords, fh_flags);
> > @@ -109,15 +114,26 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> >  {
> >  	struct path path;
> >  	int lookup_flags;
> > -	int fh_flags;
> > +	int fh_flags = 0;
> >  	int err;
> >  
> >  	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> > -		     AT_HANDLE_MNT_ID_UNIQUE))
> > +		     AT_HANDLE_MNT_ID_UNIQUE | AT_HANDLE_CONNECTABLE))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * AT_HANDLE_FID means there is no intention to decode file handle
> > +	 * AT_HANDLE_CONNECTABLE means there is an intention to decode a
> > +	 * connected fd (with known path), so these flags are conflicting.
> > +	 */
> > +	if (flag & AT_HANDLE_CONNECTABLE && flag & AT_HANDLE_FID)
> >  		return -EINVAL;
> > +	else if (flag & AT_HANDLE_FID)
> > +		fh_flags |= EXPORT_FH_FID;
> > +	else if (flag & AT_HANDLE_CONNECTABLE)
> > +		fh_flags |= EXPORT_FH_CONNECTABLE;
> >  
> >  	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> > -	fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0;
> >  	if (flag & AT_EMPTY_PATH)
> >  		lookup_flags |= LOOKUP_EMPTY;
> >  	err = user_path_at(dfd, name, lookup_flags, &path);
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 87e2dec79fea..56ff2100e021 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -153,6 +153,7 @@
> >  					   object identity and may not be
> >  					   usable with open_by_handle_at(2). */
> >  #define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
> > +#define AT_HANDLE_CONNECTABLE	0x002	/* Request a connectable file handle */
> >  
> >  #if defined(__KERNEL__)
> >  #define AT_GETATTR_NOSEC	0x80000000
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/2] fs: name_to_handle_at() support for connectable file handles

[Amir Goldstein]

On Fri, Sep 20, 2024 at 9:36 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2024-09-20 at 09:13 +0200, Jeff Layton wrote:
> > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > > nfsd encodes "connectable" file handles for the subtree_check feature.
> > > So far, userspace nfs server could not make use of this functionality.
> > >
> > > Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
> > > When used, the encoded file handle is "connectable".
> > >
> > > Note that decoding a "connectable" file handle with open_by_handle_at(2)
> > > is not guarandteed to return a "connected" fd (i.e. fd with known path).
> > > A new opt-in API would be needed to guarantee a "connected" fd.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/fhandle.c               | 24 ++++++++++++++++++++----
> > >  include/uapi/linux/fcntl.h |  1 +
> > >  2 files changed, 21 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > index 8cb665629f4a..956d9b25d4f7 100644
> > > --- a/fs/fhandle.c
> > > +++ b/fs/fhandle.c
> > > @@ -31,6 +31,11 @@ static long do_sys_name_to_handle(const struct path *path,
> > >     if (!exportfs_can_encode_fh(path->dentry->d_sb->s_export_op, fh_flags))
> > >             return -EOPNOTSUPP;
> > >
> > > +   /* Do not encode a connectable handle for a disconnected dentry */
> > > +   if (fh_flags & EXPORT_FH_CONNECTABLE &&
> > > +       path->dentry->d_flags & DCACHE_DISCONNECTED)
> > > +           return -EACCES;
> > > +
> >
> > I'm not sure about EACCES here. That implies that if you had the right
> > creds then this would work. DCACHE_DISCONNECTED has nothing to do with
> > permissions though. Maybe -EINVAL instead since getting a disconnected
> > dentry here would imply that @path is somehow bogus?
> >
> > Given how this function is used, will we ever see a disconnected dentry
> > here? The path comes from userland in this case, so I don't think it
> > can ever be disconnected. Maybe a WARN_ON_ONCE or pr_warn would be
> > appropriate in this case too?
> >

Yes, I agree (with some additions below...)

>
> Oh, I guess you can get a disconnected dentry here.
>
> You could call open_by_handle_at() for a directory fh, and then pass
> that into name_to_handle_at().

Aha, but a disconnected directory dentry is fine, because it is
still "connectable", so we should not fail on it.

>
> Either way, this API scares me since it seems like it can just randomly
> fail, depending on the state of the dcache. That's the worst-case
> scenario for an API.
>
> If you want to go through with this, you'll need to carefully document
> what's required to make this work properly, as this has some
> significant footguns.
>

Agreed.

The correct statement is that we should not get a disconnected
non-dir dentry, as long as we do not allow AT_EMPTY_PATH,
so if we return EINVAL for the flag combination
AT_EMPTY_PATH | AT_HANDLE_CONNECTABLE
we should be back to a deterministic API.

As you wrote in the first email, we should never expect to
resolve a path to a dentry that is not "connectable". Right?

Thanks,
Amir.

Re: [RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Jeff Layton]

On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2).
> This was not allowed before, so we use it to enable a new API for
> decoding "connectable" file handles that were created using the
> AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2).
>
> When mount fd is an O_PATH fd and decoding an O_PATH fd is requested,
> use that as a hint to try to decode a "connected" fd with known path,
> which is accessible (to capable user) from mount fd path.
> 
> Note that this does not check if the path is accessible to the calling
> user, just that it is accessible wrt the mount namesapce, so if there
> is no "connected" alias, or if parts of the path are hidden in the
> mount namespace, open_by_handle_at(2) will return -ESTALE.
> 
> Note that the file handles used to decode a "connected" fd do not have
> to be encoded with the AT_HANDLE_CONNECTABLE flag.  Specifically,
> directory file handles are always "connectable", regardless of using
> the AT_HANDLE_CONNECTABLE flag.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/fhandle.c | 61 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 24 deletions(-)
> 

The mountfd is only used to get a path, so I don't see a problem with
allowing that to be an O_PATH fd.

I'm less keen on using the fact that mountfd is an O_PATH fd to change
the behaviour of open_by_handle_at(). That seems very subtle. Is there
a good reason to do it that way instead of just declaring a new AT_*
flag for this?


> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 956d9b25d4f7..1fabfb79fd55 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  	return err;
>  }
>  
> -static int get_path_from_fd(int fd, struct path *root)
> +enum handle_to_path_flags {
> +	HANDLE_CHECK_PERMS   = (1 << 0),
> +	HANDLE_CHECK_SUBTREE = (1 << 1),
> +};
> +
> +struct handle_to_path_ctx {
> +	struct path root;
> +	enum handle_to_path_flags flags;
> +	unsigned int fh_flags;
> +	unsigned int o_flags;
> +};
> +
> +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx)
>  {
>  	if (fd == AT_FDCWD) {
>  		struct fs_struct *fs = current->fs;
>  		spin_lock(&fs->lock);
> -		*root = fs->pwd;
> -		path_get(root);
> +		ctx->root = fs->pwd;
> +		path_get(&ctx->root);
>  		spin_unlock(&fs->lock);
>  	} else {
> -		struct fd f = fdget(fd);
> +		struct fd f = fdget_raw(fd);
>  		if (!f.file)
>  			return -EBADF;
> -		*root = f.file->f_path;
> -		path_get(root);
> +		ctx->root = f.file->f_path;
> +		path_get(&ctx->root);
> +		/*
> +		 * Use O_PATH mount fd and requested O_PATH fd as a hint for
> +		 * decoding an fd with connected path, that is accessible from
> +		 * the mount fd path.
> +		 */
> +		if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH)
> +			ctx->flags |= HANDLE_CHECK_SUBTREE;
>  		fdput(f);
>  	}
>  
>  	return 0;
>  }
>  
> -enum handle_to_path_flags {
> -	HANDLE_CHECK_PERMS   = (1 << 0),
> -	HANDLE_CHECK_SUBTREE = (1 << 1),
> -};
> -
> -struct handle_to_path_ctx {
> -	struct path root;
> -	enum handle_to_path_flags flags;
> -	unsigned int fh_flags;
> -};
> -
>  static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
>  {
>  	struct handle_to_path_ctx *ctx = context;
> @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
>  
>  	if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
>  		retval = 1;
> -	WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> +	/*
> +	 * exportfs_decode_fh_raw() does not call acceptable() callback with
> +	 * a disconnected directory dentry, so we should have reached either
> +	 * mount fd directory or sb root.
> +	 */
> +	if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> +		WARN_ON_ONCE(d != root && d != root->d_sb->s_root);

I don't quite understand why the above change is necessary. Can you
explain why we need to limit this only to the case where
EXPORT_FH_DIR_ONLY is set?

>  	dput(d);
>  	return retval;
>  }
> @@ -265,8 +279,7 @@ static int do_handle_to_path(struct file_handle *handle, struct path *path,
>   * filesystem but that only applies to procfs and sysfs neither of which
>   * support decoding file handles.
>   */
> -static inline bool may_decode_fh(struct handle_to_path_ctx *ctx,
> -				 unsigned int o_flags)
> +static inline bool may_decode_fh(struct handle_to_path_ctx *ctx)
>  {
>  	struct path *root = &ctx->root;
>  
> @@ -276,7 +289,7 @@ static inline bool may_decode_fh(struct handle_to_path_ctx *ctx,
>  	 *
>  	 * There's only one dentry for each directory inode (VFS rule)...
>  	 */
> -	if (!(o_flags & O_DIRECTORY))
> +	if (!(ctx->o_flags & O_DIRECTORY))
>  		return false;
>  
>  	if (ns_capable(root->mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN))
> @@ -303,13 +316,13 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  	int retval = 0;
>  	struct file_handle f_handle;
>  	struct file_handle *handle = NULL;
> -	struct handle_to_path_ctx ctx = {};
> +	struct handle_to_path_ctx ctx = { .o_flags = o_flags };
>  
> -	retval = get_path_from_fd(mountdirfd, &ctx.root);
> +	retval = get_path_from_fd(mountdirfd, &ctx);
>  	if (retval)
>  		goto out_err;
>  
> -	if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx, o_flags)) {
> +	if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx)) {
>  		retval = -EPERM;
>  		goto out_path;
>  	}

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Amir Goldstein]

On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2).
> > This was not allowed before, so we use it to enable a new API for
> > decoding "connectable" file handles that were created using the
> > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2).
> >
> > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested,
> > use that as a hint to try to decode a "connected" fd with known path,
> > which is accessible (to capable user) from mount fd path.
> >
> > Note that this does not check if the path is accessible to the calling
> > user, just that it is accessible wrt the mount namesapce, so if there
> > is no "connected" alias, or if parts of the path are hidden in the
> > mount namespace, open_by_handle_at(2) will return -ESTALE.
> >
> > Note that the file handles used to decode a "connected" fd do not have
> > to be encoded with the AT_HANDLE_CONNECTABLE flag.  Specifically,
> > directory file handles are always "connectable", regardless of using
> > the AT_HANDLE_CONNECTABLE flag.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/fhandle.c | 61 +++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 37 insertions(+), 24 deletions(-)
> >
>
> The mountfd is only used to get a path, so I don't see a problem with
> allowing that to be an O_PATH fd.
>
> I'm less keen on using the fact that mountfd is an O_PATH fd to change
> the behaviour of open_by_handle_at(). That seems very subtle. Is there
> a good reason to do it that way instead of just declaring a new AT_*
> flag for this?
>

Not sure if it is a good reason, but open_by_handle_at() has an O_ flags
argument, not an AT_ flags argument...

If my hack API is not acceptable then we will need to add
open_by_handle_at2(), with struct open_how argument or something.

>
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 956d9b25d4f7..1fabfb79fd55 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> >       return err;
> >  }
> >
> > -static int get_path_from_fd(int fd, struct path *root)
> > +enum handle_to_path_flags {
> > +     HANDLE_CHECK_PERMS   = (1 << 0),
> > +     HANDLE_CHECK_SUBTREE = (1 << 1),
> > +};
> > +
> > +struct handle_to_path_ctx {
> > +     struct path root;
> > +     enum handle_to_path_flags flags;
> > +     unsigned int fh_flags;
> > +     unsigned int o_flags;
> > +};
> > +
> > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx)
> >  {
> >       if (fd == AT_FDCWD) {
> >               struct fs_struct *fs = current->fs;
> >               spin_lock(&fs->lock);
> > -             *root = fs->pwd;
> > -             path_get(root);
> > +             ctx->root = fs->pwd;
> > +             path_get(&ctx->root);
> >               spin_unlock(&fs->lock);
> >       } else {
> > -             struct fd f = fdget(fd);
> > +             struct fd f = fdget_raw(fd);
> >               if (!f.file)
> >                       return -EBADF;
> > -             *root = f.file->f_path;
> > -             path_get(root);
> > +             ctx->root = f.file->f_path;
> > +             path_get(&ctx->root);
> > +             /*
> > +              * Use O_PATH mount fd and requested O_PATH fd as a hint for
> > +              * decoding an fd with connected path, that is accessible from
> > +              * the mount fd path.
> > +              */
> > +             if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH)
> > +                     ctx->flags |= HANDLE_CHECK_SUBTREE;
> >               fdput(f);
> >       }
> >
> >       return 0;
> >  }
> >
> > -enum handle_to_path_flags {
> > -     HANDLE_CHECK_PERMS   = (1 << 0),
> > -     HANDLE_CHECK_SUBTREE = (1 << 1),
> > -};
> > -
> > -struct handle_to_path_ctx {
> > -     struct path root;
> > -     enum handle_to_path_flags flags;
> > -     unsigned int fh_flags;
> > -};
> > -
> >  static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> >  {
> >       struct handle_to_path_ctx *ctx = context;
> > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> >
> >       if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
> >               retval = 1;
> > -     WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> > +     /*
> > +      * exportfs_decode_fh_raw() does not call acceptable() callback with
> > +      * a disconnected directory dentry, so we should have reached either
> > +      * mount fd directory or sb root.
> > +      */
> > +     if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> > +             WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
>
> I don't quite understand why the above change is necessary. Can you
> explain why we need to limit this only to the case where
> EXPORT_FH_DIR_ONLY is set?
>

With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should
only be calling acceptable() with a connected directory dentry.

Until this patch, vfs_dentry_acceptable() would only be called with
EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional.

After this patch, vfs_dentry_acceptable() could also be called for
a disconnected non-dir dentry and then it should just fail to
accept the dentry, but should not WARN_ON.

Thanks for the review!
Amir.

Re: [RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Jeff Layton]

On Fri, 2024-09-20 at 18:38 +0200, Amir Goldstein wrote:
> On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2).
> > > This was not allowed before, so we use it to enable a new API for
> > > decoding "connectable" file handles that were created using the
> > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2).
> > > 
> > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested,
> > > use that as a hint to try to decode a "connected" fd with known path,
> > > which is accessible (to capable user) from mount fd path.
> > > 
> > > Note that this does not check if the path is accessible to the calling
> > > user, just that it is accessible wrt the mount namesapce, so if there
> > > is no "connected" alias, or if parts of the path are hidden in the
> > > mount namespace, open_by_handle_at(2) will return -ESTALE.
> > > 
> > > Note that the file handles used to decode a "connected" fd do not have
> > > to be encoded with the AT_HANDLE_CONNECTABLE flag.  Specifically,
> > > directory file handles are always "connectable", regardless of using
> > > the AT_HANDLE_CONNECTABLE flag.
> > > 
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/fhandle.c | 61 +++++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 37 insertions(+), 24 deletions(-)
> > > 
> > 
> > The mountfd is only used to get a path, so I don't see a problem with
> > allowing that to be an O_PATH fd.
> > 
> > I'm less keen on using the fact that mountfd is an O_PATH fd to change
> > the behaviour of open_by_handle_at(). That seems very subtle. Is there
> > a good reason to do it that way instead of just declaring a new AT_*
> > flag for this?
> > 
> 
> Not sure if it is a good reason, but open_by_handle_at() has an O_ flags
> argument, not an AT_ flags argument...
> 
> If my hack API is not acceptable then we will need to add
> open_by_handle_at2(), with struct open_how argument or something.
> 

Oh right, I forgot that open_by_handle_at doesn't take AT_* flags.
A new syscall may be best then.

I can see a couple of other potential approaches:

1/ You could add a new fcntl() cmd that puts the mountfd into a
"connectable filehandles" mode. The downside there is that it'd take 2
syscalls to do your open.

2/ You could add flags to open_how that make openat2() behave like
open_by_handle_at(). Add a flag that allows "pathname" to point to a
filehandle instead, and a second flag that indicates that the fh is
connectable.

Both of those are pretty hacky though.

> > 
> > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > index 956d9b25d4f7..1fabfb79fd55 100644
> > > --- a/fs/fhandle.c
> > > +++ b/fs/fhandle.c
> > > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> > >       return err;
> > >  }
> > > 
> > > -static int get_path_from_fd(int fd, struct path *root)
> > > +enum handle_to_path_flags {
> > > +     HANDLE_CHECK_PERMS   = (1 << 0),
> > > +     HANDLE_CHECK_SUBTREE = (1 << 1),
> > > +};
> > > +
> > > +struct handle_to_path_ctx {
> > > +     struct path root;
> > > +     enum handle_to_path_flags flags;
> > > +     unsigned int fh_flags;
> > > +     unsigned int o_flags;
> > > +};
> > > +
> > > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx)
> > >  {
> > >       if (fd == AT_FDCWD) {
> > >               struct fs_struct *fs = current->fs;
> > >               spin_lock(&fs->lock);
> > > -             *root = fs->pwd;
> > > -             path_get(root);
> > > +             ctx->root = fs->pwd;
> > > +             path_get(&ctx->root);
> > >               spin_unlock(&fs->lock);
> > >       } else {
> > > -             struct fd f = fdget(fd);
> > > +             struct fd f = fdget_raw(fd);
> > >               if (!f.file)
> > >                       return -EBADF;
> > > -             *root = f.file->f_path;
> > > -             path_get(root);
> > > +             ctx->root = f.file->f_path;
> > > +             path_get(&ctx->root);
> > > +             /*
> > > +              * Use O_PATH mount fd and requested O_PATH fd as a hint for
> > > +              * decoding an fd with connected path, that is accessible from
> > > +              * the mount fd path.
> > > +              */
> > > +             if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH)
> > > +                     ctx->flags |= HANDLE_CHECK_SUBTREE;
> > >               fdput(f);
> > >       }
> > > 
> > >       return 0;
> > >  }
> > > 
> > > -enum handle_to_path_flags {
> > > -     HANDLE_CHECK_PERMS   = (1 << 0),
> > > -     HANDLE_CHECK_SUBTREE = (1 << 1),
> > > -};
> > > -
> > > -struct handle_to_path_ctx {
> > > -     struct path root;
> > > -     enum handle_to_path_flags flags;
> > > -     unsigned int fh_flags;
> > > -};
> > > -
> > >  static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> > >  {
> > >       struct handle_to_path_ctx *ctx = context;
> > > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> > > 
> > >       if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
> > >               retval = 1;
> > > -     WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> > > +     /*
> > > +      * exportfs_decode_fh_raw() does not call acceptable() callback with
> > > +      * a disconnected directory dentry, so we should have reached either
> > > +      * mount fd directory or sb root.
> > > +      */
> > > +     if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> > > +             WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> > 
> > I don't quite understand why the above change is necessary. Can you
> > explain why we need to limit this only to the case where
> > EXPORT_FH_DIR_ONLY is set?
> > 
> 
> With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should
> only be calling acceptable() with a connected directory dentry.
> 
> Until this patch, vfs_dentry_acceptable() would only be called with
> EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional.
> 
> After this patch, vfs_dentry_acceptable() could also be called for
> a disconnected non-dir dentry and then it should just fail to
> accept the dentry, but should not WARN_ON.
> 
> Thanks for the review!
> Amir.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 7254 bytes --]

On 2024-09-21, Jeff Layton <jlayton@kernel.org> wrote:
> On Fri, 2024-09-20 at 18:38 +0200, Amir Goldstein wrote:
> > On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2).
> > > > This was not allowed before, so we use it to enable a new API for
> > > > decoding "connectable" file handles that were created using the
> > > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2).
> > > > 
> > > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested,
> > > > use that as a hint to try to decode a "connected" fd with known path,
> > > > which is accessible (to capable user) from mount fd path.
> > > > 
> > > > Note that this does not check if the path is accessible to the calling
> > > > user, just that it is accessible wrt the mount namesapce, so if there
> > > > is no "connected" alias, or if parts of the path are hidden in the
> > > > mount namespace, open_by_handle_at(2) will return -ESTALE.
> > > > 
> > > > Note that the file handles used to decode a "connected" fd do not have
> > > > to be encoded with the AT_HANDLE_CONNECTABLE flag.  Specifically,
> > > > directory file handles are always "connectable", regardless of using
> > > > the AT_HANDLE_CONNECTABLE flag.
> > > > 
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  fs/fhandle.c | 61 +++++++++++++++++++++++++++++++---------------------
> > > >  1 file changed, 37 insertions(+), 24 deletions(-)
> > > > 
> > > 
> > > The mountfd is only used to get a path, so I don't see a problem with
> > > allowing that to be an O_PATH fd.
> > > 
> > > I'm less keen on using the fact that mountfd is an O_PATH fd to change
> > > the behaviour of open_by_handle_at(). That seems very subtle. Is there
> > > a good reason to do it that way instead of just declaring a new AT_*
> > > flag for this?
> > > 
> > 
> > Not sure if it is a good reason, but open_by_handle_at() has an O_ flags
> > argument, not an AT_ flags argument...
> > 
> > If my hack API is not acceptable then we will need to add
> > open_by_handle_at2(), with struct open_how argument or something.
> > 
> 
> Oh right, I forgot that open_by_handle_at doesn't take AT_* flags.
> A new syscall may be best then.
> 
> I can see a couple of other potential approaches:
> 
> 1/ You could add a new fcntl() cmd that puts the mountfd into a
> "connectable filehandles" mode. The downside there is that it'd take 2
> syscalls to do your open.
> 
> 2/ You could add flags to open_how that make openat2() behave like
> open_by_handle_at(). Add a flag that allows "pathname" to point to a
> filehandle instead, and a second flag that indicates that the fh is
> connectable.

Hackiness aside, the latter is not workable until we can filter
extensible structs with seccomp. Container runtimes all block
open_by_handle_at(2) because it can be used to break out of non-userns
containers.

> Both of those are pretty hacky though.
> 
> > > 
> > > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > > index 956d9b25d4f7..1fabfb79fd55 100644
> > > > --- a/fs/fhandle.c
> > > > +++ b/fs/fhandle.c
> > > > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> > > >       return err;
> > > >  }
> > > > 
> > > > -static int get_path_from_fd(int fd, struct path *root)
> > > > +enum handle_to_path_flags {
> > > > +     HANDLE_CHECK_PERMS   = (1 << 0),
> > > > +     HANDLE_CHECK_SUBTREE = (1 << 1),
> > > > +};
> > > > +
> > > > +struct handle_to_path_ctx {
> > > > +     struct path root;
> > > > +     enum handle_to_path_flags flags;
> > > > +     unsigned int fh_flags;
> > > > +     unsigned int o_flags;
> > > > +};
> > > > +
> > > > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx)
> > > >  {
> > > >       if (fd == AT_FDCWD) {
> > > >               struct fs_struct *fs = current->fs;
> > > >               spin_lock(&fs->lock);
> > > > -             *root = fs->pwd;
> > > > -             path_get(root);
> > > > +             ctx->root = fs->pwd;
> > > > +             path_get(&ctx->root);
> > > >               spin_unlock(&fs->lock);
> > > >       } else {
> > > > -             struct fd f = fdget(fd);
> > > > +             struct fd f = fdget_raw(fd);
> > > >               if (!f.file)
> > > >                       return -EBADF;
> > > > -             *root = f.file->f_path;
> > > > -             path_get(root);
> > > > +             ctx->root = f.file->f_path;
> > > > +             path_get(&ctx->root);
> > > > +             /*
> > > > +              * Use O_PATH mount fd and requested O_PATH fd as a hint for
> > > > +              * decoding an fd with connected path, that is accessible from
> > > > +              * the mount fd path.
> > > > +              */
> > > > +             if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH)
> > > > +                     ctx->flags |= HANDLE_CHECK_SUBTREE;
> > > >               fdput(f);
> > > >       }
> > > > 
> > > >       return 0;
> > > >  }
> > > > 
> > > > -enum handle_to_path_flags {
> > > > -     HANDLE_CHECK_PERMS   = (1 << 0),
> > > > -     HANDLE_CHECK_SUBTREE = (1 << 1),
> > > > -};
> > > > -
> > > > -struct handle_to_path_ctx {
> > > > -     struct path root;
> > > > -     enum handle_to_path_flags flags;
> > > > -     unsigned int fh_flags;
> > > > -};
> > > > -
> > > >  static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> > > >  {
> > > >       struct handle_to_path_ctx *ctx = context;
> > > > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> > > > 
> > > >       if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
> > > >               retval = 1;
> > > > -     WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> > > > +     /*
> > > > +      * exportfs_decode_fh_raw() does not call acceptable() callback with
> > > > +      * a disconnected directory dentry, so we should have reached either
> > > > +      * mount fd directory or sb root.
> > > > +      */
> > > > +     if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> > > > +             WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> > > 
> > > I don't quite understand why the above change is necessary. Can you
> > > explain why we need to limit this only to the case where
> > > EXPORT_FH_DIR_ONLY is set?
> > > 
> > 
> > With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should
> > only be calling acceptable() with a connected directory dentry.
> > 
> > Until this patch, vfs_dentry_acceptable() would only be called with
> > EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional.
> > 
> > After this patch, vfs_dentry_acceptable() could also be called for
> > a disconnected non-dir dentry and then it should just fail to
> > accept the dentry, but should not WARN_ON.
> > 
> > Thanks for the review!
> > Amir.
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 6153 bytes --]

On 2024-09-20, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2).
> > > This was not allowed before, so we use it to enable a new API for
> > > decoding "connectable" file handles that were created using the
> > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2).
> > >
> > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested,
> > > use that as a hint to try to decode a "connected" fd with known path,
> > > which is accessible (to capable user) from mount fd path.
> > >
> > > Note that this does not check if the path is accessible to the calling
> > > user, just that it is accessible wrt the mount namesapce, so if there
> > > is no "connected" alias, or if parts of the path are hidden in the
> > > mount namespace, open_by_handle_at(2) will return -ESTALE.
> > >
> > > Note that the file handles used to decode a "connected" fd do not have
> > > to be encoded with the AT_HANDLE_CONNECTABLE flag.  Specifically,
> > > directory file handles are always "connectable", regardless of using
> > > the AT_HANDLE_CONNECTABLE flag.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/fhandle.c | 61 +++++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 37 insertions(+), 24 deletions(-)
> > >
> >
> > The mountfd is only used to get a path, so I don't see a problem with
> > allowing that to be an O_PATH fd.
> >
> > I'm less keen on using the fact that mountfd is an O_PATH fd to change
> > the behaviour of open_by_handle_at(). That seems very subtle. Is there
> > a good reason to do it that way instead of just declaring a new AT_*
> > flag for this?
> >
> 
> Not sure if it is a good reason, but open_by_handle_at() has an O_ flags
> argument, not an AT_ flags argument...
> 
> If my hack API is not acceptable then we will need to add
> open_by_handle_at2(), with struct open_how argument or something.

Does all of the stuff in openat2 make sense for open_by_handle_at? What
should RESOLVE_* or mode do? There's no standard namei lookup done.

> > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > index 956d9b25d4f7..1fabfb79fd55 100644
> > > --- a/fs/fhandle.c
> > > +++ b/fs/fhandle.c
> > > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> > >       return err;
> > >  }
> > >
> > > -static int get_path_from_fd(int fd, struct path *root)
> > > +enum handle_to_path_flags {
> > > +     HANDLE_CHECK_PERMS   = (1 << 0),
> > > +     HANDLE_CHECK_SUBTREE = (1 << 1),
> > > +};
> > > +
> > > +struct handle_to_path_ctx {
> > > +     struct path root;
> > > +     enum handle_to_path_flags flags;
> > > +     unsigned int fh_flags;
> > > +     unsigned int o_flags;
> > > +};
> > > +
> > > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx)
> > >  {
> > >       if (fd == AT_FDCWD) {
> > >               struct fs_struct *fs = current->fs;
> > >               spin_lock(&fs->lock);
> > > -             *root = fs->pwd;
> > > -             path_get(root);
> > > +             ctx->root = fs->pwd;
> > > +             path_get(&ctx->root);
> > >               spin_unlock(&fs->lock);
> > >       } else {
> > > -             struct fd f = fdget(fd);
> > > +             struct fd f = fdget_raw(fd);
> > >               if (!f.file)
> > >                       return -EBADF;
> > > -             *root = f.file->f_path;
> > > -             path_get(root);
> > > +             ctx->root = f.file->f_path;
> > > +             path_get(&ctx->root);
> > > +             /*
> > > +              * Use O_PATH mount fd and requested O_PATH fd as a hint for
> > > +              * decoding an fd with connected path, that is accessible from
> > > +              * the mount fd path.
> > > +              */
> > > +             if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH)
> > > +                     ctx->flags |= HANDLE_CHECK_SUBTREE;
> > >               fdput(f);
> > >       }
> > >
> > >       return 0;
> > >  }
> > >
> > > -enum handle_to_path_flags {
> > > -     HANDLE_CHECK_PERMS   = (1 << 0),
> > > -     HANDLE_CHECK_SUBTREE = (1 << 1),
> > > -};
> > > -
> > > -struct handle_to_path_ctx {
> > > -     struct path root;
> > > -     enum handle_to_path_flags flags;
> > > -     unsigned int fh_flags;
> > > -};
> > > -
> > >  static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> > >  {
> > >       struct handle_to_path_ctx *ctx = context;
> > > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> > >
> > >       if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
> > >               retval = 1;
> > > -     WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> > > +     /*
> > > +      * exportfs_decode_fh_raw() does not call acceptable() callback with
> > > +      * a disconnected directory dentry, so we should have reached either
> > > +      * mount fd directory or sb root.
> > > +      */
> > > +     if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> > > +             WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> >
> > I don't quite understand why the above change is necessary. Can you
> > explain why we need to limit this only to the case where
> > EXPORT_FH_DIR_ONLY is set?
> >
> 
> With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should
> only be calling acceptable() with a connected directory dentry.
> 
> Until this patch, vfs_dentry_acceptable() would only be called with
> EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional.
> 
> After this patch, vfs_dentry_acceptable() could also be called for
> a disconnected non-dir dentry and then it should just fail to
> accept the dentry, but should not WARN_ON.
> 
> Thanks for the review!
> Amir.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Amir Goldstein]

On Sat, Sep 21, 2024 at 7:33 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2024-09-20 at 18:38 +0200, Amir Goldstein wrote:
> > On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2).
> > > > This was not allowed before, so we use it to enable a new API for
> > > > decoding "connectable" file handles that were created using the
> > > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2).
> > > >
> > > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested,
> > > > use that as a hint to try to decode a "connected" fd with known path,
> > > > which is accessible (to capable user) from mount fd path.
> > > >
> > > > Note that this does not check if the path is accessible to the calling
> > > > user, just that it is accessible wrt the mount namesapce, so if there
> > > > is no "connected" alias, or if parts of the path are hidden in the
> > > > mount namespace, open_by_handle_at(2) will return -ESTALE.
> > > >
> > > > Note that the file handles used to decode a "connected" fd do not have
> > > > to be encoded with the AT_HANDLE_CONNECTABLE flag.  Specifically,
> > > > directory file handles are always "connectable", regardless of using
> > > > the AT_HANDLE_CONNECTABLE flag.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  fs/fhandle.c | 61 +++++++++++++++++++++++++++++++---------------------
> > > >  1 file changed, 37 insertions(+), 24 deletions(-)
> > > >
> > >
> > > The mountfd is only used to get a path, so I don't see a problem with
> > > allowing that to be an O_PATH fd.
> > >
> > > I'm less keen on using the fact that mountfd is an O_PATH fd to change
> > > the behaviour of open_by_handle_at(). That seems very subtle. Is there
> > > a good reason to do it that way instead of just declaring a new AT_*
> > > flag for this?
> > >
> >
> > Not sure if it is a good reason, but open_by_handle_at() has an O_ flags
> > argument, not an AT_ flags argument...
> >
> > If my hack API is not acceptable then we will need to add
> > open_by_handle_at2(), with struct open_how argument or something.
> >
>
> Oh right, I forgot that open_by_handle_at doesn't take AT_* flags.
> A new syscall may be best then.
>
> I can see a couple of other potential approaches:
>
> 1/ You could add a new fcntl() cmd that puts the mountfd into a
> "connectable filehandles" mode. The downside there is that it'd take 2
> syscalls to do your open.
>
> 2/ You could add flags to open_how that make openat2() behave like
> open_by_handle_at(). Add a flag that allows "pathname" to point to a
> filehandle instead, and a second flag that indicates that the fh is
> connectable.
>
> Both of those are pretty hacky though.
>

Yes, especially #2, very creative ;)

But I had another less hacky idea.
Instead of (ab)using a sycall flag to get a "connected" fd,
encode an explicit "connectable" fh, something like this untested patch.

WDYT?

Thanks,
Amir.

--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -59,6 +59,7 @@ static long do_sys_name_to_handle(const struct path *path,
        handle_bytes = handle_dwords * sizeof(u32);
        handle->handle_bytes = handle_bytes;
        if ((handle->handle_bytes > f_handle.handle_bytes) ||
+           WARN_ON_ONCE(retval > FILEID_INVALID) ||
            (retval == FILEID_INVALID) || (retval < 0)) {
                /* As per old exportfs_encode_fh documentation
                 * we could return ENOSPC to indicate overflow
@@ -72,8 +73,21 @@ static long do_sys_name_to_handle(const struct path *path,
                 * non variable part of the file_handle
                 */
                handle_bytes = 0;
-       } else
+       } else {
+               /*
+                * When asked to encode a connectable file handle, encode this
+                * property in the file handle itself, so we know how
to decode it.
+                * For sanity, also encode in the file handle if the
encoded object
+                * is a directory, because connectable directory file
handles do not
+                * encode the parent.
+                */
+               if (fh_flags & EXPORT_FH_CONNECTABLE) {
+                       if (d_is_dir(path->dentry))
+                               fh_flags |= EXPORT_FH_DIR_ONLY;
+                       handle->handle_flags = fh_flags;
+               }
                retval = 0;
+       }
...
@@ -336,6 +350,19 @@ static int handle_to_path(int mountdirfd, struct
file_handle __user *ufh,
                retval = -EINVAL;
                goto out_path;
        }
+       if (f_handle.handle_flags & ~EXPORT_FH_USER_FLAGS) {
+               retval = -EINVAL;
+               goto out_path;
+       }
+       /*
+        * If handle was encoded with AT_HANDLE_CONNECTABLE, verify that we
+        * are decoding an fd with connected path, which is accessible from
+        * the mount fd path.
+        */
+       ctx.fh_flags |= f_handle.handle_flags;
+       if (ctx.fh_flags & EXPORT_FH_CONNECTABLE)
+               ctx.flags |= HANDLE_CHECK_SUBTREE;
+
        handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
                         GFP_KERNEL);
...
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -159,6 +159,8 @@ struct fid {
 #define EXPORT_FH_CONNECTABLE  0x1 /* Encode file handle with parent */
 #define EXPORT_FH_FID          0x2 /* File handle may be non-decodeable */
 #define EXPORT_FH_DIR_ONLY     0x4 /* Only decode file handle for a
directory */
+/* Flags allowed in encoded handle_flags that is exported to user */
+#define EXPORT_FH_USER_FLAGS   (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
...
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1071,7 +1071,8 @@ struct file {

 struct file_handle {
        __u32 handle_bytes;
-       int handle_type;
+       int handle_type:16;
+       int handle_flags:16;
        /* file identifier */
        unsigned char f_handle[] __counted_by(handle_bytes);
 };