From: Aleksa Sarai <cyphar@cyphar.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Eric Biederman <ebiederm@xmission.com>,
Christian Brauner <christian@brauner.io>,
Jeff Layton <jlayton@kernel.org>,
"J. Bruce Fields" <bfields@fieldses.org>,
Arnd Bergmann <arnd@arndb.de>, Andy Lutomirski <luto@kernel.org>,
David Howells <dhowells@redhat.com>, Jann Horn <jannh@google.com>,
Tycho Andersen <tycho@tycho.ws>,
David Drysdale <drysdale@google.com>,
dev@opencontainers.org, containers@lists.linux-foundation.org,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arch@vger.kernel.org, linux-api@vger.kernel.org
Subject: Re: [PATCH v3 1/3] namei: implement O_BENEATH-style AT_* flags
Date: Sat, 13 Oct 2018 19:09:08 +1100 [thread overview]
Message-ID: <20181013080907.yqpuy3zbbfe46gm4@ryuk> (raw)
In-Reply-To: <20181013073319.GS32577@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 5408 bytes --]
On 2018-10-13, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> First of all, dirfd_path_init() part should be in a separate commit. And I'm
> really not happy with the logics in there. dirfd_path_init() itself is
> kinda-sorta reasonable.
Sure, I can do that.
> It is equivalent to setting the starting point for
> relative pathnames + setting ->root for LOOKUP_BENEATH, right?
Right.
> But the part in path_init() is too bloody convoluted for its own good. Let me
> try to translate:
>
> > + if (unlikely(flags & LOOKUP_XDEV)) {
> > + error = dirfd_path_init(nd);
> > + if (unlikely(error))
> > + return ERR_PTR(error);
> > + }
>
> * if LOOKUP_XDEV is set, set the starting point as if it was a relative
> pathname. If LOOKUP_BENEATH was set as well, set ->root to the same
> point.
Right. This is for two reasons (though if you disagree with these
semantics we can change this as well):
1. It's not clear to me whether openat(somefd->"/", "/tmp", O_XDEV)
should return an -EXDEV or completely ignore the starting point. Same
argument with AT_FDCWD. I opted to make it so that the starting point
has to be on the same mountpoint, but I totally understand if you
feel this is insane -- and I'd be happy to change it. The real
problem comes from (2).
2. AT_THIS_ROOT chroot-scope absolute paths, and so in the second patch
LOOKUP_CHROOT also triggers this codepath. The main argument for this
semantic is somewhat elaborated in the cover letter -- but
the short version is because AT_THIS_ROOT has to chroot-scope
absolute symlinks it would be somewhat strange if it didn't scope
absolute paths you give it -- otherwise it could either be a footgun
or would require always returning -EXDEV here.
Though, as above, if you feel that the current semantics (absolute
paths override whatever dirfd you give), then -EXDEV is the
alternative I would pitch.
> * if it's an absolute pathname,
> > if (*s == '/') {
> ... and we hadn't come here with LOOKUP_XDEV + LOOKUP_BENEATH, set ->root.
> > + if (likely(!nd->root.mnt))
> > + set_root(nd);
> * if it's an absolute pathname, set the starting point to ->root. Note that
> if we came here with LOOKUP_XDEV, we'll discard the starting point we'd
> calculated.
We wouldn't discard it -- nd_jump_root() will check whether a mount
crossing was implied here (otherwise an absolute symlink could cause you
to cross a mountpoint).
But as above, if you'd prefer that absolute paths disable all dirfd
handling (as is the case now), I can remove this semantic.
> > + error = nd_jump_root(nd);
> > + if (unlikely(error))
> > + s = ERR_PTR(error);
> > return s;
> > }
> > + if (likely(!nd->path.mnt)) {
> * if we didn't have LOOKUP_XDEV, set the starting point as if it was a relative
> pathname (which it is) and, if LOOKUP_BENEATH is also there, set ->root there
> as well.
> > + error = dirfd_path_init(nd);
> > + if (unlikely(error))
> > + return ERR_PTR(error);
> > + }
> > + return s;
> > }
>
> Pardon me, but... huh? The reason for your two calls of dirfd_path_init() is,
> AFAICS, the combination of absolute pathname with both LOOKUP_XDEV and
> LOOKUP_BENEATH at the same time. That combination is treated as if the pathname
> had been relative. Note that LOOKUP_BENEATH alone is ignored for absolute ones
> (and with a good reason - it's a no-op on path_init() level in that case).
>
> What the hell? It complicates your code and doesn't seem to provide any benefits
> whatsoever
The reasoning for this is because of how AT_THIS_ROOT uses both of these
codepaths (it causes dirfd_path_init() to be called before the absolute
check, and also causes ->root to be set).
I wrote the features in parallel and then split out the code for
AT_THIS_ROOT so it could be discussed separately (and so removing it if
it was rejected would be simpler). But unfortunately this does result in
the dirfd_path_init() code looking completely superfluous without seeing
the second patch.
> -- you could bloody well have passed the relative pathname to start with.
(I think you mean always doing dirfd_path_init() first here?)
Right, but I didn't want to discard nd->path unnecessarily -- if we do
all of the code to grab AT_FDCWD and then it is completely unused (not
even in the AT_XDEV sense of "unused") it seems like a waste.
Did I misunderstand your suggestion? Were you referring to userspace
just being able to "[pass] the relative pathname to start with"?
> IDGI... Without that kludge it becomes simply "do as we currently do for absolute
> pathnames, call dirfd_path_init() for relative ones". And I would argue that
> taking LOOKUP_BENEATH handling out of dirfd_path_init() into path_init() (relative)
> case would be a good idea.
Right, I could definitely do that -- though for AT_THIS_ROOT we'd
duplicate the ->root setting in both places.
> As it is, the logics is very hard to follow.
Sorry about that. Would you prefer if the two patches (AT_BENEATH family
and AT_THIS_ROOT) were sent as a single patch -- with the
dirfd_path_init() code split out? Or that the second patch do all of the
structural changes to refactor dirfd_path_init() usage?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2018-10-13 8:09 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-09 7:02 [PATCH v3 0/3] namei: implement various lookup restriction AT_* flags Aleksa Sarai
2018-10-09 7:02 ` Aleksa Sarai
2018-10-09 7:02 ` [PATCH v3 1/3] namei: implement O_BENEATH-style " Aleksa Sarai
2018-10-09 7:02 ` Aleksa Sarai
2018-10-13 7:33 ` Al Viro
2018-10-13 7:33 ` Al Viro
2018-10-13 8:05 ` Al Viro
2018-10-13 8:05 ` Al Viro
2018-10-13 8:20 ` Aleksa Sarai
2018-10-13 8:20 ` Aleksa Sarai
2018-10-13 8:09 ` Aleksa Sarai [this message]
2018-10-13 8:09 ` Aleksa Sarai
2018-10-09 7:02 ` [PATCH v3 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution Aleksa Sarai
2018-10-09 7:02 ` Aleksa Sarai
2018-10-09 7:02 ` [PATCH v3 3/3] namei: aggressively check for nd->root escape on ".." resolution Aleksa Sarai
2018-10-09 7:02 ` Aleksa Sarai
2018-10-09 15:19 ` Jann Horn
2018-10-09 15:19 ` Jann Horn
2018-10-09 15:37 ` Aleksa Sarai
2018-10-09 15:37 ` Aleksa Sarai
2018-10-09 16:46 ` Jann Horn
2018-10-09 16:46 ` Jann Horn
2018-10-13 8:22 ` Al Viro
2018-10-13 8:22 ` Al Viro
2018-10-13 8:53 ` Aleksa Sarai
2018-10-13 8:53 ` Aleksa Sarai
2018-10-13 9:04 ` Al Viro
2018-10-13 9:04 ` Al Viro
2018-10-13 9:27 ` Aleksa Sarai
2018-10-13 9:27 ` Aleksa Sarai
2018-10-17 15:23 ` [PATCH v3 0/3] namei: implement various lookup restriction AT_* flags Aleksa Sarai
2018-10-17 15:23 ` Aleksa Sarai
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=20181013080907.yqpuy3zbbfe46gm4@ryuk \
--to=cyphar@cyphar.com \
--cc=arnd@arndb.de \
--cc=bfields@fieldses.org \
--cc=christian@brauner.io \
--cc=containers@lists.linux-foundation.org \
--cc=dev@opencontainers.org \
--cc=dhowells@redhat.com \
--cc=drysdale@google.com \
--cc=ebiederm@xmission.com \
--cc=jannh@google.com \
--cc=jlayton@kernel.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=tycho@tycho.ws \
--cc=viro@ZenIV.linux.org.uk \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).