Fix race condition in AFFS

PATCH In review Vladimir 'φ-coder/phcoder' Serbinenko 2 participants 4 messages May 8, 2012 – May 12, 2012 VFS

Share & tools

Vladimir 'φ-coder/phcoder' Serbinenko

author

2012-05-08 21:28 UTC #0 lore raw

PATCH 1/1 Fix race condition in AFFS

AFFS preallocates few blocks per inode as an optimisation. Unfortunately there is a race condition and the same blocks ends up being allocated for both data and metadata, metadata gets overwritten and so the file is partially unreadable. Here is a fix.
Please indicate if this isn't the right place to submit this or my previous fs-related patches.

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko

=== modified file 'affs.h'

affs.h +3 -0

@@ -66,8 +66,11 @@ struct affs_inode_info {
 	struct buffer_head *i_ext_bh;		/* bh of last extended block */
 	loff_t	 mmu_private;
 	u32	 i_protect;			/* unused attribute bits */
+
 	u32	 i_lastalloc;			/* last allocated block */
 	int	 i_pa_cnt;			/* number of preallocated blocks */
+	struct mutex i_alloc;		        /* Protects last 2 fields. */
+
 	struct inode vfs_inode;
 };

=== modified file 'bitmap.c'

bitmap.c +22 ~~-10~~

