On Fri, Oct 11, 2024 at 4:18 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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@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 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.
