JFFS2 list_dirty corruption

While hacking on JFFS2 for NAND I found a possibility, where scan_medium corrupts list_dirty.

jffs2_scan_medium calls
jffs2_scan_eraseblock calls
jffs2_scan_dirent_node calls
jffs2_add_fd_to_list
There is a duplicate entry detected, so it calls
jffs2_mark_node_obsolete
There is it possible that the jeb is added to list_dirty, because it's a duplicate entry
with list_add_tail
We come back to
jffs2_scan_eraseblock
There is another condition, that marks the block dirty We come back to
jffs2_scan_medium
There we add the block to list_dirty too with add_list Then we have a circular list entry.
Result: chaos

That's not a problem of the NAND modifications. I verified, that this can happen in the actual CVS-version too.

Thomas
__________________________________________________
Thomas Gleixner, autronix automation GmbH
auf dem berg 3, d-88690 uhldingen-muehlhofen
fon: +49 7556 919891 , fax: +49 7556 919886
mail: [email protected], http://www.autronix.de  

This patch resolves the problem:

@@ -128,9 +128,15 @@
                                 (!c->nextblock || c->nextblock->free_size < 

jeb->free_size)) {
                                 /* Better candidate for the next writes to 
go to */
                                 if (c->nextblock)
+					/* We must delete, because mark_node_obsolete
+					   could have added this block to dirty_list already */
+                                        list_del(&c->nextblock->list);
                                         list_add(&c->nextblock->list, 
&c->dirty_list);
                                 c->nextblock = jeb;
                         } else {
+				/* We must delete, because mark_node_obsolete
+				   could have added this block to dirty_list already */
+				list_del(&jeb->list); 
                                 list_add(&jeb->list, &c->dirty_list);
                         }

} else {

I put it into CVS too

Thomas
__________________________________________________
Thomas Gleixner, autronix automation GmbH
auf dem berg 3, d-88690 uhldingen-muehlhofen
fon: +49 7556 919891 , fax: +49 7556 919886
mail: [email protected], http://www.autronix.de  

Ummm. Applying that patch gives me a seg fault on boot.

--Adam

Adam Wozniak (KG6GZR)   COM DEV Wireless - Digital and Software Systems
[email protected]      805 Aerovista Place, San Luis Obispo, CA 93401
                        http://www.comdev.cc
                        Voice: (805) 544-1089       Fax: (805) 544-2055

Ok, maybe this is a problem:

UMMM open { here?

Adam Wozniak (KG6GZR)   COM DEV Wireless - Digital and Software Systems
[email protected]      805 Aerovista Place, San Luis Obispo, CA 93401
                        http://www.comdev.cc
                        Voice: (805) 544-1089       Fax: (805) 544-2055

Sorry, you're right. I should send such mails better in the morning. It happened on the transcript from my NAND modified stuff. Here is the correct patch:

@@ -127,10 +127,17 @@
                         if (jeb->free_size > 2*sizeof(struct 

jffs2_raw_inode) &&

                                 (!c->nextblock || c->nextblock->free_size < 
jeb->free_size)) {
                                 /* Better candidate for the next writes to 

go to */
- if (c->nextblock)
+ if (c->nextblock) {
+ /* We must delete, because mark_node_obsolete
+ could have added this block to dirty_list already */

+                                        list_del(&c->nextblock->list);
                                         list_add(&c->nextblock->list, 
&c->dirty_list);
+				}
                                 c->nextblock = jeb;
                         } else {
+				/* We must delete, because mark_node_obsolete
+				   could have added this block to dirty_list already */
+				list_del(&jeb->list);
                                 list_add(&jeb->list, &c->dirty_list);
                         }

} else {

Thomas
P.S. David: The jffs2-nand-version is correct.
__________________________________________________
Thomas Gleixner, autronix automation GmbH
auf dem berg 3, d-88690 uhldingen-muehlhofen
fon: +49 7556 919891 , fax: +49 7556 919886
mail: [email protected], http://www.autronix.de  

That's OK, but I think it's cleaner if we prevent the mark_node_obsolete() code from touching the lists in the first place. This way, we can also prevent it from trying to physically mark them obsolete too - if that didn't happen the first time, we shouldn't be doing it on mount.

This problem didn't turn up on NOR flash because we do mark the nodes obsolete on the flash, so the mount code never had to bother with them.

Please could you confirm this also fixes the problem?

Index: include/linux/jffs2_fs_sb.h
===================================================================
RCS file: /home/cvs/mtd/include/linux/jffs2_fs_sb.h,v retrieving revision 1.18

diff -u -p -r1.18 jffs2_fs_sb.h
@@ -12,6 +12,7 @@
 #define INOCACHE_HASHSIZE 1
 
 #define JFFS2_SB_FLAG_RO 1
+#define JFFS2_SB_FLAG_MOUNTING 2
 
 /* A struct for the overall file system control.  Pointers to
    jffs2_sb_info structs are named `c' in the source code.  

Index: fs/jffs2/build.c
===================================================================
RCS file: /home/cvs/mtd/fs/jffs2/build.c,v
retrieving revision 1.22

diff -u -p -r1.22 build.c
@@ -293,6 +293,7 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
 	INIT_LIST_HEAD(&c->bad_list);
 	INIT_LIST_HEAD(&c->bad_used_list);
 	c->highest_ino = 1;
+	c->flags |= JFFS2_SB_FLAG_MOUNTING;
 
 	if (jffs2_build_filesystem(c)) {
 		D1(printk(KERN_DEBUG "build_fs failed\n"));

@@ -301,5 +302,8 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
 		kfree(c->blocks);
 		return -EIO;
 	}
+	
+	c->flags &= ~JFFS2_SB_FLAG_MOUNTING;
+
 	return 0;
 }

Index: fs/jffs2/nodemgmt.c
===================================================================
RCS file: /home/cvs/mtd/fs/jffs2/nodemgmt.c,v
retrieving revision 1.52

diff -u -p -r1.52 nodemgmt.c
@@ -318,6 +318,15 @@ void jffs2_mark_node_obsolete(struct jff
 
 	ACCT_PARANOIA_CHECK(jeb);
 
+	if (c->flags & JFFS2_SB_FLAG_MOUNTING) {
+		/* Mount in progress. Don't muck about with the block
+		   lists because they're not ready yet, and don't actually
+		   obliterate nodes that look obsolete. If they weren't 
+		   marked obsolete on the flash at the time they _became_
+		   obsolete, there was probably a reason for that. */
+		spin_unlock_bh(&c->erase_completion_lock);
+		return;
+	}
 	if (jeb == c->nextblock) {
 		D2(printk(KERN_DEBUG "Not moving nextblock 0x%08x to dirty/erase_pending list\n", jeb->offset));
 	} else if (jeb == c->gcblock) {

--
dwmw2

Yep, it does.

BTW: David, did you get my mail tonight? I had some strange notifications from my mailsystem.

Thomas
__________________________________________________
Thomas Gleixner, autronix automation GmbH
auf dem berg 3, d-88690 uhldingen-muehlhofen
fon: +49 7556 919891 , fax: +49 7556 919886
mail: [email protected], http://www.autronix.de  

I got lots :) Going through it in order.

--
dwmw2