Patchwork jffs2: Fix lock acquisition order bug in jffs2_write_begin

login
register
mail settings
Submitter Joakim Tjernlund
Date Oct. 5, 2012, 9:24 a.m.
Message ID <1349429064-10241-1-git-send-email-Joakim.Tjernlund@transmode.se>
Download mbox | patch
Permalink /patch/189450/
State New
Headers show

Comments

Joakim Tjernlund - Oct. 5, 2012, 9:24 a.m.
Thomas.Betker@rohde-schwarz.com reports:

The target system is an SoC with a dual ARMv7 (Cortex-A9), and we are
running the long-term 3.4.11 kernel (whose fs/jffs2/ seems to be pretty
close to the latest mainline kernel). The deadlock occurred when using scp
to copy files from a host system to the target system.

The GC thread hangs in lock_page(page), the write thread hangs in the
first mutex_lock(&f->sem). The cause seems to be an AB-BA deadlock:

jffs2_garbage_collect_live
    mutex_lock(&f->sem)                         (A)
    jffs2_garbage_collect_dnode [inlined]
        jffs2_gc_fetch_page
            read_cache_page_async
                do_read_cache_page
                    lock_page(page) [inlined]
                        __lock_page             (B) ***

jffs2_write_begin
    grab_cache_page_write_begin
        find_lock_page
            lock_page(page)                     (B)
    mutex_lock(&f->sem)                         (A) ***

I have manually analyzed the stacks and confirmed that both threads sit on
the theme 'struct page'.

Fix this by restructuring jffs2_write_begin to take the f->sem lock
before the page lock. However one must make sure not to hold f->sem
when calling jffs2_reserve_space according to README.Locking

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---

 This needs to be review by someone that understands the
 looking rules better. David?

 fs/jffs2/file.c |   41 +++++++++++++++++++++--------------------
 1 files changed, 21 insertions(+), 20 deletions(-)
Thomas.Betker@rohde-schwarz.com - Oct. 8, 2012, 8:25 a.m.
Hello Joakim,

I am not sure if it is okay to call jffs2_complete_reservation() without a 
previous jffs2_reserve_space(), so I have avoided that in my own patch.

Also, I am not happy with using ret to check if 
jffs2_complete_reservation() was done; I am using alloc_len for that.

"jffs2_dbg(1, "%s()\n", __func__);" comes a bit late.

And "be be" in the first comment should probably read "be done".

I am attaching my own patch for comparison (sorry, I haven't found yet a 
way to send the patch inline); the tests are running this morning.

Best regards,
Thomas
Joakim Tjernlund - Oct. 8, 2012, 8:48 a.m.
>
> Hello Joakim,
>
> I am not sure if it is okay to call jffs2_complete_reservation() without a
> previous jffs2_reserve_space(), so I have avoided that in my own patch.
>
> Also, I am not happy with using ret to check if
> jffs2_complete_reservation() was done; I am using alloc_len for that.
>
> "jffs2_dbg(1, "%s()\n", __func__);" comes a bit late.
>
> And "be be" in the first comment should probably read "be done".

Agreed on all above points, thanks.

>
> I am attaching my own patch for comparison (sorry, I haven't found yet a
> way to send the patch inline); the tests are running this morning.

Right, I don't send inline paches with Notes, I use git send-email

 Jocke
Artem Bityutskiy - Oct. 15, 2012, 1:28 p.m.
On Mon, 2012-10-08 at 10:48 +0200, Joakim Tjernlund wrote:
> > And "be be" in the first comment should probably read "be done".
> 
> Agreed on all above points, thanks.

So would someone send the final patch with the commit message,
Signed-off-by, etc? Should there be Cc: stable@vger.kernel.org?
Joakim Tjernlund - Oct. 15, 2012, 1:38 p.m.
Artem Bityutskiy <dedekind1@gmail.com> wrote on 2012/10/15 15:28:43:
> 
> On Mon, 2012-10-08 at 10:48 +0200, Joakim Tjernlund wrote:
> > > And "be be" in the first comment should probably read "be done".
> > 
> > Agreed on all above points, thanks.
> 
> So would someone send the final patch with the commit message,
> Signed-off-by, etc? Should there be Cc: stable@vger.kernel.org?

