[BUGFIX] JFFS2 NOR problem

[BUGFIX] JFFS2 NOR problem

[Artem B. Bityuckiy]

Hello,

I've detected major JFFS2 bug and have attached the fix. The bug seems
to be NOR-only. I believe everybody who uses JFFS2 on top of NOR flash
should have this bug fixed.

The brief problem description:

JFFS2 clears the JFFS2_SB_FLAG_MOUNTING (c-flags) superblock flag after
it has done the 1st filesystem build pass. But it still has non NULL
ic->scan_dents pointers which are actually assigned NULL at the very end
of the file system build procedure. Next FS build passes call the
jffs2_mark_node_obsolete() function which, in turn, utilizes the
jffs2_raw_ref_to_ic() function which *requires* ic->scan_dents = NULL.

This might cause infinite loops and oopses.

I think it is safe to clear the JFFS2_SB_FLAG_MOUNTING flag at the end
of JFFS2 filesystem build routine. Comments?

P.S. Possibly, the patch will fix Konstantin Kletschke's and Craig A.
Vanderborgh's problems. Guys, please try.



--- build.c     2005-02-26 22:08:17.503657749 +0300
+++ build.c_fixed       2005-02-26 22:07:55.806278357 +0300
@@ -116,7 +116,6 @@
                        cond_resched();
                }
        }
-       c->flags &= ~JFFS2_SB_FLAG_MOUNTING;

        D1(printk(KERN_DEBUG "Pass 1 complete\n"));

@@ -164,6 +163,8 @@
                ic->scan_dents = NULL;
                cond_resched();
        }
+       c->flags &= ~JFFS2_SB_FLAG_MOUNTING;
+
        D1(printk(KERN_DEBUG "Pass 3 complete\n"));
        D2(jffs2_dump_block_lists(c));


-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [BUGFIX] JFFS2 NOR problem

[Craig A. Vanderborgh]

Artem B. Bityuckiy wrote:

>Hello,
>
>I've detected major JFFS2 bug and have attached the fix. The bug seems
>to be NOR-only. I believe everybody who uses JFFS2 on top of NOR flash
>should have this bug fixed.
>
>The brief problem description:
>
>JFFS2 clears the JFFS2_SB_FLAG_MOUNTING (c-flags) superblock flag after
>it has done the 1st filesystem build pass. But it still has non NULL
>ic->scan_dents pointers which are actually assigned NULL at the very end
>of the file system build procedure. Next FS build passes call the
>jffs2_mark_node_obsolete() function which, in turn, utilizes the
>jffs2_raw_ref_to_ic() function which *requires* ic->scan_dents = NULL.
>
>This might cause infinite loops and oopses.
>
>I think it is safe to clear the JFFS2_SB_FLAG_MOUNTING flag at the end
>of JFFS2 filesystem build routine. Comments?
>
>P.S. Possibly, the patch will fix Konstantin Kletschke's and Craig A.
>Vanderborgh's problems. Guys, please try.
>
>  
>
>------------------------------------------------------------------------
>
>--- build.c	2005-02-26 22:08:17.503657749 +0300
>+++ build.c_fixed	2005-02-26 22:07:55.806278357 +0300
>@@ -116,7 +116,6 @@
> 			cond_resched();
> 		}
> 	}
>-	c->flags &= ~JFFS2_SB_FLAG_MOUNTING;
> 
> 	D1(printk(KERN_DEBUG "Pass 1 complete\n"));
> 
>@@ -164,6 +163,8 @@
> 		ic->scan_dents = NULL;
> 		cond_resched();
> 	}
>+	c->flags &= ~JFFS2_SB_FLAG_MOUNTING;
>+
> 	D1(printk(KERN_DEBUG "Pass 3 complete\n"));
> 	D2(jffs2_dump_block_lists(c));
> 
>  
>
Hello Artem:

Thank you for taking pity on us :^)

I have succeeded in resolving the oops that I reported in 2 different ways:

1. I applied your patch to the "vanilla" 2.6.10 sources.  To test, I 
compiled and
installed the modified kernel, and had it mount a large (~7MB) JFFS2 
root filesystem
that was "virginal".  This works.  Previously, it was this very same 
testing procedure
that produced a kernel oops "every time".

2. Before receiving your patch, I back-ported the 2.6.11-rc5 sources for 
fs/jffs2
(and the required related header files). I tested using the same 
procedure mentioned
above, and this also works.

Would you please comment on this:  which solution do you think is the 
better one?  Now that we have the newer MTD code working in the 
otherwise "vanilla" 2.6.10 kernel, is this the best solution?  Or is the 
build.c patch solution the safer bet for us?

