| Message ID | 20241011090023.655623-3-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | API for exporting connectable file handles to userspace | expand |
On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote: > nfsd encodes "connectable" file handles for the subtree_check feature, > which can be resolved to an open file with a connected path. > 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 "explicitly connectable". > > The "explicitly connectable" file handle sets bits in the high 16bit of > the handle_type field, so open_by_handle_at(2) will know that it needs > to open a file with a connected path. > > old kernels will now recognize the handle_type with high bits set, > so "explicitly connectable" file handles cannot be decoded by > open_by_handle_at(2) on old kernels. > > The flag AT_HANDLE_CONNECTABLE is not allowed together with either > AT_HANDLE_FID or AT_EMPTY_PATH. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/fhandle.c | 48 ++++++++++++++++++++++++++++++++++---- > include/linux/exportfs.h | 2 ++ > include/uapi/linux/fcntl.h | 1 + > 3 files changed, 46 insertions(+), 5 deletions(-) > > diff --git a/fs/fhandle.c b/fs/fhandle.c > index 218511f38cbb..8339a1041025 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -31,6 +31,14 @@ 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; > > + /* > + * A request to encode a connectable handle for a disconnected dentry > + * is unexpected since AT_EMPTY_PATH is not allowed. > + */ > + if (fh_flags & EXPORT_FH_CONNECTABLE && > + WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED)) Is this even possible? The dentry in this case will have been reached by pathwalk. Oh, but I guess the dfd could point to a disconnected dentry and then you pass in AT_EMPTY_PATH. I'm not sure we want to warn in that case though, since this is a situation that an unprivileged user could be able to arrange. Maybe we should just return a more distinct error code in this case? Since the scenario involves a dfd that is disconnected, how about: #define EBADFD 77 /* File descriptor in bad state */ > + return -EINVAL; > + > if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) > return -EFAULT; > > @@ -45,7 +53,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); > @@ -67,8 +75,23 @@ 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 that we later know > + * how to decode it. > + * For sanity, also encode in the file handle if the encoded > + * object is a directory and verify this during decode, because > + * decoding directory file handles is quite different than > + * decoding connectable non-directory file handles. > + */ > + if (fh_flags & EXPORT_FH_CONNECTABLE) { > + handle->handle_type |= FILEID_IS_CONNECTABLE; > + if (d_is_dir(path->dentry)) > + fh_flags |= FILEID_IS_DIR; > + } > retval = 0; > + } > /* copy the mount id */ > if (unique_mntid) { > if (put_user(real_mount(path->mnt)->mnt_id_unique, > @@ -109,15 +132,30 @@ 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. > + * AT_EMPTY_PATH could be used along with a dfd that refers to a > + * disconnected non-directory, which cannot be used to encode a > + * connectable file handle, because its parent is unknown. > + */ > + if (flag & AT_HANDLE_CONNECTABLE && nit: might need parenthesis around the above & check. > + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) > 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/linux/exportfs.h b/include/linux/exportfs.h > index 5e14d4500a75..4ee42b2cf4ab 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -169,6 +169,8 @@ struct fid { > #define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) > > /* Flags supported in encoded handle_type that is exported to user */ > +#define FILEID_IS_CONNECTABLE 0x10000 > +#define FILEID_IS_DIR 0x20000 > #define FILEID_VALID_USER_FLAGS (0) > > /** > 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, Oct 11, 2024 at 4:00 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote: > > nfsd encodes "connectable" file handles for the subtree_check feature, > > which can be resolved to an open file with a connected path. > > 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 "explicitly connectable". > > > > The "explicitly connectable" file handle sets bits in the high 16bit of > > the handle_type field, so open_by_handle_at(2) will know that it needs > > to open a file with a connected path. > > > > old kernels will now recognize the handle_type with high bits set, > > so "explicitly connectable" file handles cannot be decoded by > > open_by_handle_at(2) on old kernels. > > > > The flag AT_HANDLE_CONNECTABLE is not allowed together with either > > AT_HANDLE_FID or AT_EMPTY_PATH. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/fhandle.c | 48 ++++++++++++++++++++++++++++++++++---- > > include/linux/exportfs.h | 2 ++ > > include/uapi/linux/fcntl.h | 1 + > > 3 files changed, 46 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > index 218511f38cbb..8339a1041025 100644 > > --- a/fs/fhandle.c > > +++ b/fs/fhandle.c > > @@ -31,6 +31,14 @@ 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; > > > > + /* > > + * A request to encode a connectable handle for a disconnected dentry > > + * is unexpected since AT_EMPTY_PATH is not allowed. > > + */ > > + if (fh_flags & EXPORT_FH_CONNECTABLE && > > + WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED)) > > Is this even possible? The dentry in this case will have been reached > by pathwalk. Oh, but I guess the dfd could point to a disconnected > dentry and then you pass in AT_EMPTY_PATH. But see comment above "...is unexpected since AT_EMPTY_PATH is not allowed." and see below + * AT_EMPTY_PATH could be used along with a dfd that refers to a + * disconnected non-directory, which cannot be used to encode a + * connectable file handle, because its parent is unknown. + */ + if (flag & AT_HANDLE_CONNECTABLE && + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) return -EINVAL; The code/API should not allow this also for a malicious user, unless I missed something, hence, the assertion. > > I'm not sure we want to warn in that case though, since this is a > situation that an unprivileged user could be able to arrange. Maybe we > should just return a more distinct error code in this case? > > Since the scenario involves a dfd that is disconnected, how about: > > #define EBADFD 77 /* File descriptor in bad state */ > To me it does not look like a good fit, but let's see what others think. In the end, it is a rare condition that should never happen (hence assert), so I don't think the error value matters that much? > > + return -EINVAL; > > + > > if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) > > return -EFAULT; > > > > @@ -45,7 +53,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); > > @@ -67,8 +75,23 @@ 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 that we later know > > + * how to decode it. > > + * For sanity, also encode in the file handle if the encoded > > + * object is a directory and verify this during decode, because > > + * decoding directory file handles is quite different than > > + * decoding connectable non-directory file handles. > > + */ > > + if (fh_flags & EXPORT_FH_CONNECTABLE) { > > + handle->handle_type |= FILEID_IS_CONNECTABLE; > > + if (d_is_dir(path->dentry)) > > + fh_flags |= FILEID_IS_DIR; > > + } > > retval = 0; > > + } > > /* copy the mount id */ > > if (unique_mntid) { > > if (put_user(real_mount(path->mnt)->mnt_id_unique, > > @@ -109,15 +132,30 @@ 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. > > + * AT_EMPTY_PATH could be used along with a dfd that refers to a > > + * disconnected non-directory, which cannot be used to encode a > > + * connectable file handle, because its parent is unknown. > > + */ > > + if (flag & AT_HANDLE_CONNECTABLE && > > nit: might need parenthesis around the above & check. > > > + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) I don't think it is needed, but for readability I don't mind adding them. I am having a hard time remembering the operation precedence myself, but this one is clear to me so I don't bother with (). Thanks, Amir.
On Fri, 2024-10-11 at 16:14 +0200, Amir Goldstein wrote: > On Fri, Oct 11, 2024 at 4:00 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote: > > > nfsd encodes "connectable" file handles for the subtree_check feature, > > > which can be resolved to an open file with a connected path. > > > 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 "explicitly connectable". > > > > > > The "explicitly connectable" file handle sets bits in the high 16bit of > > > the handle_type field, so open_by_handle_at(2) will know that it needs > > > to open a file with a connected path. > > > > > > old kernels will now recognize the handle_type with high bits set, > > > so "explicitly connectable" file handles cannot be decoded by > > > open_by_handle_at(2) on old kernels. > > > > > > The flag AT_HANDLE_CONNECTABLE is not allowed together with either > > > AT_HANDLE_FID or AT_EMPTY_PATH. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/fhandle.c | 48 ++++++++++++++++++++++++++++++++++---- > > > include/linux/exportfs.h | 2 ++ > > > include/uapi/linux/fcntl.h | 1 + > > > 3 files changed, 46 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > > index 218511f38cbb..8339a1041025 100644 > > > --- a/fs/fhandle.c > > > +++ b/fs/fhandle.c > > > @@ -31,6 +31,14 @@ 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; > > > > > > + /* > > > + * A request to encode a connectable handle for a disconnected dentry > > > + * is unexpected since AT_EMPTY_PATH is not allowed. > > > + */ > > > + if (fh_flags & EXPORT_FH_CONNECTABLE && > > > + WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED)) > > > > Is this even possible? The dentry in this case will have been reached > > by pathwalk. Oh, but I guess the dfd could point to a disconnected > > dentry and then you pass in AT_EMPTY_PATH. > > But see comment above "...is unexpected since AT_EMPTY_PATH is not allowed." > > and see below > > + * AT_EMPTY_PATH could be used along with a dfd that refers to a > + * disconnected non-directory, which cannot be used to encode a > + * connectable file handle, because its parent is unknown. > + */ > + if (flag & AT_HANDLE_CONNECTABLE && > + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) > return -EINVAL; > > The code/API should not allow this also for a malicious user, > unless I missed something, hence, the assertion. > Ok. If that's the case, I'm fine with this as-is then. If that ever fires then I guess we'll know that something is wrong. > > > > I'm not sure we want to warn in that case though, since this is a > > situation that an unprivileged user could be able to arrange. Maybe we > > should just return a more distinct error code in this case? > > > > Since the scenario involves a dfd that is disconnected, how about: > > > > #define EBADFD 77 /* File descriptor in bad state */ > > > > To me it does not look like a good fit, but let's see what others think. > In the end, it is a rare condition that should never happen > (hence assert), so I don't think the error value matters that much? > Agreed. > > > + return -EINVAL; > > > + > > > if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) > > > return -EFAULT; > > > > > > @@ -45,7 +53,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); > > > @@ -67,8 +75,23 @@ 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 that we later know > > > + * how to decode it. > > > + * For sanity, also encode in the file handle if the encoded > > > + * object is a directory and verify this during decode, because > > > + * decoding directory file handles is quite different than > > > + * decoding connectable non-directory file handles. > > > + */ > > > + if (fh_flags & EXPORT_FH_CONNECTABLE) { > > > + handle->handle_type |= FILEID_IS_CONNECTABLE; > > > + if (d_is_dir(path->dentry)) > > > + fh_flags |= FILEID_IS_DIR; > > > + } > > > retval = 0; > > > + } > > > /* copy the mount id */ > > > if (unique_mntid) { > > > if (put_user(real_mount(path->mnt)->mnt_id_unique, > > > @@ -109,15 +132,30 @@ 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. > > > + * AT_EMPTY_PATH could be used along with a dfd that refers to a > > > + * disconnected non-directory, which cannot be used to encode a > > > + * connectable file handle, because its parent is unknown. > > > + */ > > > + if (flag & AT_HANDLE_CONNECTABLE && > > > > nit: might need parenthesis around the above & check. > > > > > + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) > > I don't think it is needed, but for readability I don't mind adding them. > I am having a hard time remembering the operation precedence myself, > but this one is clear to me so I don't bother with (). I (lately) get warnings from the compiler with W=1 even when the precedence is fine. If you're not seeing that then this is OK too.
On Fri, Oct 11, 2024 at 4:18 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2024-10-11 at 16:14 +0200, Amir Goldstein wrote: > > On Fri, Oct 11, 2024 at 4:00 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote: > > > > nfsd encodes "connectable" file handles for the subtree_check feature, > > > > which can be resolved to an open file with a connected path. > > > > 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 "explicitly connectable". > > > > > > > > The "explicitly connectable" file handle sets bits in the high 16bit of > > > > the handle_type field, so open_by_handle_at(2) will know that it needs > > > > to open a file with a connected path. > > > > > > > > old kernels will now recognize the handle_type with high bits set, > > > > so "explicitly connectable" file handles cannot be decoded by > > > > open_by_handle_at(2) on old kernels. > > > > > > > > The flag AT_HANDLE_CONNECTABLE is not allowed together with either > > > > AT_HANDLE_FID or AT_EMPTY_PATH. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > fs/fhandle.c | 48 ++++++++++++++++++++++++++++++++++---- > > > > include/linux/exportfs.h | 2 ++ > > > > include/uapi/linux/fcntl.h | 1 + > > > > 3 files changed, 46 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > > > index 218511f38cbb..8339a1041025 100644 > > > > --- a/fs/fhandle.c > > > > +++ b/fs/fhandle.c > > > > @@ -31,6 +31,14 @@ 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; > > > > > > > > + /* > > > > + * A request to encode a connectable handle for a disconnected dentry > > > > + * is unexpected since AT_EMPTY_PATH is not allowed. > > > > + */ > > > > + if (fh_flags & EXPORT_FH_CONNECTABLE && > > > > + WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED)) > > > > > > Is this even possible? The dentry in this case will have been reached > > > by pathwalk. Oh, but I guess the dfd could point to a disconnected > > > dentry and then you pass in AT_EMPTY_PATH. > > > > But see comment above "...is unexpected since AT_EMPTY_PATH is not allowed." > > > > and see below > > > > + * AT_EMPTY_PATH could be used along with a dfd that refers to a > > + * disconnected non-directory, which cannot be used to encode a > > + * connectable file handle, because its parent is unknown. > > + */ > > + if (flag & AT_HANDLE_CONNECTABLE && > > + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) > > return -EINVAL; > > > > The code/API should not allow this also for a malicious user, > > unless I missed something, hence, the assertion. > > > > Ok. If that's the case, I'm fine with this as-is then. If that ever > fires then I guess we'll know that something is wrong. > > > > > > > I'm not sure we want to warn in that case though, since this is a > > > situation that an unprivileged user could be able to arrange. Maybe we > > > should just return a more distinct error code in this case? > > > > > > Since the scenario involves a dfd that is disconnected, how about: > > > > > > #define EBADFD 77 /* File descriptor in bad state */ > > > > > > > To me it does not look like a good fit, but let's see what others think. > > In the end, it is a rare condition that should never happen > > (hence assert), so I don't think the error value matters that much? > > > > Agreed. > > > > > + return -EINVAL; > > > > + > > > > if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) > > > > return -EFAULT; > > > > > > > > @@ -45,7 +53,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); > > > > @@ -67,8 +75,23 @@ 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 that we later know > > > > + * how to decode it. > > > > + * For sanity, also encode in the file handle if the encoded > > > > + * object is a directory and verify this during decode, because > > > > + * decoding directory file handles is quite different than > > > > + * decoding connectable non-directory file handles. > > > > + */ > > > > + if (fh_flags & EXPORT_FH_CONNECTABLE) { > > > > + handle->handle_type |= FILEID_IS_CONNECTABLE; > > > > + if (d_is_dir(path->dentry)) > > > > + fh_flags |= FILEID_IS_DIR; > > > > + } > > > > retval = 0; > > > > + } > > > > /* copy the mount id */ > > > > if (unique_mntid) { > > > > if (put_user(real_mount(path->mnt)->mnt_id_unique, > > > > @@ -109,15 +132,30 @@ 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. > > > > + * AT_EMPTY_PATH could be used along with a dfd that refers to a > > > > + * disconnected non-directory, which cannot be used to encode a > > > > + * connectable file handle, because its parent is unknown. > > > > + */ > > > > + if (flag & AT_HANDLE_CONNECTABLE && > > > > > > nit: might need parenthesis around the above & check. > > > > > > > + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) > > > > I don't think it is needed, but for readability I don't mind adding them. > > I am having a hard time remembering the operation precedence myself, > > but this one is clear to me so I don't bother with (). > > I (lately) get warnings from the compiler with W=1 even when the > precedence is fine. If you're not seeing that then this is OK too. > Did not get any warnings, but if Christian wants to add the () on commit I have no quarrel with that :) Thanks for the review! Amir.
diff --git a/fs/fhandle.c b/fs/fhandle.c index 218511f38cbb..8339a1041025 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -31,6 +31,14 @@ 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; + /* + * A request to encode a connectable handle for a disconnected dentry + * is unexpected since AT_EMPTY_PATH is not allowed. + */ + if (fh_flags & EXPORT_FH_CONNECTABLE && + WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED)) + return -EINVAL; + if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) return -EFAULT; @@ -45,7 +53,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); @@ -67,8 +75,23 @@ 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 that we later know + * how to decode it. + * For sanity, also encode in the file handle if the encoded + * object is a directory and verify this during decode, because + * decoding directory file handles is quite different than + * decoding connectable non-directory file handles. + */ + if (fh_flags & EXPORT_FH_CONNECTABLE) { + handle->handle_type |= FILEID_IS_CONNECTABLE; + if (d_is_dir(path->dentry)) + fh_flags |= FILEID_IS_DIR; + } retval = 0; + } /* copy the mount id */ if (unique_mntid) { if (put_user(real_mount(path->mnt)->mnt_id_unique, @@ -109,15 +132,30 @@ 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. + * AT_EMPTY_PATH could be used along with a dfd that refers to a + * disconnected non-directory, which cannot be used to encode a + * connectable file handle, because its parent is unknown. + */ + if (flag & AT_HANDLE_CONNECTABLE && + flag & (AT_HANDLE_FID | AT_EMPTY_PATH)) 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/linux/exportfs.h b/include/linux/exportfs.h index 5e14d4500a75..4ee42b2cf4ab 100644 --- a/include/linux/exportfs.h +++ b/include/linux/exportfs.h @@ -169,6 +169,8 @@ struct fid { #define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) /* Flags supported in encoded handle_type that is exported to user */ +#define FILEID_IS_CONNECTABLE 0x10000 +#define FILEID_IS_DIR 0x20000 #define FILEID_VALID_USER_FLAGS (0) /** 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, which can be resolved to an open file with a connected path. 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 "explicitly connectable". The "explicitly connectable" file handle sets bits in the high 16bit of the handle_type field, so open_by_handle_at(2) will know that it needs to open a file with a connected path. old kernels will now recognize the handle_type with high bits set, so "explicitly connectable" file handles cannot be decoded by open_by_handle_at(2) on old kernels. The flag AT_HANDLE_CONNECTABLE is not allowed together with either AT_HANDLE_FID or AT_EMPTY_PATH. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/fhandle.c | 48 ++++++++++++++++++++++++++++++++++---- include/linux/exportfs.h | 2 ++ include/uapi/linux/fcntl.h | 1 + 3 files changed, 46 insertions(+), 5 deletions(-)