On Tue, Apr 16, 2024 at 08:08:26PM +0200, Christoph Hellwig wrote:
> On Tue, Apr 16, 2024 at 10:59:08AM -0700, Darrick J. Wong wrote:
> > On Mon, Apr 15, 2024 at 10:21:03PM -0700, Christoph Hellwig wrote:
> > > > + if (memcmp(&handle->ha_fsid, mp->m_fixedfsid, sizeof(struct xfs_fsid)))
> > > > + return -ESTALE;
> > > > +
> > > > + if (handle->ha_fid.fid_len != xfs_filehandle_fid_len())
> > > > + return -EINVAL;
> > >
> > > Maybe we should just stash the fid without the fsid? The check for a
> > > match fsid when userspace already had to resolve it to even call the
> > > ioctl is a bit silly.
> >
> > Remember a few revisions ago when this ioctl only returned the raw fid
> > information? At the time, I didn't want to bloat struct parent_rec with
> > the full handle information, but then I changed it to export a full
> > handle. When I went to update libfrog/getparents.c, I realized that
> > giving full handles to userspace is a much better interface because any
> > code that's trying to walk up the directory tree to compute the file
> > path can simply call getparents again with the handle it has been given.
> >
> > It's much more ergonomic if userspace programs don't need to know how to
> > construct handles from partial information they has been given. If a
> > calling program wants to open the parent directory, it can call
> > open_by_fshandle directly:
>
> Yeah. I think I actually suggested that :) So I'll take that back.
> That just leaves us with the question if we want to validate the
> fsid here and diverge from the mormal handle ops or not. I think
> I'd prefer behaving the same as the other handle ops just to avoid
> confusion.
Yeah, let's drop the fsid memcmp. Now I have:
/* Convert handle already copied to kernel space into an xfs_inode. */
static struct xfs_inode *
xfs_khandle_to_inode(
struct file *file,
struct xfs_handle *handle)
{
struct xfs_inode *ip = XFS_I(file_inode(file));
struct xfs_mount *mp = ip->i_mount;
struct inode *inode;
if (!S_ISDIR(VFS_I(ip)->i_mode))
return ERR_PTR(-ENOTDIR);
if (handle->ha_fid.fid_len != xfs_filehandle_fid_len())
return ERR_PTR(-EINVAL);
inode = xfs_nfs_get_inode(mp->m_super, handle->ha_fid.fid_ino,
handle->ha_fid.fid_gen);
if (IS_ERR(inode))
return ERR_CAST(inode);
return XFS_I(inode);
}
and later:
/*
* We don't use exportfs_decode_fh because it does too much work here.
* If the handle refers to a directory, the exportfs code will walk
* upwards through the directory tree to connect the dentries to the
* root directory dentry. For GETPARENTS we don't care about that
* because we're not actually going to open a file descriptor; we only
* want to open an inode and read its parent pointers.
*
* Note that xfs_scrub uses GETPARENTS to log that it will try to fix a
* corrupted file's metadata. For this usecase we would really rather
* userspace single-step the path reconstruction to avoid loops or
* other strange things if the directory tree is corrupt.
*/
gpx.ip = xfs_khandle_to_inode(file, handle);
if (IS_ERR(gpx.ip))
return PTR_ERR(gpx.ip);
--D
