On Tue, Oct 8, 2024 at 8:31 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Tue, 2024-10-08 at 17:21 +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@xxxxxxxxx>
> > ---
> > 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 c5792cf3c6e9..7b4c8945efcb 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;
> > +
>
> Is it possible to get a DCACHE_DISCONNECTED dentry here? This thing
> comes from a userland path walk (a'la name_to_handle_at()). That means
> that it necessarily is connected.
Can't you get it with AT_EMPTY_PATH if dfd is obtained by open_by_handle()?
>
> I'd drop the fh_flags check, since it seems like having that set on any
> dentry we get in this interface would be a bug.
>
Well I don't know if giving a disconnected dentry to name_to_handle
is intentional, but it is not wrong, unless user actually requests a
connectable file handle. No?
> > 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 76a3050b3593..230b0e1d669d 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 0x40000
>
> nit: why skip 0x20000 ?
Well in v2, the values were just shifting the EXPORT_FH_ flags left
so I kept it this way, although I am not sure if FILEID_ID_FID is ever
going to be useful, so I guess there is no good reason to skip 0x20000
Thanks,
Amir.
