linux-kernel - Re: [git pull] vfs fixes

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]

Date:   Mon, 3 Apr 2017 01:30:45 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [git pull] vfs fixes

On Sun, Apr 02, 2017 at 05:10:08PM -0700, Linus Torvalds wrote:
> On Sun, Apr 2, 2017 at 4:59 PM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> > On Sun, Apr 2, 2017 at 10:01 AM, Al Viro <viro@...iv.linux.org.uk> wrote:
> >>         statx followup fixes, fix for a nasty corner case in path_init()
> >> leaving path.dentry in RCU mode pointing to a dentry without DCACHE_RCUACCESS
> >> and a fix for stack-smashing on alpha.
> >
> > These were apparently committed minutes before sending me the pull request.
> >
> > Why? What kind of testing did this all get?
> 
> Also, that RCU fix really stinks. It makes no sense. Any valid base
> for actual pathname lookup will already have the bit set, so the only
> issue is when people play games and use a non-path file descriptor
> without a pathname. Right?
> 
> And that case shouldn't actually use RCU lookup AT ALL!
> 
> By definition such a case will  just be immediately unlazied anyway in
> complete_walk(), so doing an RCU lookup on an empty path only adds
> overhead, and there is no actual point in doing an RCU walk on an
> empty pathname.
> 
> So I get the feeling that the proper fix would be just something like
> 
>       /* Don't RCU-lookup empty pathnames */
>       if ((flags & LOOKUP_RCU) && !*s)
>             flags &= ~LOOKUP_RCU;
> 
> at the very top of path_init(), which
> 
>  (a) makes the code more efficiant, since we don't do those
> unnecessary games with sequence numbers and RCU locking only to un-RCU
> it immedately
> 
> and
> 
>  (b) obviates the need for those DCACHE_RCUACCESS games entirely,
> since anything that can actually be used as a base for pathname lookup
> will already have that bit set, afaik.

Currently true and almost certainly will remain so.  Point taken, what you
are suggesting is better.  Actually, the invariant to watch for is
"no d_can_lookup() withtout DCACHE_RCUACCESS" and that we can trivially
enforce by one-liner change in d_flags_for_inode() -
s/DCACHE_DIRECTORY_TYPE/& | DCACHE_RCUACCESS/

OK...