linux-kernel - Re: [V9fs-developer] [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: <4C0E53E0.3010300@linux.vnet.ibm.com>
Date:	Tue, 08 Jun 2010 07:29:52 -0700
From:	"Venkateswararao Jujjuri (JV)" <jvrao@...ux.vnet.ibm.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
CC:	Eric Van Hensbergen <ericvh@...il.com>,
	V9FS Developers <v9fs-developer@...ts.sourceforge.net>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [V9fs-developer] [GIT PULL] 9p file system bug fixes for 2.6.35-rc2

Linus Torvalds wrote:
> 
> 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".

Silly me. I sent out another patch using the dir inode passed through arguments.
But we still need to analyze the use of dentry->d_parent in other parts of code.. 

- JV


> 
> 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
> 
> ------------------------------------------------------------------------------
> ThinkGeek and WIRED's GeekDad team up for the Ultimate 
> GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the 
> lucky parental unit.  See the prize list and enter to win: 
> http://p.sf.net/sfu/thinkgeek-promo
> _______________________________________________
> V9fs-developer mailing list
> V9fs-developer@...ts.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/v9fs-developer


--
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/