Re: [PATCH] fix cramfs making duplicate entries in inode cache

[Dave Johnson]

Dave Johnson writes:
> Phillip Lougher writes:
> > Doesn't iget_locked() assume inode numbers are unique?
> > 
> > In Cramfs inode numbers are set to 1 for non-data inodes (fifos, 
> > sockets, devices, empty directories), i.e
> > 
> > %stat device namedpipe
> >    File: `device'
> >    Size: 0               Blocks: 0          IO Block: 4096   character 
> > special file
> > Device: 700h/1792d      Inode: 1           Links: 1     Device type: 1,1
> > Access: (0644/crw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> > Access: 1970-01-01 01:00:00.000000000 +0100
> > Modify: 1970-01-01 01:00:00.000000000 +0100
> > Change: 1970-01-01 01:00:00.000000000 +0100
> >    File: `namedpipe'
> >    Size: 0               Blocks: 0          IO Block: 4096   fifo
> > Device: 700h/1792d      Inode: 1           Links: 1
> > Access: (0644/prw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
> > Access: 1970-01-01 01:00:00.000000000 +0100
> > Modify: 1970-01-01 01:00:00.000000000 +0100
> > Change: 1970-01-01 01:00:00.000000000 +0100
> > 
> > Should iget5_locked() be used here?
> 
> Yep, that was busted.  Below patch should be better.

Doh, 2nd attempt wasn't verifying the inode number in the test
function to protect against hash collisions (iget_locked does this but
iget5_locked doesn't)

It should be good now.

-- 
Dave Johnson
Starent Networks

===== fs/cramfs/inode.c 1.42 vs edited =====
--- 1.42/fs/cramfs/inode.c	2005-07-14 12:24:48 -04:00
+++ edited/fs/cramfs/inode.c	2005-08-19 22:06:44 -04:00
@@ -42,12 +42,46 @@
 #define CRAMINO(x)	((x)->offset?(x)->offset<<2:1)
 #define OFFSET(x)	((x)->i_ino)
 
+
+static int cramfs_iget5_test(struct inode *inode, void *opaque)
+{
+	struct cramfs_inode * cramfs_inode = (struct cramfs_inode *)opaque;
+
+	if (inode->i_ino != CRAMINO(cramfs_inode))
+		return 0; /* does not match */
+
+	if (inode->i_ino != 1)
+		return 1;
+
+	/* all empty directories, char, block, pipe, and sock, share inode #1 */
+  
+	if ((inode->i_mode != cramfs_inode->mode) ||
+	    (inode->i_gid != cramfs_inode->gid) ||
+	    (inode->i_uid != cramfs_inode->uid))
+		return 0; /* does not match */
+  
+	if ((S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) &&
+	    (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
+		return 0; /* does not match */
+
+	return 1; /* matches */
+}
+
+static int cramfs_iget5_set(struct inode *inode, void *opaque)
+{
+	struct cramfs_inode * cramfs_inode = (struct cramfs_inode *)opaque;
+	inode->i_ino = CRAMINO(cramfs_inode);
+	return 0;
+}
+
 static struct inode *get_cramfs_inode(struct super_block *sb, struct cramfs_inode * cramfs_inode)
 {
-	struct inode * inode = new_inode(sb);
+	struct inode * inode = iget5_locked(sb, CRAMINO(cramfs_inode),
+					    cramfs_iget5_test, cramfs_iget5_set,
+					    cramfs_inode);
 	static struct timespec zerotime;
 
-	if (inode) {
+	if (inode && (inode->i_state & I_NEW)) {
 		inode->i_mode = cramfs_inode->mode;
 		inode->i_uid = cramfs_inode->uid;
 		inode->i_size = cramfs_inode->size;
@@ -63,7 +99,6 @@
 		   but it's the best we can do without reading the directory
 	           contents.  1 yields the right result in GNU find, even
 		   without -noleaf option. */
-		insert_inode_hash(inode);
 		if (S_ISREG(inode->i_mode)) {
 			inode->i_fop = &generic_ro_fops;
 			inode->i_data.a_ops = &cramfs_aops;
@@ -75,6 +110,7 @@
 			init_special_inode(inode, inode->i_mode,
 				old_decode_dev(cramfs_inode->size));
 		}
+		unlock_new_inode(inode);
 	}
 	return inode;
 }

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/