Re: Linux 2.4.33-rc1

Willy Tarreau
2006-06-19T23:04:13Z

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