On Sun, Apr 2, 2017 at 4:59 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sun, Apr 2, 2017 at 10:01 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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.
So honestly, that fix to add DCACHE_RCUACCESS just looks like the
wrong thing to me.
Linus
