Fwd: [PATCH] Fix race condition in AFFS

Fwd: [PATCH] Fix race condition in AFFS

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 712 bytes --]

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 <phcoder@gmail.com>
To: 	linux-kernel@vger.kernel.org



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.

-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: affs-race.diff --]
[-- Type: text/x-diff; name="affs-race.diff", Size: 3423 bytes --]

=== modified file 'affs.h'
--- affs.h	2012-05-07 16:42:19 +0000
+++ affs.h	2012-05-08 21:24:08 +0000
@@ -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	2012-05-06 21:40:49 +0000
+++ bitmap.c	2012-05-08 21:24:08 +0000
@@ -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	2012-05-06 21:40:49 +0000
+++ file.c	2012-05-08 21:24:08 +0000
@@ -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	2012-05-07 16:23:07 +0000
+++ inode.c	2012-05-08 21:24:08 +0000
@@ -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);
 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: [PATCH] Fix race condition in AFFS

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]


>  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


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]