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

Linus Torvalds
2010-06-08T00:08:30Z

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