Thomas already did, he fixed it up and sent it to the ML. I acked it.

 Jocke
Thomas.Betker@rohde-schwarz.com - Oct. 15, 2012, 1:38 p.m.
Hello Artem:

> So would someone send the final patch with the commit message,
> Signed-off-by, etc? Should there be Cc: stable@vger.kernel.org?

I sent a "PATCH RESEND" message to the list, with the same topic; please 
use that one (sorry if I caused a confusion).

Cc: stable@vger.kernel.org makes sense -- can you add it? Does stable 
cover 3.4 as well?

Best regards,
Thomas

Patch

diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index db3889b..23adcdc 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -140,31 +140,35 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 	struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
 	pgoff_t index = pos >> PAGE_CACHE_SHIFT;
 	uint32_t pageofs = index << PAGE_CACHE_SHIFT;
-	int ret = 0;
+	int ret = -ENOMEM;
+	struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
+	struct jffs2_raw_inode ri;
+	uint32_t alloc_len;
 
+	if (pageofs > inode->i_size) {
+		/* reserve space must be be before locking f->sem */
+		ret = jffs2_reserve_space(c, sizeof(ri), &alloc_len,
+					  ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
+		if (ret)
+			return ret;
+	}
+
+	mutex_lock(&f->sem);
 	pg = grab_cache_page_write_begin(mapping, index, flags);
 	if (!pg)
-		return -ENOMEM;
+		goto out_unlock;
+
 	*pagep = pg;
 
 	jffs2_dbg(1, "%s()\n", __func__);
 
-	if (pageofs > inode->i_size) {
+	if (!ret) { /* reserve_space succeded */
 		/* Make new hole frag from old EOF to new page */
-		struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
-		struct jffs2_raw_inode ri;
 		struct jffs2_full_dnode *fn;
-		uint32_t alloc_len;
 
 		jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current EOF and new page\n",
 			  (unsigned int)inode->i_size, pageofs);
 
-		ret = jffs2_reserve_space(c, sizeof(ri), &alloc_len,
-					  ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
-		if (ret)
-			goto out_page;
-
-		mutex_lock(&f->sem);
 		memset(&ri, 0, sizeof(ri));
 
 		ri.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
@@ -190,8 +194,6 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 
 		if (IS_ERR(fn)) {
 			ret = PTR_ERR(fn);
-			jffs2_complete_reservation(c);
-			mutex_unlock(&f->sem);
 			goto out_page;
 		}
 		ret = jffs2_add_full_dnode_to_inode(c, f, fn);
@@ -205,13 +207,10 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 				  ret);
 			jffs2_mark_node_obsolete(c, fn->raw);
 			jffs2_free_full_dnode(fn);
-			jffs2_complete_reservation(c);
-			mutex_unlock(&f->sem);
 			goto out_page;
 		}
 		jffs2_complete_reservation(c);
 		inode->i_size = pageofs;
-		mutex_unlock(&f->sem);
 	}
 
 	/*
@@ -220,18 +219,20 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 	 * case of a short-copy.
 	 */
 	if (!PageUptodate(pg)) {
-		mutex_lock(&f->sem);
 		ret = jffs2_do_readpage_nolock(inode, pg);
-		mutex_unlock(&f->sem);
 		if (ret)
 			goto out_page;
 	}
+	mutex_unlock(&f->sem);
 	jffs2_dbg(1, "end write_begin(). pg->flags %lx\n", pg->flags);
-	return ret;
+	return 0;
 
 out_page:
 	unlock_page(pg);
 	page_cache_release(pg);
+out_unlock:
+	jffs2_complete_reservation(c);
+	mutex_unlock(&f->sem);
 	return ret;
 }