Please advise.

Thanks a lot for looking into this, we REALLY appreciate it and it's 
great to be fully operational on kernel 2.6!!

Best regards,
craig vanderborgh
voxware incorporated
The 4th Reich

Re: [BUGFIX] JFFS2 NOR problem

[David Woodhouse]

On Sat, 2005-02-26 at 22:19 +0300, Artem B. Bityuckiy wrote:
>I think it is safe to clear the JFFS2_SB_FLAG_MOUNTING flag at the end
>of JFFS2 filesystem build routine. Comments?

Didn't we move it from there to where it is now quite recently for other
reasons?

-- 
dwmw2

Re: [BUGFIX] JFFS2 NOR problem

[Artem B. Bityuckiy]

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

Craig, 

I've found that the patch I sent yesterday has defect which might cause
errors or at least lead to an inefficient behavior.

The problem is that if we have the JFFS2_SB_FLAG_MOUNTING flag set
during the entire FS build process we risk to loose proper blocks
accounting. Namely, jffs2_mark_node_obsolete() function moves blocks
between different lists, but this peace of code is skipped owing to the
JFFS2_SB_FLAG_MOUNTING flag.

New patch (attached) must be better. Comments?

On Sat, 2005-02-26 at 22:43 -0500, Craig A. Vanderborgh wrote:
> I have succeeded in resolving the oops that I reported in 2 different ways:
Good, I hope Konstantin Kletschke's problem will go away as well.

> Would you please comment on this:  which solution do you think is the 
> better one?  Now that we have the newer MTD code working in the 
> otherwise "vanilla" 2.6.10 kernel, is this the best solution?  Or is the 
> build.c patch solution the safer bet for us?
Well, I can't advice ably but I'd rather use CVS head albeit it is a bit
more dangerous. And last but not least, I'd like to stress - CVS head,
not 2.6.11-rc5 as its MTD snapshot doesn't have to be latest. You might
download the latest MTD snapshot and install it to your kernel
(patchin.sh utility). Refer to linux-mtd.infradead.org for more info.


On Sun, 2005-02-27 at 09:40 +0000, David Woodhouse wrote: 
> Didn't we move it from there to where it is now quite recently for other
> reasons?
Yes, we did. First the flag was set only around scan and we moved it 
lover to embrace the Pass 1 as well.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

[-- Attachment #2: jffs2_mount_fix-01.diff --]
[-- Type: text/x-patch, Size: 3021 bytes --]

diff -auNr --exclude=CVS mtd/fs/jffs2/build.c mtd-tst/fs/jffs2/build.c
--- mtd/fs/jffs2/build.c	2004-12-16 23:22:18.000000000 +0300
+++ mtd-tst/fs/jffs2/build.c	2005-02-27 12:56:19.000000000 +0300
@@ -97,14 +97,16 @@
 	/* First, scan the medium and build all the inode caches with
 	   lists of physical nodes */
 
-	c->flags |= JFFS2_SB_FLAG_MOUNTING;
+	c->flags |= JFFS2_SB_FLAG_SCANNING;
 	ret = jffs2_scan_medium(c);
+	c->flags &= ~JFFS2_SB_FLAG_SCANNING;
 	if (ret)
 		goto exit;
 
 	D1(printk(KERN_DEBUG "Scanned flash completely\n"));
 	D2(jffs2_dump_block_lists(c));
 
+	c->flags |= JFFS2_SB_FLAG_BUILDING;
 	/* Now scan the directory tree, increasing nlink according to every dirent found. */
 	for_each_inode(i, c, ic) {
 		D1(printk(KERN_DEBUG "Pass 1: ino #%u\n", ic->ino));
@@ -116,7 +118,6 @@
 			cond_resched();
 		}
 	}
-	c->flags &= ~JFFS2_SB_FLAG_MOUNTING;
 
 	D1(printk(KERN_DEBUG "Pass 1 complete\n"));
 
@@ -164,6 +165,8 @@
 		ic->scan_dents = NULL;
 		cond_resched();
 	}
+	c->flags &= ~JFFS2_SB_FLAG_BUILDING;
+	
 	D1(printk(KERN_DEBUG "Pass 3 complete\n"));
 	D2(jffs2_dump_block_lists(c));
 
