linux-kernel - Re: [BUG] fs/dcache: might_sleep is called under a spinlock

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: <20171003031920.GI21978@ZenIV.linux.org.uk>
Date:   Tue, 3 Oct 2017 04:19:20 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Jia-Ju Bai <baijiaju1990@....com>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [BUG] fs/dcache: might_sleep is called under a spinlock

On Tue, Oct 03, 2017 at 10:38:25AM +0800, Jia-Ju Bai wrote:
> According to fs/dcache.c, might_sleep is called under a spinlock,
> and the function call path is:
> d_prune_aliases (acquire the spinlock)
>   dput
>     might_sleep
> 
> This bug is found by my static analysis tool and my code review.
> A possible fix is to remove might_sleep in dput.

... or to fix your static analysis tool.  First of all, that call
of dput() really *can* block and if we had inode->i_lock or dentry->d_lock
still held at that point we'd have a real bug.  However, __dentry_kill()
there is called with dentry->d_inode == inode and inode->i_lock held,
so dentry->d_inode is stable until inode->i_lock is dropped.  Said
__dentry_kill() contains
        if (dentry->d_inode)
                dentry_unlink_inode(dentry);
with inode->i_lock held until that point.  dentry_unlink_inode() starts
with
        struct inode *inode = dentry->d_inode;
        bool hashed = !d_unhashed(dentry);

        if (hashed)
                raw_write_seqcount_begin(&dentry->d_seq);
        __d_clear_type_and_inode(dentry);
        hlist_del_init(&dentry->d_u.d_alias);
        if (hashed)
                raw_write_seqcount_end(&dentry->d_seq);
        spin_unlock(&dentry->d_lock);
        spin_unlock(&inode->i_lock);
so
	1) inode in there is guaranteed to be equal to the argument of
d_prune_aliases() and
	2) both dentry->d_lock and inode->i_lock are dropped before
dentry_unlink_inode() returns.  inode->i_lock is not regained in the
rest of __dentry_kill(); dentry->d_lock is regained and dropped before
__dentry_kill() returns.  IOW, we are fine - dput() in d_prune_aliases()
is called without any spinlocks held.

That, BTW, is the reason for
                                goto restart;
in there, instead of just continuing the loop - if we get to that point,
the list of aliases might have changed.

Removing might_sleep() in dput() would've been wrong - it really might
sleep when called from that point.  Here's how: we used to have two
links to the same file - foo/bar and baz/barf.  baz/barf used to be
opened, then rm -rf baz happened and later we'd called d_prune_aliases()
on the inode of foo/bar.  And as the loop had been executed on one CPU,
on another the opened file got closed, dropping the last reference to
dentry that used to be baz/barf.  Note that its parent (the thing that
used to be dentry of baz) is unhashed and the only contributor to its
refcount is our dentry, so dput(parent) *does* drop the last remaining
reference, triggering the final iput() on inode of baz, along with
freeing on-disk inode, doing disk IO, etc.

Again, it's not that we can't block in that dput() - it's that __dentry_kill()
drops all spinlocks.