@@ -151,12 +151,18 @@ affs_alloc_block(struct inode *inode, u3
 
 	pr_debug("AFFS: balloc(inode=%lu,goal=%u): ", inode->i_ino, goal);
 
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+
 	if (AFFS_I(inode)->i_pa_cnt) {
-		pr_debug("%d\n", AFFS_I(inode)->i_lastalloc+1);
+		u32 ret;
 		AFFS_I(inode)->i_pa_cnt--;
-		return ++AFFS_I(inode)->i_lastalloc;
+		ret = ++AFFS_I(inode)->i_lastalloc;
+		mutex_unlock(&AFFS_I (inode)->i_alloc);
+		return ret;
 	}
 
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
+
 	if (!goal || goal > sbi->s_partition_size) {
 		if (goal)
 			affs_warning(sb, "affs_balloc", "invalid goal %d", goal);

@@ -230,16 +236,22 @@ find_bit:
 	bit = ffs(tmp & mask) - 1;
 	blk += bit + sbi->s_reserved;
 	mask2 = mask = 1 << (bit & 31);
-	AFFS_I(inode)->i_lastalloc = blk;
 
-	/* prealloc as much as possible within this word */
-	while ((mask2 <<= 1)) {
-		if (!(tmp & mask2))
-			break;
-		AFFS_I(inode)->i_pa_cnt++;
-		mask |= mask2;
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+	if (!AFFS_I(inode)->i_pa_cnt) {
+		AFFS_I(inode)->i_lastalloc = blk;
+
+		/* prealloc as much as possible within this word */
+		while ((mask2 <<= 1)) {
+			if (!(tmp & mask2))
+				break;
+			AFFS_I(inode)->i_pa_cnt++;
+			mask |= mask2;
+		}
+		bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
 	}
-	bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
+
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
 
 	*data = cpu_to_be32(tmp & ~mask);

=== modified file 'file.c'

file.c +12 -3

@@ -795,12 +795,21 @@ void
 affs_free_prealloc(struct inode *inode)
 {
 	struct super_block *sb = inode->i_sb;
+	u32 first, cnt;
 
 	pr_debug("AFFS: free_prealloc(ino=%lu)\n", inode->i_ino);
 
-	while (AFFS_I(inode)->i_pa_cnt) {
-		AFFS_I(inode)->i_pa_cnt--;
-		affs_free_block(sb, ++AFFS_I(inode)->i_lastalloc);
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+	first = AFFS_I(inode)->i_lastalloc;
+	cnt = AFFS_I(inode)->i_pa_cnt;
+	AFFS_I(inode)->i_lastalloc += cnt;
+	AFFS_I(inode)->i_pa_cnt = 0;
+
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
+
+	while (cnt) {
+		cnt--;
+		affs_free_block(sb, ++first);
 	}
 }

=== modified file 'inode.c'

inode.c +2 -0

@@ -70,6 +70,7 @@ struct inode *affs_iget(struct super_blo
 	AFFS_I(inode)->mmu_private = 0;
 	AFFS_I(inode)->i_lastalloc = 0;
 	AFFS_I(inode)->i_pa_cnt = 0;
+	mutex_init(&AFFS_I (inode)->i_alloc);
 
 	if (sbi->s_flags & SF_SETMODE)
 		inode->i_mode = sbi->s_mode;

@@ -326,6 +327,7 @@ affs_new_inode(struct inode *dir)
 	AFFS_I(inode)->i_pa_cnt = 0;
 	AFFS_I(inode)->i_extcnt = 1;
 	AFFS_I(inode)->i_ext_last = ~1;
+	mutex_init(&AFFS_I (inode)->i_alloc);
 
 	insert_inode_hash(inode);

Vladimir 'φ-coder/phcoder' Serbinenko

author

2012-05-09 22:02 UTC #1 lore raw

PATCH 1/1 Fix race condition in AFFS

Since it was ignored on LKML, I guess this is the right list.

-------- Original Message --------
Subject: [PATCH] Fix race condition in AFFS
Date: Tue, 08 May 2012 23:28:33 +0200
From: Vladimir 'φ-coder/phcoder' Serbinenko <[email protected]>
To: [email protected]

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko

=== modified file 'affs.h'

affs.h +3 -0

@@ -66,8 +66,11 @@ struct affs_inode_info {
 	struct buffer_head *i_ext_bh;		/* bh of last extended block */
 	loff_t	 mmu_private;
 	u32	 i_protect;			/* unused attribute bits */
+
 	u32	 i_lastalloc;			/* last allocated block */
 	int	 i_pa_cnt;			/* number of preallocated blocks */
+	struct mutex i_alloc;		        /* Protects last 2 fields. */
+
 	struct inode vfs_inode;
 };

=== modified file 'bitmap.c'

bitmap.c +22 ~~-10~~

@@ -151,12 +151,18 @@ affs_alloc_block(struct inode *inode, u3
 
 	pr_debug("AFFS: balloc(inode=%lu,goal=%u): ", inode->i_ino, goal);
 
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+
 	if (AFFS_I(inode)->i_pa_cnt) {
-		pr_debug("%d\n", AFFS_I(inode)->i_lastalloc+1);
+		u32 ret;
 		AFFS_I(inode)->i_pa_cnt--;
-		return ++AFFS_I(inode)->i_lastalloc;
+		ret = ++AFFS_I(inode)->i_lastalloc;
+		mutex_unlock(&AFFS_I (inode)->i_alloc);
+		return ret;
 	}
 
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
+
 	if (!goal || goal > sbi->s_partition_size) {
 		if (goal)
 			affs_warning(sb, "affs_balloc", "invalid goal %d", goal);

@@ -230,16 +236,22 @@ find_bit:
 	bit = ffs(tmp & mask) - 1;
 	blk += bit + sbi->s_reserved;
 	mask2 = mask = 1 << (bit & 31);
-	AFFS_I(inode)->i_lastalloc = blk;
 
-	/* prealloc as much as possible within this word */
-	while ((mask2 <<= 1)) {
-		if (!(tmp & mask2))
-			break;
-		AFFS_I(inode)->i_pa_cnt++;
-		mask |= mask2;
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+	if (!AFFS_I(inode)->i_pa_cnt) {
+		AFFS_I(inode)->i_lastalloc = blk;
+
+		/* prealloc as much as possible within this word */
+		while ((mask2 <<= 1)) {
+			if (!(tmp & mask2))
+				break;
+			AFFS_I(inode)->i_pa_cnt++;
+			mask |= mask2;
+		}
+		bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
 	}
-	bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
+
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
 
 	*data = cpu_to_be32(tmp & ~mask);

=== modified file 'file.c'

file.c +12 -3

@@ -795,12 +795,21 @@ void
 affs_free_prealloc(struct inode *inode)
 {
 	struct super_block *sb = inode->i_sb;
+	u32 first, cnt;
 
 	pr_debug("AFFS: free_prealloc(ino=%lu)\n", inode->i_ino);
 
-	while (AFFS_I(inode)->i_pa_cnt) {
-		AFFS_I(inode)->i_pa_cnt--;
-		affs_free_block(sb, ++AFFS_I(inode)->i_lastalloc);
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+	first = AFFS_I(inode)->i_lastalloc;
+	cnt = AFFS_I(inode)->i_pa_cnt;
+	AFFS_I(inode)->i_lastalloc += cnt;
+	AFFS_I(inode)->i_pa_cnt = 0;
+
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
+
+	while (cnt) {
+		cnt--;
+		affs_free_block(sb, ++first);
 	}
 }

=== modified file 'inode.c'

inode.c +2 -0

@@ -70,6 +70,7 @@ struct inode *affs_iget(struct super_blo
 	AFFS_I(inode)->mmu_private = 0;
 	AFFS_I(inode)->i_lastalloc = 0;
 	AFFS_I(inode)->i_pa_cnt = 0;
+	mutex_init(&AFFS_I (inode)->i_alloc);
 
 	if (sbi->s_flags & SF_SETMODE)
 		inode->i_mode = sbi->s_mode;

@@ -326,6 +327,7 @@ affs_new_inode(struct inode *dir)
 	AFFS_I(inode)->i_pa_cnt = 0;
 	AFFS_I(inode)->i_extcnt = 1;
 	AFFS_I(inode)->i_ext_last = ~1;
+	mutex_init(&AFFS_I (inode)->i_alloc);
 
 	insert_inode_hash(inode);

Jan Kara

reviewer

2012-05-10 14:23 UTC #2 lore raw

In reply to Vladimir 'φ-coder/phcoder' Serbinenko

Hello,

Quoted 6 lines · Vladimir 'φ-coder/phcoder' Serbinenko

AFFS preallocates few blocks per inode as an optimisation. Unfortunately there is a race condition and the same blocks ends up being allocated for both data and metadata, metadata gets overwritten and so the file is partially unreadable. Here is a fix.
Please indicate if this isn't the right place to submit this or my previous fs-related patches.

Thanks for sending this fix! A couple of notes: It would be better to CC
[email protected] with this patch as well. It is indicated in MAINTAINERS file as a list to be used for this filesystem and it has much lower traffic than LKML so there's lower chance the patch just gets lost. Also please read Documentation/SubmittingPatches about how your patch should look like. In particular I'm missing your Signed-off-by line and the patch doesn't contain full paths to modified files.

After these formalities lets get to the real code :):

Quoted 4 lines

=== modified file 'affs.h'

affs.h struct affs_inode_info { · line 75

Quoted 10 lines

affs.h +3 -0

@@ -66,8 +66,11 @@ struct affs_inode_info {
 	struct buffer_head *i_ext_bh;		/* bh of last extended block */
 	loff_t	 mmu_private;
 	u32	 i_protect;			/* unused attribute bits */
+
 	u32	 i_lastalloc;			/* last allocated block */
 	int	 i_pa_cnt;			/* number of preallocated blocks */
+	struct mutex i_alloc;		        /* Protects last 2 fields. */
+
 	struct inode vfs_inode;
 };

Looking into the code below, it seems you don't have to sleep inside the
i_alloc lock. So using spinlock is better for performance in that case.

Quoted 97 lines

=== modified file 'bitmap.c'

bitmap.c +22 ~~-10~~

@@ -151,12 +151,18 @@ affs_alloc_block(struct inode *inode, u3
 
 	pr_debug("AFFS: balloc(inode=%lu,goal=%u): ", inode->i_ino, goal);
 
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+
 	if (AFFS_I(inode)->i_pa_cnt) {
-		pr_debug("%d\n", AFFS_I(inode)->i_lastalloc+1);
+		u32 ret;
 		AFFS_I(inode)->i_pa_cnt--;
-		return ++AFFS_I(inode)->i_lastalloc;
+		ret = ++AFFS_I(inode)->i_lastalloc;
+		mutex_unlock(&AFFS_I (inode)->i_alloc);
+		return ret;
 	}
 
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
+
 	if (!goal || goal > sbi->s_partition_size) {
 		if (goal)
 			affs_warning(sb, "affs_balloc", "invalid goal %d", goal);

@@ -230,16 +236,22 @@ find_bit:
 	bit = ffs(tmp & mask) - 1;
 	blk += bit + sbi->s_reserved;
 	mask2 = mask = 1 << (bit & 31);
-	AFFS_I(inode)->i_lastalloc = blk;
 
-	/* prealloc as much as possible within this word */
-	while ((mask2 <<= 1)) {
-		if (!(tmp & mask2))
-			break;
-		AFFS_I(inode)->i_pa_cnt++;
-		mask |= mask2;
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+	if (!AFFS_I(inode)->i_pa_cnt) {
+		AFFS_I(inode)->i_lastalloc = blk;
+
+		/* prealloc as much as possible within this word */
+		while ((mask2 <<= 1)) {
+			if (!(tmp & mask2))
+				break;
+			AFFS_I(inode)->i_pa_cnt++;
+			mask |= mask2;
+		}
+		bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
 	}
-	bm->bm_free -= AFFS_I(inode)->i_pa_cnt + 1;
+
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
 
 	*data = cpu_to_be32(tmp & ~mask);

=== modified file 'file.c'

file.c +12 -3

@@ -795,12 +795,21 @@ void
 affs_free_prealloc(struct inode *inode)
 {
 	struct super_block *sb = inode->i_sb;
+	u32 first, cnt;
 
 	pr_debug("AFFS: free_prealloc(ino=%lu)\n", inode->i_ino);
 
-	while (AFFS_I(inode)->i_pa_cnt) {
-		AFFS_I(inode)->i_pa_cnt--;
-		affs_free_block(sb, ++AFFS_I(inode)->i_lastalloc);
+	mutex_lock(&AFFS_I (inode)->i_alloc);
+	first = AFFS_I(inode)->i_lastalloc;
+	cnt = AFFS_I(inode)->i_pa_cnt;
+	AFFS_I(inode)->i_lastalloc += cnt;
+	AFFS_I(inode)->i_pa_cnt = 0;
+
+	mutex_unlock(&AFFS_I (inode)->i_alloc);
+
+	while (cnt) {
+		cnt--;
+		affs_free_block(sb, ++first);
 	}
 }

=== modified file 'inode.c'

inode.c +1 -0

@@ -70,6 +70,7 @@ struct inode *affs_iget(struct super_blo
 	AFFS_I(inode)->mmu_private = 0;
 	AFFS_I(inode)->i_lastalloc = 0;
 	AFFS_I(inode)->i_pa_cnt = 0;
+	mutex_init(&AFFS_I (inode)->i_alloc);
 
 	if (sbi->s_flags & SF_SETMODE)
 		inode->i_mode = sbi->s_mode;

inode.c affs_new_inode(struct inode *dir) · line 332

Quoted 6 lines

inode.c +1 -0

@@ -326,6 +327,7 @@ affs_new_inode(struct inode *dir)
 	AFFS_I(inode)->i_pa_cnt = 0;
 	AFFS_I(inode)->i_extcnt = 1;
 	AFFS_I(inode)->i_ext_last = ~1;
+	mutex_init(&AFFS_I (inode)->i_alloc);
 
 	insert_inode_hash(inode);

I think it's better to use affs_alloc_inode() to initialize the lock. You
won't have to initialize it at two places and it's more robust for future as well.

Otherwise your patch looks fine.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Vladimir 'φ-coder/phcoder' Serbinenko

author

2012-05-12 00:06 UTC #3 lore raw

Quoted 2 lines

Otherwise your patch looks fine.

Ok, I've fixed trhe problems you mentioned, now need to test it and so need to free few GiBs to compile Linux first. Wil send new version ASAP

Signature · 2 lines

Regards
Vladimir 'φ-coder/phcoder' Serbinenko