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
