Re: Linux 2.4.33-rc1

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

On Sat, Jun 17, 2006 at 04:24:00PM +1000, Grant Coady wrote:
> On Sat, 17 Jun 2006 07:13:57 +0200, Willy Tarreau <w@1wt.eu> wrote:
> 
> >Hi Grant,
> >
> >On Sat, Jun 17, 2006 at 09:24:02AM +1000, Grant Coady wrote:
> >> On Sat, 17 Jun 2006 00:38:10 +0200, "Michal Piotrowski" <michal.k.k.piotrowski@gmail.com> wrote:
> >> 
> >> >Can you revert 42cce987d63d3048595b8b00d74be786707d5e5d commit?
> >> 
> >> Only if you point me at the patch, that string seems not to be 
> >> in the patch-2.4.33-rc1.gz file.
> >
> >First, revert this one :
> >
> >  http://kernel.org/git/?p=linux/kernel/git/marcelo/linux-2.4.git;a=commitdiff_plain;h=efc95599c0261dd7ab3a1d9071024ca140b4c644;hp=6601095e2de35f00325a33c8be6b548f81fe76d5
> >
> >--- a/fs/namei.c
> >+++ b/fs/namei.c
> >@@ -1479,19 +1479,20 @@ int vfs_unlink(struct inode *dir, struct
> > {
> > 	int error;
> > 
> >-	double_down(&dir->i_zombie, &dentry->d_inode->i_zombie);
> > 	error = may_delete(dir, dentry, 0);
> >-	if (!error) {
> >-		error = -EPERM;
> >-		if (dir->i_op && dir->i_op->unlink) {
> >-			DQUOT_INIT(dir);
> >-			if (d_mountpoint(dentry))
> >-				error = -EBUSY;
> >-			else {
> >-				lock_kernel();
> >-				error = dir->i_op->unlink(dir, dentry);
> >-				unlock_kernel();
> >-			}
> >+	if (error)
> >+		return error;
> >+
> >+	double_down(&dir->i_zombie, &dentry->d_inode->i_zombie);
> >+	error = -EPERM;
> >+	if (dir->i_op && dir->i_op->unlink) {
> >+		DQUOT_INIT(dir);
> >+		if (d_mountpoint(dentry))
> >+			error = -EBUSY;
> >+		else {
> >+			lock_kernel();
> >+			error = dir->i_op->unlink(dir, dentry);
> >+			unlock_kernel();
> > 		}
> > 	}
> > 	double_up(&dir->i_zombie, &dentry->d_inode->i_zombie);
> >
> >
> >It will put you back to the state where all your machines hanged at boot
> >with -hf32.5, then revert this one :
> >
> >  http://kernel.org/git/?p=linux/kernel/git/marcelo/linux-2.4.git;a=commitdiff_plain;h=f41e0ce901260d3d1ae5bd8bae34266891b4a65d;hp=925c7ce0a2d9a676cd8e4a2baf411b23cf6762d6
> >
> >--- a/fs/namei.c
> >+++ b/fs/namei.c
> >@@ -1479,7 +1479,7 @@ int vfs_unlink(struct inode *dir, struct
> > {
> > 	int error;
> > 
> >-	down(&dir->i_zombie);
> >+	double_down(&dir->i_zombie, &dentry->d_inode->i_zombie);
> > 	error = may_delete(dir, dentry, 0);
> > 	if (!error) {
> > 		error = -EPERM;
> >@@ -1491,14 +1491,14 @@ int vfs_unlink(struct inode *dir, struct
> > 				lock_kernel();
> > 				error = dir->i_op->unlink(dir, dentry);
> > 				unlock_kernel();
> >-				if (!error)
> >-					d_delete(dentry);
> > 			}
> > 		}
> > 	}
> >-	up(&dir->i_zombie);
> >-	if (!error)
> >+	double_up(&dir->i_zombie, &dentry->d_inode->i_zombie);
> >+	if (!error) {
> >+		d_delete(dentry);
> > 		inode_dir_notify(dir, DN_DELETE);
> >+	}
> > 	return error;
> > }
> > 
> >@@ -1607,18 +1607,19 @@ int vfs_link(struct dentry *old_dentry, 
> > 	struct inode *inode;
> > 	int error;
> > 
> >-	down(&dir->i_zombie);
> > 	error = -ENOENT;
> > 	inode = old_dentry->d_inode;
> > 	if (!inode)
> >-		goto exit_lock;
> >-
> >-	error = may_create(dir, new_dentry);
> >-	if (error)
> >-		goto exit_lock;
> >+		goto exit;
> > 
> > 	error = -EXDEV;
> > 	if (dir->i_dev != inode->i_dev)
> >+		goto exit;
> >+
> >+	double_down(&dir->i_zombie, &old_dentry->d_inode->i_zombie);
> >+
> >+	error = may_create(dir, new_dentry);
> >+	if (error)
> > 		goto exit_lock;
> > 
> > 	/*
> >@@ -1636,9 +1637,10 @@ int vfs_link(struct dentry *old_dentry, 
> > 	unlock_kernel();
> > 
> > exit_lock:
> >-	up(&dir->i_zombie);
> >+	double_up(&dir->i_zombie, &old_dentry->d_inode->i_zombie);
> > 	if (!error)
> > 		inode_dir_notify(dir, DN_CREATE);
> >+exit:
> > 	return error;
> > }
> > 
> >
> >And you will have something equivalent to -hf32.6 (you remember, I only
> >reverted the fix in -hf32.6, and did not merge Marcelo's fix for the fix).
> >Then we will have to decide whether we can fix it again or revert it
> >completely. I would have liked to Cc: Vadim Egorov, but I cannot find his
> >email.
> >
> 
> Hi Willy, this worked.  

OK Fine. However, I don't understand. From you oops, it looks like
dentry->d_inode suddenly gets NULL before calling this line :

        double_up(&dir->i_zombie, &dentry->d_inode->i_zombie);

I'm wondering if the unlink(dir, dentry) above can produce this. If this
is the case, the Vadim's fix is terribly broken. I also think that in
vfs_link(), we can encounter the same problem that Marcelo had to fix,
because double_down() is performed without prior checking that
old_dentry->d_inode is valid.

Marcelo, do you have Vadim's email somewhere ? I think he should help us
on this otherwise it would be better to revert his fix, as it has introduced
lots of sensible changes in the error path.

> Grant.

Thanks for the tests, Grant.
Willy