linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: John Johansen <john.johansen@canonical.com>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [git pull] apparmor fix for __d_path() misuse
Date: Wed, 7 Dec 2011 00:16:43 +0000	[thread overview]
Message-ID: <20111207001643.GN2203@ZenIV.linux.org.uk> (raw)
In-Reply-To: <CA+55aFwn=n5H_PuaRhnVPpMtfeig8nSi+Wj4Yzv6VBdjkUBEjw@mail.gmail.com>

On Tue, Dec 06, 2011 at 03:45:15PM -0800, Linus Torvalds wrote:
> How about this change:
>  - don't change 'root' (and mark it const)
>  - if we hit the expected root, we're all happy and do what we do now
>  - if we hit some *unexpected* root (the "global root") add a '?' or
> something at the head of the path.
> 
> End result: callers like getcwd() can trivially replace their current
> "path_equal(&tmp,&root)" (or whatever they do) with just checking the
> first character of the end result. A good path always starts with '/'.

You get broken /proc/self/mountinfo for chrooted processes with that patch.
You also get /proc/mounts contents change for the same.

Moreover, while we _probably_ can get away with that "prepend '?'",
we'll need to make sure that all checks are comparing with '?', _not_
with '/', or you'll get nasty surprises when __d_path() gets called
on e.g. pipe dentry (pipe:[...]).  And while we are at it, we'd better
document that "->d_dname() should never use '?' as the first character"
restriction we've got.

I don't know... playing with magical substrings in what it returns is,
IMO, a bad idea.  I really wonder if we'd be better off with just
this:
	__d_path(path, root, buf, buflen) - expects non-NULL in
root->mnt, never changes root, returns NULL if path is not under root
	d_absolute_path(path, ancestor, buf, buflen) - grabs the
reference to the most remote ancestor it can find, puts pathname
into buf, never returns NULL.

Let tomoyo use that one and path_put(ancestor) afterwards (or look at
it first, if it cares).  And let apparmor do the following:
	* first call __d_path(), unless asked not to.  If it returns
non-NULL, great we've got that path, game over.  Otherwise call
d_absolute_path() and log that partial pathname, check where we'd got,
etc.  Just remember to path_put(ancestor) after that.

We are trying to shove two different things in one function and result
is ugly; so let's just split it instead of trying to breed weird
hybrids.

Comments?

  parent reply	other threads:[~2011-12-07  0:16 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-12-06 15:48 [git pull] apparmor fix for __d_path() misuse Al Viro
2011-12-06 16:41 ` Al Viro
2011-12-06 17:21   ` Linus Torvalds
2011-12-06 19:54 ` Linus Torvalds
2011-12-06 20:53   ` Al Viro
2011-12-06 21:07     ` Linus Torvalds
2011-12-06 21:41       ` Al Viro
2011-12-06 22:48         ` John Johansen
2011-12-06 22:19       ` John Johansen
2011-12-06 22:41         ` Al Viro
2011-12-06 23:12           ` John Johansen
2011-12-06 23:45             ` Linus Torvalds
2011-12-07  0:09               ` John Johansen
2011-12-07  0:16               ` Al Viro [this message]
2011-12-07  0:39                 ` Al Viro
2011-12-07  0:42                   ` Linus Torvalds
2011-12-07  1:10                     ` Al Viro
2011-12-07  1:37                       ` Al Viro
2011-12-07  1:44                         ` Al Viro
2011-12-07  2:21                         ` Linus Torvalds
2011-12-07  3:23                           ` Al Viro
2011-12-07  3:11                         ` John Johansen
2011-12-07  4:26                           ` John Johansen
2011-12-07  4:45                             ` Al Viro
2011-12-07  4:59                               ` Al Viro
2011-12-07  3:26                         ` Tetsuo Handa
2011-12-07  3:42                           ` Al Viro
2011-12-07  5:01                             ` Tetsuo Handa
2011-12-07  5:19                               ` Al Viro
2011-12-07  5:44                                 ` Tetsuo Handa
2011-12-07  6:54                                   ` Al Viro
2011-12-07  8:59                                     ` Tetsuo Handa
2011-12-07 16:32                                       ` Al Viro
2011-12-07 17:51                                       ` Al Viro
2011-12-07  0:39                 ` Linus Torvalds
2011-12-07  0:52                   ` Al Viro
2011-12-07  1:11                     ` Linus Torvalds
2011-12-07  1:23                       ` Al Viro
2011-12-07  2:02                         ` Linus Torvalds
2011-12-07  2:17                           ` Al Viro
2011-12-07  2:29                             ` Linus Torvalds

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=20111207001643.GN2203@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=john.johansen@canonical.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /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).