On Tue, Oct 8, 2024 at 10:31 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> 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;
> }
>
>
FYI, following Jan's comment about mixing bitwise with negative values,
I changes this to:
if (type > 0 && 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;
because ->encode_fh() method are allowed to return a negative error
(e.g. -EOVERFLOW)
> > > + 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);
>
And changed this to:
@@ -436,6 +446,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt,
struct fid *fid, int fh_len,
char nbuf[NAME_MAX+1];
int err;
+ if (fileid_type < 0 || 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.
>
If no further comments I will post v4.
Thanks,
Amir.
