Messages in this thread | | | Date | Mon, 7 Jun 2010 17:08:19 -0700 (PDT) | From | Linus Torvalds <> | 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
| |