d_add on negative dentry?

d_add on negative dentry?

[Urban Widmark]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1009 bytes --]


Is it valid to call d_add on a negative dentry?
(or on a dentry that is already linked in d_hash, but all negative
 dentries are, right?)

I'm guessing it isn't because I think that is how I can get my machine to
hang in d_lookup, looping on a corrupt d_hash list.


The problem can be reproduced like this. /mnt/smb is a smbfs mount of
/mnt/samba/export from a samba server on localhost.

/mnt/smb% ls
aa
/mnt/smb% rm aa
/mnt/smb% touch /mnt/samba/export/aa
/mnt/smb% ls
ls: aa: No such file or directory

And shortly after it will lock up completely.


My printk's tell me that a negative dentry is still hashed since d_hash is
non-empty. d_add calls d_instantiate and d_rehash, the later adds it to a
d_hash list without first removing it. But it was already linked so now 2
extra dentries are also pointing to this dentry. And it is then no longer
a list ...

The attached patch fixes things for me. Comments?

Oh, and I *think* ncpfs has the same problem. But that's just from reading
the code.

/Urban

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1064 bytes --]

diff -urN -X exclude linux-2.4.2-ac11-orig/fs/smbfs/cache.c linux-2.4.2-ac11-smbfs/fs/smbfs/cache.c
--- linux-2.4.2-ac11-orig/fs/smbfs/cache.c	Thu Feb 22 20:52:03 2001
+++ linux-2.4.2-ac11-smbfs/fs/smbfs/cache.c	Mon Mar  5 23:48:12 2001
@@ -167,6 +167,7 @@
 	struct inode *newino, *inode = dentry->d_inode;
 	struct smb_cache_control ctl = *ctrl;
 	int valid = 0;
+	int hashed = 0;
 	ino_t ino = 0;
 
 	qname->hash = full_name_hash(qname->name, qname->len);
@@ -181,9 +182,11 @@
 		newdent = d_alloc(dentry, qname);
 		if (!newdent)
 			goto end_advance;
-	} else
+	} else {
+		hashed = 1;
 		memcpy((char *) newdent->d_name.name, qname->name,
 		       newdent->d_name.len);
+	}
 
 	if (!newdent->d_inode) {
 		smb_renew_times(newdent);
@@ -191,7 +194,10 @@
 		newino = smb_iget(inode->i_sb, entry);
 		if (newino) {
 			smb_new_dentry(newdent);
-			d_add(newdent, newino);
+			if (hashed)
+				d_instantiate(newdent, newino);
+			else
+				d_add(newdent, newino);
 		}
 	} else
 		smb_set_inode_attr(newdent->d_inode, entry);

Re: d_add on negative dentry?

[Alexander Viro]

On Tue, 6 Mar 2001, Urban Widmark wrote:

> 
> Is it valid to call d_add on a negative dentry?
> (or on a dentry that is already linked in d_hash, but all negative
>  dentries are, right?)

Not all of them. It _is_ legal to do d_add() on a negative dentry.
Doing that for hashed dentries is a bug. Use d_instantiate() instead.
							Cheers,
								Al

PS: as for the patch, better make it
	d_instantiate(...);
	if (!hashed)
		d_rehash(...);

Re: d_add on negative dentry?

[Petr Vandrovec]

On  5 Mar 01 at 18:08, Alexander Viro wrote:
> On Tue, 6 Mar 2001, Urban Widmark wrote:
> 
> > 
> > Is it valid to call d_add on a negative dentry?
> > (or on a dentry that is already linked in d_hash, but all negative
> >  dentries are, right?)
> 
> Not all of them. It _is_ legal to do d_add() on a negative dentry.
> Doing that for hashed dentries is a bug. Use d_instantiate() instead.
>                             Cheers,
>                                 Al
> 
> PS: as for the patch, better make it
>     d_instantiate(...);
>     if (!hashed)
>         d_rehash(...);

It could explain why I'm getting once a month CPU spinning in d_lookup()
because of some circular list is no more one circle... 
Many thanks, I'll apply it to ncpfs ASAP.
                                        Best regards,
                                                Petr Vandrovec
                                                vandrove@vc.cvut.cz