Re: fishy ->put_inode usage in ntfs

[Christoph Hellwig <hch@lst.de>]

On Thu, Feb 10, 2005 at 02:59:32PM +0000, Anton Altaparmakov wrote:
> On Thu, 2005-02-10 at 15:50 +0100, Christoph Hellwig wrote:
> > On Thu, Feb 10, 2005 at 02:48:26PM +0000, Anton Altaparmakov wrote:
> > > If the igrab() were not done, it would be possible for clear_inode to be
> > > called on the 'parent' inode whilst at the same time one or more attr
> > > inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> > > happen...
> > 
> > What bad thing specificly?  If there's shared information we should
> > probably refcount them separately.
> 
> Each attr inode stores a pointer to its parent inode in NTFS_I(struct
> inode *vi)->ext.base_ntfs_ino.  This pointer would point to random
> memory if clear_inode is called on the parent whilst the attr inode is
> still in use.

Ok.  Al Viro suggested to put a spinlock around the bmp_ino pointer and
then igrab the master inode when accessing it, iput it when done.

Below is a rather half-backed patch to implement the suggestion.  It
compiles, but I'm pretty sure I introduced various bugs.

Also the new spinlock isn't nice, it'd probably be better to reuse some
other lock for this single field.


--- 1.85/fs/ntfs/dir.c	2004-11-05 13:48:35 +01:00
+++ edited/fs/ntfs/dir.c	2005-02-13 17:34:51 +01:00
@@ -1102,7 +1102,7 @@ static int ntfs_readdir(struct file *fil
 {
 	s64 ia_pos, ia_start, prev_ia_pos, bmp_pos;
 	loff_t fpos;
-	struct inode *bmp_vi, *vdir = filp->f_dentry->d_inode;
+	struct inode *bmp_vi = NULL, *vdir = filp->f_dentry->d_inode;
 	struct super_block *sb = vdir->i_sb;
 	ntfs_inode *ndir = NTFS_I(vdir);
 	ntfs_volume *vol = NTFS_SB(sb);
@@ -1250,17 +1250,14 @@ skip_index_root:
 	/* Get the offset into the index allocation attribute. */
 	ia_pos = (s64)fpos - vol->mft_record_size;
 	ia_mapping = vdir->i_mapping;
-	bmp_vi = ndir->itype.index.bmp_ino;
+
+	bmp_vi = ntfs_grab_bmp_inode(vdir);
 	if (unlikely(!bmp_vi)) {
-		ntfs_debug("Inode 0x%lx, regetting index bitmap.", vdir->i_ino);
-		bmp_vi = ntfs_attr_iget(vdir, AT_BITMAP, I30, 4);
-		if (IS_ERR(bmp_vi)) {
-			ntfs_error(sb, "Failed to get bitmap attribute.");
-			err = PTR_ERR(bmp_vi);
-			goto err_out;
-		}
-		ndir->itype.index.bmp_ino = bmp_vi;
+		ntfs_error(sb, "Failed to get bitmap attribute.");
+		err = -EINVAL;
+		goto err_out;
 	}
+
 	bmp_mapping = bmp_vi->i_mapping;
 	/* Get the starting bitmap bit position and sanity check it. */
 	bmp_pos = ia_pos >> ndir->itype.index.block_size_bits;
@@ -1445,6 +1442,8 @@ EOD:
 abort:
 	kfree(name);
 done:
+	if (bmp_vi)
+		ntfs_ungrab_bmp_inode(vdir);
 #ifdef DEBUG
 	if (!rc)
 		ntfs_debug("EOD, fpos 0x%llx, returning 0.", fpos);
@@ -1455,6 +1454,8 @@ done:
 	filp->f_pos = fpos;
 	return 0;
 err_out:
+	if (bmp_vi)
+		ntfs_ungrab_bmp_inode(vdir);
 	if (bmp_page)
 		ntfs_unmap_page(bmp_page);
 	if (ia_page) {
@@ -1537,8 +1538,16 @@ static int ntfs_dir_fsync(struct file *f
 
 	ntfs_debug("Entering for inode 0x%lx.", vi->i_ino);
 	BUG_ON(!S_ISDIR(vi->i_mode));
-	if (NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino)
-		write_inode_now(ni->itype.index.bmp_ino, !datasync);
+
+	if (NInoIndexAllocPresent(ni)) {
+		struct inode *bmp_inode = ntfs_grab_bmp_inode(vi);
+
+		if (bmp_inode) {
+			write_inode_now(bmp_inode, !datasync);
+			ntfs_ungrab_bmp_inode(vi);
+		}
+	}
+
 	ret = ntfs_write_inode(vi, 1);
 	write_inode_now(vi, !datasync);
 	err = sync_blockdev(vi->i_sb->s_bdev);
--- 1.154/fs/ntfs/inode.c	2004-11-09 11:42:05 +01:00
+++ edited/fs/ntfs/inode.c	2005-02-13 17:40:55 +01:00
@@ -388,6 +388,7 @@ void __ntfs_init_inode(struct super_bloc
 	ni->attr_list = NULL;
 	ntfs_init_runlist(&ni->attr_list_rl);
 	ni->itype.index.bmp_ino = NULL;
+	spin_lock_init(&ni->itype.index.bmp_ino_lock);
 	ni->itype.index.block_size = 0;
 	ni->itype.index.vcn_size = 0;
 	ni->itype.index.collation_rule = 0;
@@ -414,6 +415,29 @@ inline ntfs_inode *ntfs_new_extent_inode
 	return ni;
 }
 
+/*
+ * Grab primary inode when we're using the attr inode because they 
+ * share state.
+ */
+struct inode *ntfs_grab_bmp_inode(struct inode *inode)
+{
+	struct inode *bmp_inode;
+	ntfs_inode *ni = NTFS_I(inode);
+
+	spin_lock(&ni->itype.index.bmp_ino_lock);
+	bmp_inode = ni->itype.index.bmp_ino;
+	if (bmp_inode && !igrab(inode))
+		bmp_inode = NULL;
+	spin_unlock(&ni->itype.index.bmp_ino_lock);
+
+	return inode;
+}
+
+void ntfs_ungrab_bmp_inode(struct inode *inode)
+{
+	iput(inode);
+}
+
 /**
  * ntfs_is_extended_system_file - check if a file is in the $Extend directory
  * @ctx:	initialized attribute search context
@@ -1365,7 +1389,6 @@ static int ntfs_read_locked_attr_inode(s
 	 * Make sure the base inode doesn't go away and attach it to the
 	 * attribute inode.
 	 */
-	igrab(base_vi);
 	ni->ext.base_ntfs_ino = base_ni;
 	ni->nr_extents = -1;
 
@@ -1651,7 +1674,6 @@ skip_large_index_stuff:
 	 * Make sure the base inode doesn't go away and attach it to the
 	 * index inode.
 	 */
-	igrab(base_vi);
 	ni->ext.base_ntfs_ino = base_ni;
 	ni->nr_extents = -1;
 
@@ -2102,37 +2124,6 @@ err_out:
 	return -1;
 }
 
-/**
- * ntfs_put_inode - handler for when the inode reference count is decremented
- * @vi:		vfs inode
- *
- * The VFS calls ntfs_put_inode() every time the inode reference count (i_count)
- * is about to be decremented (but before the decrement itself.
- *
- * If the inode @vi is a directory with two references, one of which is being
- * dropped, we need to put the attribute inode for the directory index bitmap,
- * if it is present, otherwise the directory inode would remain pinned for
- * ever.
- */
-void ntfs_put_inode(struct inode *vi)
-{
-	if (S_ISDIR(vi->i_mode) && atomic_read(&vi->i_count) == 2) {
-		ntfs_inode *ni = NTFS_I(vi);
-		if (NInoIndexAllocPresent(ni)) {
-			struct inode *bvi = NULL;
-			down(&vi->i_sem);
-			if (atomic_read(&vi->i_count) == 2) {
-				bvi = ni->itype.index.bmp_ino;
-				if (bvi)
-					ni->itype.index.bmp_ino = NULL;
-			}
-			up(&vi->i_sem);
-			if (bvi)
-				iput(bvi);
-		}
-	}
-}
-
 static void __ntfs_clear_inode(ntfs_inode *ni)
 {
 	/* Free all alocated memory. */
@@ -2198,18 +2189,6 @@ void ntfs_clear_big_inode(struct inode *
 {
 	ntfs_inode *ni = NTFS_I(vi);
 
-	/*
-	 * If the inode @vi is an index inode we need to put the attribute
-	 * inode for the index bitmap, if it is present, otherwise the index
-	 * inode would disappear and the attribute inode for the index bitmap
-	 * would no longer be referenced from anywhere and thus it would remain
-	 * pinned for ever.
-	 */
-	if (NInoAttr(ni) && (ni->type == AT_INDEX_ALLOCATION) &&
-			NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino) {
-		iput(ni->itype.index.bmp_ino);
-		ni->itype.index.bmp_ino = NULL;
-	}
 #ifdef NTFS_RW
 	if (NInoDirty(ni)) {
 		BOOL was_bad = (is_bad_inode(vi));
@@ -2236,15 +2215,22 @@ void ntfs_clear_big_inode(struct inode *
 
 	__ntfs_clear_inode(ni);
 
+	/* Release the base inode if we are holding it. */
 	if (NInoAttr(ni)) {
-		/* Release the base inode if we are holding it. */
-		if (ni->nr_extents == -1) {
-			iput(VFS_I(ni->ext.base_ntfs_ino));
+		ntfs_inode *base_inode = ni->ext.base_ntfs_ino;
+
+		if (S_ISDIR(VFS_I(base_inode)->i_mode)) {
+			spin_lock(&base_inode->itype.index.bmp_ino_lock);
+			base_inode->itype.index.bmp_ino = NULL;
+			iput(VFS_I(base_inode));
+			spin_unlock(&base_inode->itype.index.bmp_ino_lock);
+			ni->ext.base_ntfs_ino = NULL;
+		} else if (ni->nr_extents == -1) {
 			ni->nr_extents = 0;
 			ni->ext.base_ntfs_ino = NULL;
+			iput(VFS_I(base_inode));
 		}
 	}
-	return;
 }
 
 /**
--- 1.45/fs/ntfs/inode.h	2004-10-25 11:46:30 +02:00
+++ edited/fs/ntfs/inode.h	2005-02-13 17:32:58 +01:00
@@ -101,6 +101,7 @@ struct _ntfs_inode {
 		struct { /* It is a directory, $MFT, or an index inode. */
 			struct inode *bmp_ino;	/* Attribute inode for the
 						   index $BITMAP. */
+			spinlock_t bmp_ino_lock;
 			u32 block_size;		/* Size of an index block. */
 			u32 vcn_size;		/* Size of a vcn in this
 						   index. */
@@ -275,6 +276,9 @@ extern struct inode *ntfs_attr_iget(stru
 extern struct inode *ntfs_index_iget(struct inode *base_vi, ntfschar *name,
 		u32 name_len);
 
+extern struct inode *ntfs_grab_bmp_inode(struct inode *inode);
+extern void ntfs_ungrab_bmp_inode(struct inode *inode);
+
 extern struct inode *ntfs_alloc_big_inode(struct super_block *sb);
 extern void ntfs_destroy_big_inode(struct inode *inode);
 extern void ntfs_clear_big_inode(struct inode *vi);
@@ -295,8 +299,6 @@ extern ntfs_inode *ntfs_new_extent_inode
 extern void ntfs_clear_extent_inode(ntfs_inode *ni);
 
 extern int ntfs_read_inode_mount(struct inode *vi);
-
-extern void ntfs_put_inode(struct inode *vi);
 
 extern int ntfs_show_options(struct seq_file *sf, struct vfsmount *mnt);
 
--- 1.184/fs/ntfs/super.c	2005-01-05 03:48:14 +01:00
+++ edited/fs/ntfs/super.c	2005-02-13 17:15:47 +01:00
@@ -2186,9 +2186,6 @@ static int ntfs_statfs(struct super_bloc
 static struct super_operations ntfs_sops = {
 	.alloc_inode	= ntfs_alloc_big_inode,	  /* VFS: Allocate new inode. */
 	.destroy_inode	= ntfs_destroy_big_inode, /* VFS: Deallocate inode. */
-	.put_inode	= ntfs_put_inode,	  /* VFS: Called just before
-						     the inode reference count
-						     is decreased. */
 #ifdef NTFS_RW
 	//.dirty_inode	= NULL,			/* VFS: Called from
 	//					   __mark_inode_dirty(). */