LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Steven Whitehouse <swhiteho@redhat.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [git pull] mount API series
Date: Mon, 17 Dec 2018 23:10:00 +0000	[thread overview]
Message-ID: <20181217231000.GA30610@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20181112205439.GF32577@ZenIV.linux.org.uk>

On Mon, Nov 12, 2018 at 08:54:39PM +0000, Al Viro wrote:
> On Sun, Nov 11, 2018 at 08:07:20PM -0600, Eric W. Biederman wrote:
> > Steven Whitehouse <swhiteho@redhat.com> writes:
> 
> > > Can you share some details of what this NULL dereference is? David and
> > > Al have been working on the changes as requested by Linus later in
> > > this thread, and they'd like to tidy up this issue too at the same
> > > time if possible. We are not asking you to actually provide a fix, in
> > > case you are too busy to do so, however it would be good to know what
> > > the issue is so that we can make sure that it is resolved in the next
> > > round of patches,
> > 
> > https://lore.kernel.org/lkml/87bm7n5k1r.fsf@xmission.com/
> 
> Thought it had been dealt with, but you are right - oops is still there
> and obviously needs fixing.  However, looking at that place in mainline...
> How does that thing manage to work?  Look:
>         /* Notice when we are propagating across user namespaces */
>         if (m->mnt_ns->user_ns != user_ns)
>                 type |= CL_UNPRIVILEGED;
>         child = copy_tree(last_source, last_source->mnt.mnt_root, type);
>         if (IS_ERR(child))
>                 return PTR_ERR(child);
>         child->mnt.mnt_flags &= ~MNT_LOCKED;
>         mnt_set_mountpoint(m, mp, child);
>         last_dest = m;
>         last_source = child;
> OK, we'd created the copy to be attached to the next candidate mountpoint.
> If we have e.g. a 4-element peer group, we'll start with what we'd been
> asked to mount, then call that sucker three times, getting a copy for
> each of those mountpoints.  Right?  Now, what happens if the 1st, 3rd and
> 4th members live in your namespace, with the second one being elsewhere?
> We have
> 	source_mnt: that'll go on top of the 1st mountpoint
> 	copy of source_mnt: that'll go on top of the 2nd mountpoint
> 	copy of copy of source_mnt: that'll go on top of the 3rd mountpoint
> 	copy of copy of copy of source_mnt: that'll go on top of the 4th one
> And AFAICS your logics there has just made sure that everything except the
> source_mnt will have MNT_LOCK_... all over the place.  IOW, the effect of
> CL_UNPRIVELEGED is cumulative.
> 
> How the hell does that code avoid this randomness?  Note had the members of
> that peer group been in a different order, you would've gotten a different result.
> What am I missing here?
> 
> Oops is a separate story, and a regression in its own right; it needs to be
> fixed.  But I would really like to sort out the semantics of the existing
> code in that area, so that we don't end up with patch conflicts.

Ping?

FWIW, I don't believe that CL_UNPRIVELEGED approach is workable; the logics
we want is, AFAICS, "when destination is not yours, whatever you attach to
it should be all locked".  Correct?

If so, the natural place to deal with that would be in commit_tree() after
propagate_mnt() has created all copies, not while creating those copies.
That, after all, is where we mark all those struct mount as belonging to
namespace...

Again, this is quite independent from the oops you've reported (and that,
BTW, can be triggered without any userns involvement - commit_tree() itself
will oops just fine if parent's ->mnt_ns is NULL, userns or no userns).
I think I understand how to deal with that thing, but it's a separate
story; handling of MNT_LOCK... is a problem that exists in the mainline
and whatever fix we come up with for this one will need to be backportable.

Al "resurfaced from long and thoroughly nasty dive through the LSM gutter
and finally getting around to more pleasant stuff" Viro

  reply	other threads:[~2018-12-17 23:10 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-31  5:33 Al Viro
2018-10-31 15:38 ` Eric W. Biederman
2018-10-31 16:18   ` Eric W. Biederman
2018-10-31 16:36   ` Al Viro
2018-11-01 16:51     ` Al Viro
2018-11-10 14:19   ` Steven Whitehouse
2018-11-12  2:07     ` Eric W. Biederman
2018-11-12 20:54       ` Al Viro
2018-12-17 23:10         ` Al Viro [this message]
2018-12-21 16:25           ` Eric W. Biederman
2018-10-31 16:18 ` Linus Torvalds
2018-11-01 10:53   ` Steven Whitehouse
2018-11-01 15:57     ` Linus Torvalds
2018-11-01 17:18     ` David Howells
2018-11-01 18:33       ` Linus Torvalds
2018-11-01 22:05         ` Al Viro
2018-11-01 22:07           ` Linus Torvalds
2018-11-01 23:59       ` David Howells
2018-11-02  4:07         ` Al Viro
2018-11-02 19:42           ` Al Viro
2018-11-03  6:14             ` Gao Xiang
2018-11-03  6:30           ` Gao Xiang
2018-10-31 18:39 ` David Howells
2018-10-31 20:49   ` Miklos Szeredi
2018-10-31 18:45 ` [PATCH] vfs: Fix incorrect user_ns assignment in proc and mqueue David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181217231000.GA30610@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=ebiederm@xmission.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=swhiteho@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [git pull] mount API series' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).