linux-kernel - Re: [CFT] Re: VFS deadlock ?

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:	Thu, 21 Mar 2013 21:55:10 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Dave Jones <davej@...hat.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [CFT] Re: VFS deadlock ?

On Thu, Mar 21, 2013 at 9:37 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> FWIW, a relatively crude solution is this:
>
> -                       d_add(dentry, inode);
> -                       return NULL;
> +                       return d_materialise_unique(dentry, inode);
>
> It *is* crude, but it restores the assert, killing the deadlock and lets
> everything work more or less as it used to.  The case where things start
> to look odd is this:
>
> root@...-amd64:~# cd /proc/1/net/stat/; ls /proc/2/net/stat; /bin/pwd
> arp_cache  ndisc_cache  rt_cache
> /proc/2/net/stat
>
> IOW, if we were about to create a directory alias, the old dentry gets moved
> in new place.

Ugh, no, this is worse than the bug we're trying to fix.

However, I do wonder if we could take another approach... There's
really no reason why we should look up an old inode with iget_locked()
in proc_get_inode() and only fill it in if it is new. We might as well
just always create a new inode. The "iget_locked()" logic really comes
from the bad old days when the inode was the primary data structure,
but it's really the dentry that is the important data structure, and
the inode might as well follow the dentry, instead of the other way
around.

So why not just use "new_inode_pseudo()" instead? IOW, something like
this (totally untested, natch) patch? This way, if you have a new
dentry, you are guaranteed to always have a new inode. None of the
silly inode alias issues..

This seems too simple, but I don't see why iget_locked() would be any
better. It just wastes time hashing the inode that we aren't really
interested in hashing. The inode is always filled by the caller
anyway, so we migth as well just get a fresh pseudo-filesystem inode
without any crazy hashing..

                     Linus

Download attachment "patch.diff" of type "application/octet-stream" (672 bytes)