Re: Linux 2.4.33-rc1

[Marcelo Tosatti <marcelo@kvack.org>]

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.