Re: [PATCH/RFC] VFS: LOOKUP_MOUNTPOINT should used cached info whenever possible.

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2023-04-18 at 10:04 +0200, Christian Brauner wrote:
> On Mon, Apr 17, 2023 at 09:25:20PM -0600, Andreas Dilger wrote:
> > 
> > > On Apr 17, 2023, at 9:21 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > On Mon, 2023-04-17 at 16:24 +0200, Christian Brauner wrote:
> > > > And I'm curious why is it obvious that we don't want to revalidate _any_
> > > > path component and not just the last one? Why is that generally safe?
> > > > Why can't this be used to access files and directories the caller
> > > > wouldn't otherwise be able to access? I would like to have this spelled
> > > > out for slow people like me, please.
> > > > 
> > > > From my point of view, this would only be somewhat safe _generally_ if
> > > > you'd allow circumvention for revalidation and permission checking if
> > > > MNT_FORCE is specified and the caller has capable(CAP_DAC_READ_SEARCH).
> > > > You'd still mess with overlayfs permission model in this case though.
> > > > 
> > > > Plus, there are better options of solving this problem. Again, I'd
> > > > rather build a separate api for unmounting then playing such potentially
> > > > subtle security sensitive games with permission checking during path
> > > > lookup.
> > > 
> > > umount(2) is really a special case because the whole intent is to detach
> > > a mount from the local hierarchy and stop using it. The unfortunate bit
> > > is that it is a path-based syscall.
> > > 
> > > So usually we have to:
> > > 
> > > - determine the path: Maybe stat() it and to validate that it's the
> > >   mountpoint we want to drop
> > 
> > The stat() itself may hang because a remote server, or USB stick is
> > inaccessible or having media errors.
> > 
> > I've just been having a conversation with Karel Zak to change
> > umount(1) to use statx() so that it interacts minimally with the fs.
> 
> So we're talking about this commit:
> https://github.com/util-linux/util-linux/commit/42e141d20505a0deb969c2e583a463c26aadc62f
> and the patch we discussed here:
> https://github.com/util-linux/util-linux/issues/2049
> 
> > 
> > In particular, nfs_getattr() skips revalidate if only minimal attrs
> > are fetched (STATX_TYPE | STATX_INO), and also skips revalidate if
> > locally-cached attrs are still valid (STATX_MODE), so this will
> > avoid yet one more place that unmount can hang.
> > 
> > In theory, vfs_getattr() could get all of these attributes directly
> > from the vfs_inode in the unmount case.
> 
> We don't really need that. As pointed out in that discussion and as
> Karel did we just want to pass AT_STATX_DONT_SYNC.
> 
> An api that would allow unmounting by mount id can safely check and
> retrieve AT_STATX_DONT_SYNC with STATX_ATTR_MOUNT_ROOT and STATX_MNT_ID
> without ever syncing with the server.

Unfortunately, I don't think that flag trickles down to the ->permission
checks for the pathwalk down to the point you're stat'ing. That turns
out to be a big part of the problem when trying to umount things that
are stuck down in inaccessible trees.

I'm not opposed at all to new APIs that allow for more reliable
unmounting. I think that's a good idea, and something worth hashing out.

But...we're stuck with umount() in perpetuity. Even if you were to merge
a new API for unmounting today, it'll be a decade or more until the
ecosystem catches up. I think we have a responsibility to make the
umount syscall work as well as we can. In this case, that means giving
it as light a footprint as possible on the pathwalk down.

If we need to gate this behavior behind existing or new flags to
umount2(), then so be it, but lets not dismiss this idea in favor of
some new unmounting interface that may never materialize. Improving
umount has the potential to help a lot of our users.

-- 
Jeff Layton <jlayton@kernel.org>