| Message ID | 20240919140611.1771651-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | API for exporting connectable file handles to userspace | expand |
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
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> >
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.
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
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(-)