On Tue, Oct 8, 2024 at 8:19 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> > We would like to use the high 16bit of the handle_type field to encode
> > file handle traits, such as "connectable".
> >
> > In preparation for this change, make sure that filesystems do not return
> > a handle_type value with upper bits set and that the open_by_handle_at(2)
> > syscall rejects these handle types.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/exportfs/expfs.c | 14 ++++++++++++--
> > fs/fhandle.c | 6 ++++++
> > include/linux/exportfs.h | 14 ++++++++++++++
> > 3 files changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index 4f2dd4ab4486..c8eb660fdde4 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> > int *max_len, struct inode *parent, int flags)
> > {
> > const struct export_operations *nop = inode->i_sb->s_export_op;
> > + enum fid_type type;
> >
> > if (!exportfs_can_encode_fh(nop, flags))
> > return -EOPNOTSUPP;
> >
> > if (!nop && (flags & EXPORT_FH_FID))
> > - return exportfs_encode_ino64_fid(inode, fid, max_len);
> > + type = exportfs_encode_ino64_fid(inode, fid, max_len);
> > + else
> > + type = nop->encode_fh(inode, fid->raw, max_len, parent);
> > +
> > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(type)))
> > + return -EINVAL;
> > +
>
> The stack trace won't be very useful here. Rather than a WARN, it might
> be better to dump out some info about the fstype (and maybe other
> info?) that returned the bogus type value here. I'm pretty sure most
> in-kernel fs's don't do this, but who knows what 3rd party fs's might
> do.
>
Right. I changed to:
if (FILEID_USER_FLAGS(type)) {
pr_warn_once("%s: unexpected fh type value 0x%x from
fstype %s.\n",
__func__, type, inode->i_sb->s_type->name);
return -EINVAL;
}
> > + return type;
> >
> > - return nop->encode_fh(inode, fid->raw, max_len, parent);
> > }
> > EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
> >
> > @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
> > char nbuf[NAME_MAX+1];
> > int err;
> >
> > + if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
> > + return -EINVAL;
> > +
>
>
> This is called from do_handle_to_path() or nfsd_set_fh_dentry(), which
> means that this fh comes from userland or from an NFS client. I don't
> think we want to WARN because someone crafted a bogus fh and passed it
> to us.
>
Good point, I will remove the WARN and also fix the bug :-/
if (FILEID_USER_FLAGS(fileid_type))
return ERR_PTR(-EINVAL);
Pushed this and othe minor fixes to
https://github.com/amir73il/linux/commits/connectable-fh/
until we sort out the rest of your comments and maybe get more feedback.
Thanks for the review!
Amir.
