linux-kernel - Re: [PATCH 4/4] fs/dcache: Avoid the try_lock loops in dentry

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: <87y3js36s7.fsf@linutronix.de>
Date:   Fri, 16 Feb 2018 23:32:08 +0100
From:   John Ogness <john.ogness@...utronix.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Christoph Hellwig <hch@....de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] fs/dcache: Avoid the try_lock loops in dentry_kill()

On 2018-02-16, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> On Fri, Feb 16, 2018 at 7:09 AM, John Ogness <john.ogness@...utronix.de> wrote:
>> dentry_kill() holds dentry->d_lock and needs to acquire both
>> dentry->d_inode->i_lock and dentry->d_parent->d_lock. This cannot be
>> done with spin_lock() operations because it's the reverse of the
>> regular lock order. To avoid ABBA deadlocks it is done with two
>> trylock loops.
>>
>> Trylock loops are problematic in two scenarios:
>
> I don't mind this patch series per se (although I would really like Al
> to ack it), but this particular patch I hate.
>
> Why?
>
>> Avoid the trylock loops by using dentry_lock_inode() and lock_parent()
>> which take the locks in the appropriate order. As both functions drop
>> dentry->lock briefly, this requires rechecking of the dentry content
>> as it might have changed after dropping the lock.
>
> I think the trylock should be done first, and then you don't need that
> recheck for the common case.
>
> I realize that the recheck itself isn't expensive, but it's mostly
> about the code flow and the comment:
>
>> +                * Recheck refcount as it might have been incremented while
>> +                * d_lock was dropped.
>
> the thing is, 99.9% of the time the d_lock wasn't dropped, so that
> "while d_lock was dropped" comment is misleading.
>
> Re-organizing it to do the trylock fastpath explicitly here and not
> bothering with the re-check etc crid for the common case is the rioght
> thing to do.
>
> And the old code was actually organized exactly that way, with a
>
>         if (inode && unlikely(!spin_trylock(&inode->i_lock)))
>                 goto failed;
>
> at the top.
>
> But instead of having that unlikely "failed" case do the complex
> thing, you made the *normal* case do the complex thing.
>
> So NAK on this.

lock_parent() already has the problem you are referring to. Callers are
required to recheck the dentry contents and check the returned parent
because they do not know if the trylock succeeded. See
d_prune_aliases(), for example.

Would you like my v2 to fixup lock_parent() semantics to address your
concerns there as well?

John Ogness