Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fabbb94e0e35190ce8033107dbbff99188f72e18.camel@kernel.org>
Subject: Re: allowing for a completely cached umount(2) pathwalk
From:   Jeff Layton <jlayton@kernel.org>
To:     Christian Brauner <brauner@kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>
Cc:     Trond Myklebust <trondmy@hammerspace.com>,
        Neil Brown <neilb@suse.de>,
        Dave Wysochanski <dwysocha@redhat.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-nfs <linux-nfs@vger.kernel.org>,
        David Howells <dhowells@redhat.com>,
        Christoph Hellwig <hch@lst.de>
Date:   Fri, 14 Apr 2023 08:33:09 -0400
In-Reply-To: <20230414-gerissen-bemessen-835e95dc7552@brauner>
References: <95ee689c76bf034fa2fe9fade0bccdb311f3a04f.camel@kernel.org>
         <168142566371.24821.15867603327393356000@noble.neil.brown.name>
         <20230414024312.GF3390869@ZenIV>
         <8EC5C625-ACD6-4BA0-A190-21A73CCBAC34@hammerspace.com>
         <20230414035104.GH3390869@ZenIV>
         <20230414-leihgabe-eisig-71fb7bb44d49@brauner>
         <3492fa76339672ccc48995ccf934744c63db4b80.camel@kernel.org>
         <20230414-gerissen-bemessen-835e95dc7552@brauner>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-04-14 at 13:16 +0200, Christian Brauner wrote:
> On Fri, Apr 14, 2023 at 06:09:46AM -0400, Jeff Layton wrote:
> > On Fri, 2023-04-14 at 11:41 +0200, Christian Brauner wrote:
> > > On Fri, Apr 14, 2023 at 04:51:04AM +0100, Al Viro wrote:
> > > > On Fri, Apr 14, 2023 at 03:28:45AM +0000, Trond Myklebust wrote:
> > > >=20
> > > > > We already have support for directory file descriptors when mount=
ing with move_mount(). Why not add a umountat() with similar support for th=
e unmount side?
> > > > > Then add a syscall to allow users with (e.g.) the CAP_DAC_OVERRID=
E privilege to convert the mount-id into an O_PATH file descriptor.
> > > >=20
> > > > You can already do umount -l /proc/self/fd/69 if you have a descrip=
tor.
> > >=20
> > > Way back when we put together stuff for [2] we had umountat() as an i=
tem
> > > but decided against it because it's mostely useful when used as AT_EM=
PTY_PATH.
> > >=20
> > > umount("/proc/self/fd/<nr>", ...) is useful when you don't trust the
> > > path and you need to resolve it with lookup restrictions. Then path
> > > resolution restrictions of openat2() can be used to get an fd. Which =
can
> > > be passed to umount().
> > >=20
> > > I need to step outside so this is a halfway-out-the-door thought but
> > > given your description of the problem Jeff, why doesn't the following
> > > work (Just sketching this, you can't call openat2() like that.):
> > >=20
> > >         fd_mnt =3D openat2("/my/funky/nfs/share/mount", RESOLVE_CACHE=
D)
> > >         umount("/proc/self/fd/fd_mnt", MNT_DETACH)
> >=20
> > Something like that might work. A RESOLVE_CACHED flag is something that
> > would involve more than just umount(2) though. That said, it could be
> > useful in other situations.
>=20
> I think I introduced an ambiguity by accident. What I meant by "you
> can't call openat2() like that" is that it takes a struct open_how
> argument not just a simple flags argument.
>=20
> The flag I was talking about, RESOLVE_CACHED, does exist already. So it
> is already possible to use openat2() to resolve paths like that. See
> include/uapi/linux/openat2.h
>=20

Oh, thanks! I haven't tried this yet, but I doubt it'd fix the problem
David was reproducing. There, the problem is mostly permission checks
during the pathwalk. It tries to contact the server to check the
permission, but because it's unexported, EACCES bubbles up.

Also, to follow up: I tried Al's suggestion of lazily unmounting the bad
intermediate mount closest to the root, and it does seem to clean up
child mounts as expected. So, it does look like there is recourse in
current kernels, even if the method is a bit unintuitive.

Still, it might be worth discussing at LSF or here on the list. It'd be
nice if umount "just worked" in this situation. Is there any reason to
do a full-blown d_revalidating and permission-checking pathwalk on
umount, assuming the caller already has CAP_SYS_ADMIN or similar?

--=20
Jeff Layton <jlayton@kernel.org>