linux-kernel - Re: [PATCH 00/17] clean up readlinks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160928021727.GN19539@ZenIV.linux.org.uk>
Date:   Wed, 28 Sep 2016 03:17:27 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Miklos Szeredi <mszeredi@...hat.com>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        David Howells <dhowells@...hat.com>,
        Tyler Hicks <tyhicks@...onical.com>
Subject: Re: [PATCH 00/17] clean up readlinks

On Tue, Sep 27, 2016 at 11:38:33AM +0200, Miklos Szeredi wrote:
> >         I have no problem with "let's get rid of generic_readlink" - not that
> > it bought us much, but sure, if you want to have decision made based upon
> > the combination of flags, let's do it.  Just make NULL ->readlink + non-NULL
> > ->get_link() mean generic_readlink(), and we are done.
> 
> Indeed.  Except it really should be the other way round:
> 
> - .get_link always returning the symlink body
> - only proc setting .jump_link to do its thing
> - RIP .readlink

> But that's an extra branch in the symlink following.  I was worried
> about that and hence gone for the unification of the two.

Symlink traversal is a much hotter path than readlink() would ever be.
What's more, we do have jumps on normal symlink traversal - after all,
absolute symlinks are exactly that; it's "jump to root, then traverse
the following sequence of components".  So having ->get_link() that
includes jumps is not that much of a stretch (note that it could both
jump and return a relative pathname to traverse after that; none of the
procfs-style ones do that, but there's no reason to prohibit that).

What I'd prefer is
	* it's a symlink iff it has ->get_link()
	* readlink(2) on a symlink is normally just using generic_readlink()
	* that can be overridden by supplying a ->readlink() method.
	* the first time readlink() hits a symlink it will check both
->get_link() and ->readlink() presence.  Then, if it's a normal symlink,
the inode will get marked as such and all subsequent calls will just call
generic_readlink().  IOW, I would go for
	if (unlikely(!marked)) {
		if ->readlink is present
			call ->readlink and return
		if ->get_link is absent
			fail
		mark
	}
	call generic_readlink

> Yeah.  We can do your above suggestion, it's certainly less brittle.
> But I think it's rather confusing, having ->get_link normally do
> readlink, except for proc, where readlink is done by ->readlink.

	->readlink() is just an override for the cases when readlink(2) wants
to fake something (or, as in case of AFS ugliness, is used on non-symlinks).
The primary function of symlinks is traversal, not readlink...