linux-kernel - Re: [GIT PULL] 9p file system bug fixes for 2.6.35-rc2

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: <alpine.LFD.2.00.1006071654290.4506@i5.linux-foundation.org>
Date:	Mon, 7 Jun 2010 17:08:19 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Eric Van Hensbergen <ericvh@...il.com>
cc:	V9FS Developers <v9fs-developer@...ts.sourceforge.net>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] 9p file system bug fixes for 2.6.35-rc2

On Mon, 7 Jun 2010, Eric Van Hensbergen wrote:
> 
> jvrao (2):
>       Add a helper function to get fsgid for a file create.
>       9p: Add a wstat after TCREATE to fix the gid.

Quite frankly, this looks rather broken.

It uses "dentry->d_parent" without locking (it so happens to likely be ok, 
since we are in "create()" and thus should be holding the parent 
semaphore). On its own, that might be excusable (if people were even 
_aware_ of the this locking rule!), but it does so just to get the inode 
pointer to that parent.

And the only thing that makes it ok to access dentry->d_parent - the fact 
that we are in v9fs_create() - is also the thing that should have made 
people look at the arguments to the function and say "hmm".

We pass in the directory inode pointer as an argument to the create 
function! The code could have used that thing directly, instead of 
mucking around with dentry pointers that it had no business looking at.

I see why it seems to have happened: v9fs does the exact same thing for 
the pre-existing "v9fs_fid_lookup()". So there is history to this 
behavior.

Maybe people weren't aware of the fact that just dereferencing 
dentry->d_parent willy-nilly isn't actually allowed. That field changes. 
Sure, there are cases where it's ok, but this is a dangerous thing to do 
in general. 

In fact, the other thing that I find doing that whole "dentry->d_parent" 
thing seems to literally be broken. If you look at v9fs_fid_lookup(), 
you'll notice how it walks up the d_parent chain, and at that point you do 
NOT own the directory i_mutex, so at that point d_parent really _can_ be 
changing wildly due to concurrent renames or whatever.

So 9pfs seems to have some preexisting bugs in this area. I'm not going to 
pull new bug-prone code. See the other discussions about being tight this 
release about really _only_ taking regressions after the merge window 
closed.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/