Re: [PATCH] First casuality of hlist poisoning in 2.5.70

[Linus Torvalds]

On Wed, 11 Jun 2003, Trond Myklebust wrote:
> 
>   This patch removes the Oops that occurs when either the source or
> the target of a d_move() operation is unhashed. It is currently
> triggered by the NFS sillyrename code.

Btw, thinking more about this, the patch can't be right.

> +		if (!hlist_unhashed(&dentry->d_hash))
> +			hlist_del_rcu(&dentry->d_hash);

This leaves the d_hash pointers as crapola.

> +		if (!hlist_unhashed(&target->d_hash)) {
> +			hlist_add_head_rcu(&dentry->d_hash, target->d_bucket);
> +			dentry->d_vfs_flags &= ~DCACHE_UNHASHED;
> +		} else
> +			dentry->d_vfs_flags |= DCACHE_UNHASHED;

And this will fix it only if target is hashed.

So either the patch depends on the target being hashed (and if so, it just 
shouldn't have the test, or have a BUG_ON() instead), or the patch is 
buggy.

I have this suspicion that the logical patch would be something like this:

	if (hlist_unhashed(&target->d_hash)) {
		/* Unhash the dentry too */
		__d_drop(dentry);
	} else {
		/* Rehash the dentry onto the same hash as the target */
		hlist_del_rcu(&dentry->d_hash);
		hlist_add_head_rcu(&dentry->d_hash, target->d_bucket);
		dentry->d_vfs_flags &= ~DCACHE_UNHASHED;   
	}

At least this would make slightly more sense than the patch.

HOWEVER, I actually suspect that the target really _cannot_ be unhashed, 
and that the test makes no sense, and the sequence should just be

	/* Rehash the dentry onto the same hash as the target */
	hlist_del_rcu(&dentry->d_hash);
	hlist_add_head_rcu(&dentry->d_hash, target->d_bucket);
	dentry->d_vfs_flags &= ~DCACHE_UNHASHED;

this is actually identical to what we have right now, except for one small 
detail: the current 2.5.x tree does not clear the DCACHE_UNHASHED bit in 
d_move().

Also, if a unhashed dentry really is ok, then I think the test for 

        /* Move the dentry to the target hash queue, if on different bucket */
        if (dentry->d_bucket != target->d_bucket) {
		...

should really be something like

	if (!d_hashed(dentry) || dentry->d_bucket != target->d_bucket) {
		...

because otherwise the dentry may well share the same bucket as the target, 
but if it wasn't hashed before, it won't be hashed after either. 

But I suspect that neither dentry nor target should really ever be
unhashed by the time we call d_move(). That's reinforced by the fact that
it looks like a unhashed dentry in d_move() would have been a silent bug
previously - staying unhashed if it just shared the bucket.

Al, I'll be really happy having you go over this code too. And whatever we 
decide is right (enforcing hashedness or whatever), we should assert it, 
because clearly d_move() has been a bit too subtle for us so far.

		Linus