linux-kernel - Re: [GIT PULL (resend)] readlink cleanup

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 16 Dec 2016 23:08:23 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Miklos Szeredi <miklos@...redi.hu>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [GIT PULL (resend)] readlink cleanup

On Fri, Dec 16, 2016 at 11:48:59PM +0100, Miklos Szeredi wrote:

> This is a rework of the readlink cleanup patchset from the last cycle.  Now
> readlink(2) does the following:
> 
>  - if i_op->readlink() is non-NULL (only proc and afs mountpoints for now)
>    then it calls that
> 
>  - otherwise call i_op->get_link()
> 
>  - signature of ->readlink() now matches that of ->get_link()
> 
> In particular this last bullet point buys us:
> 
>  - less complexity, because we already handle the delayed free of the
>    buffer and copying to userspace due to ->get_link() being the normal way
>    to read the symlink

Less complexity where, exactly?  In the caller the life does not become
any simpler - instead of "call ->readlink() and bugger off" you have
"call ->readlink() and go through the same motions as in ->get_link()-based
case".  In the instances it becomes _more_ complex.

What's more, this new signature for ->readlink() makes no sense - instead of
"symlink traversal does not involve resolving a pathname, so we have to
fake one for readlink(2)" you get something resembling ->get_link(), which
would _not_ function as ->get_link() ought to.  But it can be called by the
same codepath that calls ->get_link(), saving us the burden of returning
without doing what ->get_link-based case would - we still get to check if
->readlink() is there, but we rejoin the common path immediately.  And AFAICS
that's the _only_ benefit of that signature change - making it possible to
reuse a few lines that adapt ->get_link() to readlinkat(2) needs.

IOW, I'm still not convinced.  Beginning of the series is fine - having
NULL ->readlink() interpreted for symlinks as "no override, use
generic_readlink()" makes a lot of sense.  This part, IMO, does not.