[PATCH] udf deadlock fix

Yes, me too.  generic_shutdown_super() takes lock_super().  And udf uses
lock_super for protecting its block allocation data strutures.  Trivial
deadlock on unmount.

Below is a fix to switch udf to it's own private locking.  It's safe
because it doesn't intefere with VFS lock_super usage anywhere.

udf_free_inode has some more updates than simply switching the used
lock:

 - clear_inode() call moved outside locked section to avoid another
   deadlock
 - unused variable ino killed
 - is_directory moved into the conditional it's actually used in
Signed-off-by: Christoph Hellwig <hch@lst.de>

(note that I see memory corruption in UDF_I_DATA(inode), but I've
reproduced that with a kernel without all recent udf changes.  I'll
debug that one further)
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

fs/udf/balloc.c

@@ -148,6 +148,7 @@ static void udf_bitmap_free_blocks(struct super_block * sb,
 	struct udf_bitmap *bitmap,
 	kernel_lb_addr bloc, uint32_t offset, uint32_t count)
 {
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	struct buffer_head * bh = NULL;
 	unsigned long block;
 	unsigned long block_group;
@@ -156,7 +157,7 @@ static void udf_bitmap_free_blocks(struct super_block * sb,
 	int bitmap_nr;
 	unsigned long overflow;
 
-	lock_super(sb);
+	down(&sbi->s_alloc_sem);
 	if (bloc.logicalBlockNum < 0 ||
 		(bloc.logicalBlockNum + count) > UDF_SB_PARTLEN(sb, bloc.partitionReferenceNum))
 	{
@@ -215,7 +216,7 @@ static void udf_bitmap_free_blocks(struct super_block * sb,
 	sb->s_dirt = 1;
 	if (UDF_SB_LVIDBH(sb))
 		mark_buffer_dirty(UDF_SB_LVIDBH(sb));
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 	return;
 }
 
@@ -224,13 +225,13 @@ static int udf_bitmap_prealloc_blocks(struct super_block * sb,
 	struct udf_bitmap *bitmap, uint16_t partition, uint32_t first_block,
 	uint32_t block_count)
 {
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	int alloc_count = 0;
 	int bit, block, block_group, group_start;
 	int nr_groups, bitmap_nr;
 	struct buffer_head *bh;
 
-	lock_super(sb);
-
+	down(&sbi->s_alloc_sem);
 	if (first_block < 0 || first_block >= UDF_SB_PARTLEN(sb, partition))
 		goto out;
 
@@ -279,7 +280,7 @@ static int udf_bitmap_prealloc_blocks(struct super_block * sb,
 		mark_buffer_dirty(UDF_SB_LVIDBH(sb));
 	}
 	sb->s_dirt = 1;
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 	return alloc_count;
 }
 
@@ -287,6 +288,7 @@ static int udf_bitmap_new_block(struct super_block * sb,
 	struct inode * inode,
 	struct udf_bitmap *bitmap, uint16_t partition, uint32_t goal, int *err)
 {
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	int newbit, bit=0, block, block_group, group_start;
 	int end_goal, nr_groups, bitmap_nr, i;
 	struct buffer_head *bh = NULL;
@@ -294,7 +296,7 @@ static int udf_bitmap_new_block(struct super_block * sb,
 	int newblock = 0;
 
 	*err = -ENOSPC;
-	lock_super(sb);
+	down(&sbi->s_alloc_sem);
 
 repeat:
 	if (goal < 0 || goal >= UDF_SB_PARTLEN(sb, partition))
@@ -367,7 +369,7 @@ static int udf_bitmap_new_block(struct super_block * sb,
 	}
 	if (i >= (nr_groups*2))
 	{
-		unlock_super(sb);
+		up(&sbi->s_alloc_sem);
 		return newblock;
 	}
 	if (bit < sb->s_blocksize << 3)
@@ -376,7 +378,7 @@ static int udf_bitmap_new_block(struct super_block * sb,
 		bit = udf_find_next_one_bit(bh->b_data, sb->s_blocksize << 3, group_start << 3);
 	if (bit >= sb->s_blocksize << 3)
 	{
-		unlock_super(sb);
+		up(&sbi->s_alloc_sem);
 		return 0;
 	}
 
@@ -390,7 +392,7 @@ static int udf_bitmap_new_block(struct super_block * sb,
 	 */
 	if (inode && DQUOT_ALLOC_BLOCK(inode, 1))
 	{
-		unlock_super(sb);
+		up(&sbi->s_alloc_sem);
 		*err = -EDQUOT;
 		return 0;
 	}
@@ -413,13 +415,13 @@ static int udf_bitmap_new_block(struct super_block * sb,
 		mark_buffer_dirty(UDF_SB_LVIDBH(sb));
 	}
 	sb->s_dirt = 1;
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 	*err = 0;
 	return newblock;
 
 error_return:
 	*err = -EIO;
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 	return 0;
 }
 
@@ -428,6 +430,7 @@ static void udf_table_free_blocks(struct super_block * sb,
 	struct inode * table,
 	kernel_lb_addr bloc, uint32_t offset, uint32_t count)
 {
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	uint32_t start, end;
 	uint32_t nextoffset, oextoffset, elen;
 	kernel_lb_addr nbloc, obloc, eloc;
@@ -435,7 +438,7 @@ static void udf_table_free_blocks(struct super_block * sb,
 	int8_t etype;
 	int i;
 
-	lock_super(sb);
+	down(&sbi->s_alloc_sem);
 	if (bloc.logicalBlockNum < 0 ||
 		(bloc.logicalBlockNum + count) > UDF_SB_PARTLEN(sb, bloc.partitionReferenceNum))
 	{
@@ -669,7 +672,7 @@ static void udf_table_free_blocks(struct super_block * sb,
 
 error_return:
 	sb->s_dirt = 1;
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 	return;
 }
 
@@ -678,6 +681,7 @@ static int udf_table_prealloc_blocks(struct super_block * sb,
 	struct inode *table, uint16_t partition, uint32_t first_block,
 	uint32_t block_count)
 {
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	int alloc_count = 0;
 	uint32_t extoffset, elen, adsize;
 	kernel_lb_addr bloc, eloc;
@@ -694,8 +698,7 @@ static int udf_table_prealloc_blocks(struct super_block * sb,
 	else
 		return 0;
 
-	lock_super(sb);
-
+	down(&sbi->s_alloc_sem);
 	extoffset = sizeof(struct unallocSpaceEntry);
 	bloc = UDF_I_LOCATION(table);
 
@@ -739,7 +742,7 @@ static int udf_table_prealloc_blocks(struct super_block * sb,
 		mark_buffer_dirty(UDF_SB_LVIDBH(sb));
 		sb->s_dirt = 1;
 	}
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 	return alloc_count;
 }
 
@@ -747,6 +750,7 @@ static int udf_table_new_block(struct super_block * sb,
 	struct inode * inode,
 	struct inode *table, uint16_t partition, uint32_t goal, int *err)
 {
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	uint32_t spread = 0xFFFFFFFF, nspread = 0xFFFFFFFF;
 	uint32_t newblock = 0, adsize;
 	uint32_t extoffset, goal_extoffset, elen, goal_elen = 0;
@@ -763,8 +767,7 @@ static int udf_table_new_block(struct super_block * sb,
 	else
 		return newblock;
 
-	lock_super(sb);
-
+	down(&sbi->s_alloc_sem);
 	if (goal < 0 || goal >= UDF_SB_PARTLEN(sb, partition))
 		goal = 0;
 
@@ -814,7 +817,7 @@ static int udf_table_new_block(struct super_block * sb,
 	if (spread == 0xFFFFFFFF)
 	{
 		udf_release_data(goal_bh);
-		unlock_super(sb);
+		up(&sbi->s_alloc_sem);
 		return 0;
 	}
 
@@ -830,7 +833,7 @@ static int udf_table_new_block(struct super_block * sb,
 	if (inode && DQUOT_ALLOC_BLOCK(inode, 1))
 	{
 		udf_release_data(goal_bh);
-		unlock_super(sb);
+		up(&sbi->s_alloc_sem);
 		*err = -EDQUOT;
 		return 0;
 	}
@@ -849,7 +852,7 @@ static int udf_table_new_block(struct super_block * sb,
 	}
 
 	sb->s_dirt = 1;
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 	*err = 0;
 	return newblock;
 }

fs/udf/ialloc.c

@@ -35,11 +35,8 @@
 
 void udf_free_inode(struct inode * inode)
 {
-	struct super_block * sb = inode->i_sb;
-	int is_directory;
-	unsigned long ino;
-
-	ino = inode->i_ino;
+	struct super_block *sb = inode->i_sb;
+	struct udf_sb_info *sbi = UDF_SB(sb);
 
 	/*
 	 * Note: we must free any quota before locking the superblock,
@@ -48,36 +45,32 @@ void udf_free_inode(struct inode * inode)
 	DQUOT_FREE_INODE(inode);
 	DQUOT_DROP(inode);
 
-	lock_super(sb);
-
-	is_directory = S_ISDIR(inode->i_mode);
-
 	clear_inode(inode);
 
-	if (UDF_SB_LVIDBH(sb))
-	{
-		if (is_directory)
+	down(&sbi->s_alloc_sem);
+	if (sbi->s_lvidbh) {
+		if (S_ISDIR(inode->i_mode))
 			UDF_SB_LVIDIU(sb)->numDirs =
 				cpu_to_le32(le32_to_cpu(UDF_SB_LVIDIU(sb)->numDirs) - 1);
 		else
 			UDF_SB_LVIDIU(sb)->numFiles =
 				cpu_to_le32(le32_to_cpu(UDF_SB_LVIDIU(sb)->numFiles) - 1);
 		
-		mark_buffer_dirty(UDF_SB_LVIDBH(sb));
+		mark_buffer_dirty(sbi->s_lvidbh);
 	}
-	unlock_super(sb);
+	up(&sbi->s_alloc_sem);
 
 	udf_free_blocks(sb, NULL, UDF_I_LOCATION(inode), 0, 1);
 }
 
 struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 {
-	struct super_block *sb;
+	struct super_block *sb = dir->i_sb;
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	struct inode * inode;
 	int block;
 	uint32_t start = UDF_I_LOCATION(dir).logicalBlockNum;
 
-	sb = dir->i_sb;
 	inode = new_inode(sb);
 
 	if (!inode)
@@ -94,8 +87,8 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 		iput(inode);
 		return NULL;
 	}
-	lock_super(sb);
 
+	down(&sbi->s_alloc_sem);
 	UDF_I_UNIQUE(inode) = 0;
 	UDF_I_LENEXTENTS(inode) = 0;
 	UDF_I_NEXT_ALLOC_BLOCK(inode) = 0;
@@ -160,8 +153,8 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 		UDF_I_CRTIME(inode) = current_fs_time(inode->i_sb);
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
+	up(&sbi->s_alloc_sem);
 
-	unlock_super(sb);
 	if (DQUOT_ALLOC_INODE(inode))
 	{
 		DQUOT_DROP(inode);

fs/udf/super.c

@@ -1504,6 +1504,8 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 	sb->s_fs_info = sbi;
 	memset(UDF_SB(sb), 0x00, sizeof(struct udf_sb_info));
 
+	init_MUTEX(&sbi->s_alloc_sem);
+
 	if (!udf_parse_options((char *)options, &uopt))
 		goto error_out;
 

include/linux/udf_fs_sb.h

@@ -18,6 +18,8 @@
 #ifndef _UDF_FS_SB_H
 #define _UDF_FS_SB_H 1
 
+#include <asm/semaphore.h>
+
 #pragma pack(1)
 
 #define UDF_MAX_BLOCK_LOADED	8
@@ -113,6 +115,8 @@ struct udf_sb_info
 
 	/* VAT inode */
 	struct inode		*s_vat;
+
+	struct semaphore	s_alloc_sem;
 };
 
 #endif /* _UDF_FS_SB_H */