diff -auNr --exclude=CVS mtd/fs/jffs2/nodemgmt.c mtd-tst/fs/jffs2/nodemgmt.c
--- mtd/fs/jffs2/nodemgmt.c	2005-01-25 23:11:11.000000000 +0300
+++ mtd-tst/fs/jffs2/nodemgmt.c	2005-02-27 13:04:36.000000000 +0300
@@ -403,7 +403,7 @@
 	jeb = &c->blocks[blocknr];
 
 	if (jffs2_can_mark_obsolete(c) && !jffs2_is_readonly(c) &&
-	    !(c->flags & JFFS2_SB_FLAG_MOUNTING)) {
+	    !(c->flags & (JFFS2_SB_FLAG_SCANNING | JFFS2_SB_FLAG_BUILDING))) {
 		/* Hm. This may confuse static lock analysis. If any of the above 
 		   three conditions is false, we're going to return from this 
 		   function without actually obliterating any nodes or freeing
@@ -470,8 +470,8 @@
 
 	D1(ACCT_PARANOIA_CHECK(jeb));
 
-	if (c->flags & JFFS2_SB_FLAG_MOUNTING) {
-		/* Mount in progress. Don't muck about with the block
+	if (c->flags & JFFS2_SB_FLAG_SCANNING) {
+		/* Flash scanning is 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_
@@ -530,7 +530,8 @@
 
 	spin_unlock(&c->erase_completion_lock);
 
-	if (!jffs2_can_mark_obsolete(c) || jffs2_is_readonly(c)) {
+	if (!jffs2_can_mark_obsolete(c) || jffs2_is_readonly(c) ||
+		(c->flags & JFFS2_SB_FLAG_BUILDING)) {
 		/* We didn't lock the erase_free_sem */
 		return;
 	}
diff -auNr --exclude=CVS mtd/include/linux/jffs2_fs_sb.h mtd-tst/include/linux/jffs2_fs_sb.h
--- mtd/include/linux/jffs2_fs_sb.h	2005-02-09 12:23:55.000000000 +0300
+++ mtd-tst/include/linux/jffs2_fs_sb.h	2005-02-27 13:13:36.000000000 +0300
@@ -14,7 +14,8 @@
 #include <linux/rwsem.h>
 
 #define JFFS2_SB_FLAG_RO 1
-#define JFFS2_SB_FLAG_MOUNTING 2
+#define JFFS2_SB_FLAG_SCANNING 2 /* Flash scanning is in progress */
+#define JFFS2_SB_FLAG_BUILDING 4 /* File system building is in progress */
 
 struct jffs2_inodirty;
 

Re: [BUGFIX] JFFS2 NOR problem

[Ray Lehtiniemi]

On Sat, Feb 26, 2005 at 10:43:52PM -0500, Craig A. Vanderborgh wrote:
> Artem B. Bityuckiy wrote:
> 
> >Hello,
> >
> >I've detected major JFFS2 bug and have attached the fix. The bug seems
> >to be NOR-only. I believe everybody who uses JFFS2 on top of NOR flash
> >should have this bug fixed.

> Thanks a lot for looking into this, we REALLY appreciate it and it's 
> great to be fully operational on kernel 2.6!!


i have been experiencing this bug as well, and can confirm that the
patch to move "c->flags &= ~JFFS2_SB_FLAG_MOUNTING" seems to clear up
the panic for me.


however, there are still some issues... 


if i enable CONFIG_DEBUG_SLAB, i get this message a few seconds after
the filesystem is mounted:

   VFS: Mounted root (jffs2 filesystem).
   Freeing init memory: 84K
   Initializing random number generator... Slab corruption: start=c1475924, len=24
   Redzone: 0x5a2cf071/0x5a2cf071.
   Last user: [<c00fd468>](jffs2_mark_node_obsolete+0x1300/0x1648)
   010: 6b 6b 6b 6b 03 00 00 00
   Prev obj: start=c1475900, len=24
   Redzone: 0x170fc2a5/0x170fc2a5.
   Last user: [<c0102ac4>](jffs2_scan_make_ino_cache+0x34/0x7c)
   000: 00 00 00 00 34 be 47 c1 fc 68 47 c1 a1 00 00 00
   010: 01 00 00 00 03 00 00 00
   Next obj: start=c1475948, len=24
   Redzone: 0x170fc2a5/0x170fc2a5.
   Last user: [<c0102ac4>](jffs2_scan_make_ino_cache+0x34/0x7c)
   000: 00 00 00 00 7c be 47 c1 ec 65 47 c1 9f 00 00 00
   010: 01 00 00 00 03 00 00 00
   done.
   Starting network...

the boot process then appears to continue normally.



i had seen this message before applying the patch as well. i was able to
reproduce it as follows:

