Re: [PATCH 27/32] xfs: Add parent pointer ioctls

Darrick J. Wong
2024-04-15T19:40:37Z

On Sun, Apr 14, 2024 at 07:18:16AM +0200, Christoph Hellwig wrote:
> [full quote deleted. It took me about a minute of scrolling to find
> the actual contents, *sigh*]
>
> On Fri, Apr 12, 2024 at 10:39:57AM -0700, Darrick J. Wong wrote:
> > I noticed a couple of things while doing more testing here -- first,
> > xfs_khandle_to_dentry doesn't check that the handle fsid actually
> > matches this filesystem, and AFAICT *nothing* actually checks that.
>
> Yes. Userspace better have resolved that, as the ioctl only works
> on the given file system, so libhandle has to resolve it before
> even calling the ioctl.

True, libhandle is a very nice wrapper for the kernel ioctls. I wish
Linux projects did that more often. But suppose you're calling the
ioctls directly without libhandle and mess it up?

> > So I guess that's a longstanding weakness of handle validation, and we
> > probably haven't gotten any reports because what's the chance that
> > you'll get lucky with an ino/gen from a different filesystem?
>
> Not really, see above.
>
> > The second thing is that exportfs_decode_fh does too much work here --
> > if the handle references a directory, it'll walk up the directory tree
> > to the root to try to reconnect the dentry paths. For GETPARENTS we
> > don't care about that since we're not doing anything with dentries.
> > Walking upwards in the directory tree is extra work that doesn't change
> > the results.
>
> In theory no one cares as all operations work just fine with disconnected
> dentries, and exportfs_decode_fh doesn't do these checks unless the
> accpetable parameter is passed to it. The real question is why we (which
> in this case means 15 years younger me) decided back then we want this
> checking for XFS handle operations? I can't really think of one
> right now..

Me neither. Though at this point there are a lot of filesystems that
implement ->get_parent, so I think removing XFS's will need a discussion
at least on linux-xfs, if not fsdevel. In the meantime, getparents can
do minimal validation + iget for now and if it makes sense to port it
back to xfs_khandle_to_dentry, I can do that easily.

(FWIW turning off reconnection would likely fix some of the annoying
behaviors of xfs_scrub where it tries to open a dir to scan it and then
sprays dmesg with errors from unrelated parents as it stumbles over
reconnection only to fail the open, at which point it falls back to
scrubbing by handle anyway.)

--D