Hi Marcelo,
On Wed, Jun 21, 2006 at 10:50:15AM -0300, Marcelo Tosatti wrote:
> Hi Willy,
>
> On Wed, Jun 21, 2006 at 12:23:57AM +0200, Willy Tarreau wrote:
> > On Mon, Jun 19, 2006 at 08:45:06PM -0300, Marcelo Tosatti wrote:
> > > >
> > > > ---- from here ----
> > > >
> > > >
> > > > > + inode = dentry->d_inode;
> > > > > + if (inode)
> > > > > + atomic_inc(&inode->i_count);
> > > > > error = vfs_unlink(nd.dentry->d_inode, dentry);
> > > > > exit2:
> > > > > dput(dentry);
> > > > > }
> > > > > up(&nd.dentry->d_inode->i_sem);
> > > > > + if (inode)
> > > > > + iput(inode);
> > > >
> > > > ---- to here ----
> > > >
> > > > I believe that nd.dentry->d_inode cannot vanish because it is protected by the
> > > > down(->i_sem) before and the up(->i_sem) after. Am I right or am I missing
> > > > something important ?
> > >
> > > Indeed it can't, but dentry->d_inode will be set to NULL by
> > > nfs_unlink->nfs_safe_remove->d_delete. Thus the problem.
> >
> > What puzzles me is how are we supposed to up(&nd.dentry->d_inode->i_sem) if
> > dentry->d_inode can become NULL ?
>
> down(&nd.dentry->d_inode->i_sem);
> dentry = lookup_hash(&nd.last, nd.dentry);
> error = vfs_unlink(nd.dentry->d_inode, dentry);
> ^^^^^^^
>
> It does vfs_unlink(parent, child). The child is killed.
>
> > simply by keeping a copy of it ? I thought
> > that the down() protected the whole thing, but may be that's stupid anyway.
>
> It does not protect from last reference going away, which is what
> happens when NFS d_delete's the dentry. dentry_iput() gets called, and
> maybe the last reference to "struct inode" might be gone too.
OK for this one, but what I mean is that vfs_unlink() will kill dentry
(its second argument, which your fix protects), while the down()/up()
we initially suspected, and tried to protect, play with nd.dentry->d_inode,
which is the directory in which we are removing dentry. And I can't see how
nd.dentry->d_inode can be killed during vfs_unlink(), reason why I thought
the fix above did not provide any benefit.
> > I've been running rc1 without this patch for a few hours and during kernel
> > compiles without a problem, so I'm not sure about what to think about the
> > other changes which were apparently harmless too :-/
>
> Use NFS... :)
I'm using it at home right now, doing lots of git-checkout -f and make -j
dep ; make -j 4 bzImage modules on SMP without a glitch. Right now I'm
running rc2 without trouble and I noticed you did not merge the fix for
sys_unlink() that we discussed above. May be you simply have forgotten it
or may be we are in sync but were talking about different things.
> > Well, if I resume it right, we only need to merge your patch and mine and
> > it *should* be OK.
> >
> > BTW, I've been reviewing the PaX patch and found *at least* one patch
> > that should be merged (fix for oops). I'll send it separately, and it's
> > queued in -upstream.
>
> Merged, thanks.
Thanks,
Willy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