prior to trying this patch, i had determined that the panic occurred
while mounting a virgin (never-been-mounted) jffs2 root filesystem
while CONFIG_DEBUG_SLAB=y. if CONFIG_DEBUG_SLAB=n, the mount just
appeared to "work"... and then if i re-enabled slab debugging and
mounted the non-virgin jffs2 root, i would then see the above-mentioned
slab corruption message.

after applying the patch, i now proceed directly to the slab corruption
message, virgin fs or not.





i am running on a cirrus ep9302-based EDB9302 board. it has a non-intel
clone device on it, marked MT28F128J3:

   physmap flash device: 1000000 at 60000000
   phys_mapped_flash: Found 1 x16 devices at 0x0 in 16-bit bank
    Intel/Sharp Extended Query Table at 0x0031
   Using buffer write method
   cfi_cmdset_0001: Erase suspend on write enabled



my kernel is:

  - newest linux-cirrus bk tree
    - this tree is synced to linux-2.6.11-rc3
    - so it should contain russell's 2.6.11-rc1 pud_t slab corruption fix

  - mtd-snapshot-20050225.tar.bz2 has been applied using 'patchin.sh -j -c'

  - CONFIG_JFFS2_FS_DEBUG=1 (but i didn't see any extra output...)



my userspace is a 2005-02-26 svn checkout of buildroot,  using busybox
and uClibc snapshots.  my toolchain is also from buildroot, consisting
of binutils-2.15.91.0.2 and gcc-3.4.2. i replaced the standard mtd
download from debian with the mtd snapshot from linux.org.uk.



please let me know if i can provide any more info.



thanks

-- 
----------------------------------------------------------------------
     Ray L   <rayl@mail.com>

Re: [BUGFIX] JFFS2 NOR problem

[Artem B. Bityuckiy]

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

On Sun, 2005-02-27 at 11:10 -0700, Ray Lehtiniemi wrote:
> however, there are still some issues... 

I can't be completely certain but it is likely that your problem is the
same problem that I've observed about a month ago. Take a glimpse at:
http://lists.infradead.org/pipermail/linux-mtd/2005-January/011357.html

The discussion was stopped but has been resumed today. Please, follow
the thread if you're curious.

Here is a somewhat tweaked David's patch. I tried it and it looks like
it works pretty good. Please, try it together with my last mount fix.

P.S. We'll commit both patches to the CVS after you (and others) have
reported they are OK.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

[-- Attachment #2: jff2_slab_fix-01.diff --]
[-- Type: text/x-patch, Size: 3874 bytes --]

diff -auNr --exclude=CVS mtd/fs/jffs2/erase.c mtd-dwmw2fix/fs/jffs2/erase.c
--- mtd/fs/jffs2/erase.c	2005-02-09 12:17:40.000000000 +0300
+++ mtd-dwmw2fix/fs/jffs2/erase.c	2005-02-27 21:19:56.587664987 +0300
@@ -277,11 +277,8 @@
 		printk("\n");
 	});
 
-	if (ic->nodes == (void *)ic) {
-		D1(printk(KERN_DEBUG "inocache for ino #%u is all gone now. Freeing\n", ic->ino));
+	if (ic->nodes == (void *)ic)
 		jffs2_del_ino_cache(c, ic);
-		jffs2_free_inode_cache(ic);
-	}
 }
 
 static void jffs2_free_all_node_refs(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
diff -auNr --exclude=CVS mtd/fs/jffs2/nodelist.c mtd-dwmw2fix/fs/jffs2/nodelist.c
--- mtd/fs/jffs2/nodelist.c	2005-01-19 22:22:00.000000000 +0300
+++ mtd-dwmw2fix/fs/jffs2/nodelist.c	2005-02-27 21:28:41.013605613 +0300
@@ -508,7 +508,7 @@
 void jffs2_del_ino_cache(struct jffs2_sb_info *c, struct jffs2_inode_cache *old)
 {
 	struct jffs2_inode_cache **prev;
-	D2(printk(KERN_DEBUG "jffs2_del_ino_cache: Del %p (ino #%u)\n", old, old->ino));
+	D1(printk(KERN_DEBUG "jffs2_del_ino_cache: Del %p (ino #%u)\n", old, old->ino));
 	spin_lock(&c->inocache_lock);
 	
 	prev = &c->inocache_list[old->ino % INOCACHE_HASHSIZE];
@@ -520,6 +520,14 @@
 		*prev = old->next;
 	}
 
+	/* Free it now unless it's in READING or CLEARING state, which
+	   are the transitions upon read_inode() and clear_inode(). The
+	   rest of the time we know nobody else is looking at it, and 
+	   if it's held by read_inode() or clear_inode() they'll free it
+	   for themselves. */
+	if (old->state != INO_STATE_READING && old->state != INO_STATE_CLEARING)
+		jffs2_free_inode_cache(old);
+
 	spin_unlock(&c->inocache_lock);
 }
 
