Re: [PATCH 1/2] fs: introduce uid/gid shifting bind mount

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2019-12-03 at 16:33 +0200, Amir Goldstein wrote:
> On Tue, Dec 3, 2019 at 4:10 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > [splitting topics for ease of threading]
> > On Tue, 2019-12-03 at 08:55 +0200, Amir Goldstein wrote:
> > > On Tue, Dec 3, 2019 at 7:12 AM James Bottomley
> > > <James.Bottomley@hansenpartnership.com> wrote:
> > > > 
> > > > On Tue, 2019-12-03 at 06:51 +0200, Amir Goldstein wrote:
> > > > > [cc: ebiederman]
> > 
> > [...]
> > > > > 2. Needs serious vetting by Eric (cc'ed)
> > > > > 3. A lot of people have been asking me for filtering of
> > > > > "dirent" fsnotify events (i.e. create/delete) by path, which
> > > > > is not available in those vfs functions, so if the concept of
> > > > > current-mnt flies, fsnotify is going to want to use it as
> > > > > well.
> > > > 
> > > > Just a caveat: current->mnt is used in this patch simply as a
> > > > tag, which means it doesn't need to be refcounted.  I think I
> > > > can prove that it is absolutely valid if the cred is shifted
> > > > because the reference is held by the code that shifted the
> > > > cred, but it's definitely not valid except for a tag comparison
> > > > outside of that.  Thus, if it is useful for fsnotify, more
> > > > thought will have to be given to refcounting it.
> > > > 
> > > 
> > > Yes. Is there anything preventing us from taking refcount on
> > > current->mnt?
> > 
> > We could, but what would it usefully mean?  It would just be the
> > last mnt that had its credentials shifted.  I think stashing a
> > refcounted mnt in the task structure is reasonably easy:  The creds
> > are refcounted, so you simply follow all the task mnt_cred logic I
> > added for releasing the ref in the correct places, so if you want
> > to do that, I can simply rename this tag to something less generic
> > ... unless you have some idea about using the last shift mnt?
> > 
> 
> Nevermind. Didn't want to derail the thread.

Don't worry, that's why I separated the issues.

> I am still not sure what the semantics of generic current->mnt should
> be.

OK, so that's easy: it's not designed in the current patch set ever to
be used as a mnt.  It's simply a tag to tell you if the cached shifted
credentials in mnt_cred are valid for reuse.  To the only use I put it
to is in change_userns_cred() where we look at the task cached
mnt+mnt_cred and if mnt matches the mnt change_user_ns_cred was called
for we know that if mnt_cred is not NULL then it's usable as the pre-
prepared credentials.

> operations like copy_file_range() with two path arguments can
> get confusing and handling nesting (e.g. overlayfs can be confusing
> as well).

So I think the concept of using the task struct to carry information
you don't want to thread all the way up and down the stack, like how
I've done for knowledge of the shift being in effect, is potentially a
useful one.  It is a bit of a hack though to work around the fact that
our API is missing stuff.

James