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, 12 Nov 2018 20:54:39 +0000	[thread overview]
Message-ID: <20181112205439.GF32577@ZenIV.linux.org.uk> (raw)
In-Reply-To: <877ehjkq07.fsf@xmission.com>

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.

  reply	other threads:[~2018-11-12 20:54 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 [this message]
2018-12-17 23:10         ` Al Viro
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=20181112205439.GF32577@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).