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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAJfpegspVwtuUufPVbpLRVQ25dp=CUPbLLkdJLzgPc0TD2ETFg@mail.gmail.com>
Date:   Sat, 17 Dec 2016 09:49:11 +0100
From:   Miklos Szeredi <miklos@...redi.hu>
To:     Al Viro <viro@...iv.linux.org.uk>
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 Sat, Dec 17, 2016 at 12:08 AM, Al Viro <viro@...iv.linux.org.uk> wrote:
> 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.

Have you looked?  Because in actual fact they don't.

 Theoretically it's either:

 - kmalloc + fill + readlink_copy + kfree  -->  kmalloc + fill +
set_delayed_call

 - declare char[] on stack + fill + readlink_copy --> kmalloc + fill +
set_delayed_call

Presumably it's the second one you are talking about becoming more
complex.  There's exactly one instance of that in the tree and it
actually becomes cleaner after the change.

Current code does:

  - guess max link size to be 50 (very scientifically I'm sure, but no
explanation given)
  - call filler
  - hope it didn't get truncated

Which becomes:

  - call filler which allocates correctly sized buffer.

> 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.

With the signature change we get a consistent interface for reading
the contents of symlinks.  With that it will never make sense to play
the stupid get_ds/set_ds() games that we've had.  And no need to
duplicate helper functions, like page_readlink() that is exactly the
same as page_getlink() only for the different interface.  And no need
to export readlink_copy() which is something the filesystems never
actually want to care about.

Having different interfaces for the same thing is going to be more
complex.  I just don't get it what you are opposed to here.

Thanks,
Miklos