Re: Linux 2.4.33-rc1

[Willy Tarreau <w@1wt.eu>]

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);

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 ?

>  exit1:
>  	path_release(&nd);
>  exit:

Thanks,
Willy