Re: [PATCH] Fix a race condition between ->i_mapping and iput()

[OGAWA Hirofumi]

Andrew Morton <akpm@xxxxxxxx> writes:

>>  void bd_forget(struct inode *inode)
>>  {
>> +	struct block_device *old = NULL;
>> +
>>  	spin_lock(&bdev_lock);
>> -	if (inode->i_bdev)
>> +	if (inode->i_bdev) {
>> +		if (inode->i_sb != blockdev_superblock)
>> +			old = inode->i_bdev;
>>  		__bd_forget(inode);
>> +	}
>>  	spin_unlock(&bdev_lock);
>> +
>> +	if (old)
>> +		iput(old->bd_inode);
>>  }
>
> We're missing an atomic_inc(i_count) here?

I think we don't need an atomic_inc(i_count) here. The inode of
argument has already I_FREEING, so we just call iput(bdev's inode).

But, sorry. My patch seems broken.

With that simple rule, the code became more simple. And comment was
added.

I'll test a following patch today.
-- 
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>

diff -puN fs/block_dev.c~i_mapping-race-fix-2 fs/block_dev.c
--- linux-2.6/fs/block_dev.c~i_mapping-race-fix-2	2006-03-10 22:54:09.000000000 +0900
+++ linux-2.6-hirofumi/fs/block_dev.c	2006-03-10 23:05:03.000000000 +0900
@@ -432,21 +432,32 @@ EXPORT_SYMBOL(bdput);
 static struct block_device *bd_acquire(struct inode *inode)
 {
 	struct block_device *bdev;
+
+	BUG_ON(inode->i_sb == blockdev_superblock);
 	spin_lock(&bdev_lock);
 	bdev = inode->i_bdev;
-	if (bdev && igrab(bdev->bd_inode)) {
+	if (bdev) {
+		atomic_inc(&bdev->bd_inode->i_count);
 		spin_unlock(&bdev_lock);
 		return bdev;
 	}
 	spin_unlock(&bdev_lock);
+
 	bdev = bdget(inode->i_rdev);
 	if (bdev) {
 		spin_lock(&bdev_lock);
-		if (inode->i_bdev)
-			__bd_forget(inode);
-		inode->i_bdev = bdev;
-		inode->i_mapping = bdev->bd_inode->i_mapping;
-		list_add(&inode->i_devices, &bdev->bd_inodes);
+		if (!inode->i_bdev) {
+			/*
+			 * We take an additional bd_inode->i_count for inode,
+			 * and it's released in clear_inode() of inode.
+			 * So, we can access it via ->i_mapping always
+			 * without igrab().
+			 */
+			atomic_inc(&bdev->bd_inode->i_count);
+			inode->i_bdev = bdev;
+			inode->i_mapping = bdev->bd_inode->i_mapping;
+			list_add(&inode->i_devices, &bdev->bd_inodes);
+		}
 		spin_unlock(&bdev_lock);
 	}
 	return bdev;
@@ -456,10 +467,18 @@ static struct block_device *bd_acquire(s
 
 void bd_forget(struct inode *inode)
 {
+	struct block_device *old = NULL;
+
 	spin_lock(&bdev_lock);
-	if (inode->i_bdev)
+	if (inode->i_bdev) {
+		if (inode->i_sb != blockdev_superblock)
+			old = inode->i_bdev;
 		__bd_forget(inode);
+	}
 	spin_unlock(&bdev_lock);
+
+	if (old)
+		iput(old->bd_inode);
 }
 
 int bd_claim(struct block_device *bdev, void *holder)
_
-
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/