[lkml]   [2010]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [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

    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


     \ /
      Last update: 2010-06-08 02:11    [W:3.473 / U:0.056 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site