On Tue, Jun 20, 2006 at 01:00:07AM +0200, Willy Tarreau wrote:
> On Mon, Jun 19, 2006 at 07:04:05PM -0300, Marcelo Tosatti wrote:
>
> > Think this is the right thing to do, except that it must be guaranteed
> > that the inode struct won't be freed in the meantime, need to grab a
> > reference to it.
>
> OK, I believe it will be right this time. I took inspiration from your
> precedent patch to sys_unlink().
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 42cce98..374b767 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1478,12 +1478,16 @@ exit:
> int vfs_unlink(struct inode *dir, struct dentry *dentry)
> {
> int error;
> + struct inode *inode;
>
> error = may_delete(dir, dentry, 0);
> if (error)
> return error;
>
> - double_down(&dir->i_zombie, &dentry->d_inode->i_zombie);
> + inode = dentry->d_inode;
> + atomic_inc(&inode->i_count);
> + double_down(&dir->i_zombie, &inode->i_zombie);
> +
> error = -EPERM;
> if (dir->i_op && dir->i_op->unlink) {
> DQUOT_INIT(dir);
> @@ -1495,7 +1499,9 @@ int vfs_unlink(struct inode *dir, struct
> unlock_kernel();
> }
> }
> - double_up(&dir->i_zombie, &dentry->d_inode->i_zombie);
> + double_up(&dir->i_zombie, &inode->i_zombie);
> + iput(inode);
> +
> if (!error) {
> d_delete(dentry);
> inode_dir_notify(dir, DN_DELETE);
Yeah thats better.
> BTW, I might be wrong because my knowledge in this area is rather poor, but
> I now believe that your previously proposed fix below indeed is not needed
> at all :
>
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 42cce98..69da199 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1509,6 +1511,7 @@ asmlinkage long sys_unlink(const char *
> > char * name;
> > struct dentry *dentry;
> > struct nameidata nd;
> > + struct inode *inode = NULL;
> >
> > name = getname(pathname);
> > if(IS_ERR(name))
> > @@ -1527,11 +1530,16 @@ asmlinkage long sys_unlink(const char *
> > /* Why not before? Because we want correct error value */
> > if (nd.last.name[nd.last.len])
> > goto slashes;
>
> ---- 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.
-
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]