@@ -532,7 +540,6 @@
 		this = c->inocache_list[i];
 		while (this) {
 			next = this->next;
-			D2(printk(KERN_DEBUG "jffs2_free_ino_caches: Freeing ino #%u at %p\n", this->ino, this));
 			jffs2_free_inode_cache(this);
 			this = next;
 		}
diff -auNr --exclude=CVS mtd/fs/jffs2/nodelist.h mtd-dwmw2fix/fs/jffs2/nodelist.h
--- mtd/fs/jffs2/nodelist.h	2005-02-09 12:23:53.000000000 +0300
+++ mtd-dwmw2fix/fs/jffs2/nodelist.h	2005-02-27 21:19:56.589664558 +0300
@@ -135,6 +135,7 @@
 #define INO_STATE_CHECKEDABSENT	3	/* Checked, cleared again */
 #define INO_STATE_GC		4	/* GCing a 'pristine' node */
 #define INO_STATE_READING	5	/* In read_inode() */
+#define INO_STATE_CLEARING	6	/* In clear_inode() */
 
 #define INOCACHE_HASHSIZE 128
 
diff -auNr --exclude=CVS mtd/fs/jffs2/nodemgmt.c mtd-dwmw2fix/fs/jffs2/nodemgmt.c
--- mtd/fs/jffs2/nodemgmt.c	2005-01-25 23:11:11.000000000 +0300
+++ mtd-dwmw2fix/fs/jffs2/nodemgmt.c	2005-02-27 21:19:56.590664344 +0300
@@ -593,11 +593,8 @@
 		*p = ref->next_in_ino;
 		ref->next_in_ino = NULL;
 
-		if (ic->nodes == (void *)ic) {
-			D1(printk(KERN_DEBUG "inocache for ino #%u is all gone now. Freeing\n", ic->ino));
+		if (ic->nodes == (void *)ic)
 			jffs2_del_ino_cache(c, ic);
-			jffs2_free_inode_cache(ic);
-		}
 
 		spin_unlock(&c->erase_completion_lock);
 	}
diff -auNr --exclude=CVS mtd/fs/jffs2/readinode.c mtd-dwmw2fix/fs/jffs2/readinode.c
--- mtd/fs/jffs2/readinode.c	2004-11-20 21:06:54.000000000 +0300
+++ mtd-dwmw2fix/fs/jffs2/readinode.c	2005-02-27 21:28:19.009070839 +0300
@@ -672,6 +672,9 @@
 	down(&f->sem);
 	deleted = f->inocache && !f->inocache->nlink;
 
+	if (f->inocache && f->inocache->state != INO_STATE_CHECKING)
+		jffs2_set_inocache_state(c, f->inocache, INO_STATE_CLEARING);
+
 	if (f->metadata) {
 		if (deleted)
 			jffs2_mark_node_obsolete(c, f->metadata->raw);
@@ -688,8 +691,11 @@
 		jffs2_free_full_dirent(fd);
 	}
 
-	if (f->inocache && f->inocache->state != INO_STATE_CHECKING)
+	if (f->inocache && f->inocache->state != INO_STATE_CHECKING) {
 		jffs2_set_inocache_state(c, f->inocache, INO_STATE_CHECKEDABSENT);
+		if (f->inocache->nodes == (void *)f->inocache)
+			jffs2_del_ino_cache(c, f->inocache);
+	}
 
 	up(&f->sem);
 }

Re: [BUGFIX] JFFS2 NOR problem

[Ray Lehtiniemi]

On Sun, Feb 27, 2005 at 09:31:55PM +0300, Artem B. Bityuckiy wrote:
> 
> I can't be completely certain but it is likely that your problem is the
> same problem that I've observed about a month ago. Take a glimpse at:
> http://lists.infradead.org/pipermail/linux-mtd/2005-January/011357.html

seems like it, yes.


> Here is a somewhat tweaked David's patch. I tried it and it looks like
> it works pretty good. Please, try it together with my last mount fix.
> 
> P.S. We'll commit both patches to the CVS after you (and others) have
> reported they are OK.

i have applied this patch against 2.6.11-rc3 + 20050225 mtd snapshot +
your previous patch, and it seems to cure both the oops with a virgin
fs and the slab corruption with a non-virgin fs.


thanks!

-- 
----------------------------------------------------------------------
     Ray L   <rayl@mail.com>