JFFS2: jffs2_symlink/jffs2_mkdir/jffs2_mknod bug?

JFFS2: jffs2_symlink/jffs2_mkdir/jffs2_mknod bug?

[Tao Huang]

On jffs2_symlink/jffs2_mkdir/jffs2_mknod, after jffs2_write_dnode,
any call jffs2_clear_inode will no call jffs2_mark_node_obsolete because
pino_nlink is not zero. This will make kernel BUG on jffs2_garbage_collect_live.

Run fsstress on NANDSIM can easy see this bug when no space, for example:
fsstress -p 2 -n 100000 -d /data -l 0 -s 100000 on 128KB NANDSIM.

Re: JFFS2: jffs2_symlink/jffs2_mkdir/jffs2_mknod bug?

[David Woodhouse]

On Wed, 2009-12-02 at 10:11 +0800, Tao Huang wrote:
> On jffs2_symlink/jffs2_mkdir/jffs2_mknod, after jffs2_write_dnode,
> any call jffs2_clear_inode will no call jffs2_mark_node_obsolete because
> pino_nlink is not zero. This will make kernel BUG on jffs2_garbage_collect_live.
> 
> Run fsstress on NANDSIM can easy see this bug when no space, for example:
> fsstress -p 2 -n 100000 -d /data -l 0 -s 100000 on 128KB NANDSIM. 

Hm, good catch; thanks.

We _need_ to have a non-zero pino_nlink for new inodes during their
creation. We can't just create them with zero nlink and then use
jffs2_do_link() because that would leave an opportunity for the garbage
collector to delete them in between.

So we need to zero the nlink for a 'failed' creation. Something like
this...?

diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index 7aa4417..f91aa89 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -358,6 +358,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 
 	if (IS_ERR(fn)) {
 		/* Eeek. Wave bye bye */
+		f->inocache->pino_nlink = 0;
 		mutex_unlock(&f->sem);
 		jffs2_complete_reservation(c);
 		jffs2_clear_inode(inode);
@@ -368,6 +369,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 	f->target = kmalloc(targetlen + 1, GFP_KERNEL);
 	if (!f->target) {
 		printk(KERN_WARNING "Can't allocate %d bytes of memory\n", targetlen + 1);
+		f->inocache->pino_nlink = 0;
 		mutex_unlock(&f->sem);
 		jffs2_complete_reservation(c);
 		jffs2_clear_inode(inode);
@@ -387,11 +389,17 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 
 	ret = jffs2_init_security(inode, dir_i);
 	if (ret) {
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
 	ret = jffs2_init_acl_post(inode);
 	if (ret) {
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
@@ -400,6 +408,9 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 				  ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
 	if (ret) {
 		/* Eep. */
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
@@ -408,6 +419,9 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 	if (!rd) {
 		/* Argh. Now we treat it like a normal delete */
 		jffs2_complete_reservation(c);
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return -ENOMEM;
 	}
@@ -436,6 +450,7 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 		   as if it were the final unlink() */
 		jffs2_complete_reservation(c);
 		jffs2_free_raw_dirent(rd);
+		f->inocache->pino_nlink = 0;
 		mutex_unlock(&dir_f->sem);
 		jffs2_clear_inode(inode);
 		return PTR_ERR(fd);
@@ -517,6 +532,7 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, int mode)
 
 	if (IS_ERR(fn)) {
 		/* Eeek. Wave bye bye */
+		f->inocache->pino_nlink = 0;
 		mutex_unlock(&f->sem);
 		jffs2_complete_reservation(c);
 		jffs2_clear_inode(inode);
@@ -532,11 +548,17 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, int mode)
 
 	ret = jffs2_init_security(inode, dir_i);
 	if (ret) {
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
 	ret = jffs2_init_acl_post(inode);
 	if (ret) {
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
@@ -545,6 +567,9 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, int mode)
 				  ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
 	if (ret) {
 		/* Eep. */
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
@@ -553,6 +578,9 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, int mode)
 	if (!rd) {
 		/* Argh. Now we treat it like a normal delete */
 		jffs2_complete_reservation(c);
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return -ENOMEM;
 	}
@@ -581,6 +609,7 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, int mode)
 		   as if it were the final unlink() */
 		jffs2_complete_reservation(c);
 		jffs2_free_raw_dirent(rd);
+		f->inocache->pino_nlink = 0;
 		mutex_unlock(&dir_f->sem);
 		jffs2_clear_inode(inode);
 		return PTR_ERR(fd);
@@ -691,6 +720,7 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, int mode, de
 
 	if (IS_ERR(fn)) {
 		/* Eeek. Wave bye bye */
+		f->inocache->pino_nlink = 0;
 		mutex_unlock(&f->sem);
 		jffs2_complete_reservation(c);
 		jffs2_clear_inode(inode);
@@ -706,11 +736,17 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, int mode, de
 
 	ret = jffs2_init_security(inode, dir_i);
 	if (ret) {
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
 	ret = jffs2_init_acl_post(inode);
 	if (ret) {
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
@@ -719,6 +755,9 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, int mode, de
 				  ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
 	if (ret) {
 		/* Eep. */
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return ret;
 	}
@@ -727,6 +766,9 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, int mode, de
 	if (!rd) {
 		/* Argh. Now we treat it like a normal delete */
 		jffs2_complete_reservation(c);
+		mutex_lock(&f->sem);
+		f->inocache->pino_nlink = 0;
+		mutex_unlock(&f->sem);
 		jffs2_clear_inode(inode);
 		return -ENOMEM;
 	}
@@ -758,6 +800,7 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, int mode, de
 		   as if it were the final unlink() */
 		jffs2_complete_reservation(c);
 		jffs2_free_raw_dirent(rd);
+		f->inocache->pino_nlink = 0;
 		mutex_unlock(&dir_f->sem);
 		jffs2_clear_inode(inode);
 		return PTR_ERR(fd);


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: JFFS2: jffs2_symlink/jffs2_mkdir/jffs2_mknod bug?

[Joakim Tjernlund]

>
> On Wed, 2009-12-02 at 10:11 +0800, Tao Huang wrote:
> > On jffs2_symlink/jffs2_mkdir/jffs2_mknod, after jffs2_write_dnode,
> > any call jffs2_clear_inode will no call jffs2_mark_node_obsolete because
> > pino_nlink is not zero. This will make kernel BUG on jffs2_garbage_collect_live.
> >
> > Run fsstress on NANDSIM can easy see this bug when no space, for example:
> > fsstress -p 2 -n 100000 -d /data -l 0 -s 100000 on 128KB NANDSIM.
>
> Hm, good catch; thanks.
>
> We _need_ to have a non-zero pino_nlink for new inodes during their
> creation. We can't just create them with zero nlink and then use
> jffs2_do_link() because that would leave an opportunity for the garbage
> collector to delete them in between.
>
> So we need to zero the nlink for a 'failed' creation. Something like
> this...?

> +      mutex_lock(&f->sem);
> +      f->inocache->pino_nlink = 0;
> +      mutex_unlock(&f->sem);

Do you need mutex lock/unlock around a simple assignment?

    Jocke

Re: JFFS2: jffs2_symlink/jffs2_mkdir/jffs2_mknod bug?

[David Woodhouse]

On Wed, 2009-12-02 at 10:16 +0100, Joakim Tjernlund wrote:
> Do you need mutex lock/unlock around a simple assignment? 

Those are the locking rules.

The GC code may be currently processing this inode, and have the mutex
locked. It has the right to expect that fields of the data structure
won't be changed underneath it while it has the lock -- that's kind of
the point of the locking, isn't it?

It _might_ be the case that it wouldn't matter, because of the way that
the GC code uses the pino_nlink field, and because the mutex is going to
get locked by jffs2_do_clear_inode() anyway, so the inode won't
_actually_ get cleaned up until the GC code has finished with it.

But if you want to violate the locking rules, you'd need to _prove_ that
it's safe. And why would you bother? It's not as if it's worth
optimising this code path for performance -- it's a rarely-used error
path (which is why it's existed fairly much for ever without anyone
noticing. I don't think it was introduced by the more recent pino_nlink
merge; it was there when the nlink field was separate).

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: JFFS2: jffs2_symlink/jffs2_mkdir/jffs2_mknod bug?

[David Woodhouse]

On Wed, 2009-12-02 at 10:11 +0800, Tao Huang wrote:
> On jffs2_symlink/jffs2_mkdir/jffs2_mknod, after jffs2_write_dnode,
> any call jffs2_clear_inode will no call jffs2_mark_node_obsolete
> because pino_nlink is not zero.

True, I think. Well spotted. Although it may actually turn out to be
harmless, for reasons similar to those I outlined to Joakim earlier.

At worst, it'll cause a space leak, because until the next reboot we'll
think that the inode in question has a non-zero nlink and can't be
removed.

>  This will make kernel BUG on jffs2_garbage_collect_live.

But this isn't true. In fact, there isn't even a BUG in
jffs2_garbage_collect_live() -- you're seeing an Oops, not a BUG.
And it's not related to the locking issue. It's this:

diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index 090c556..3b6f2fa 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -700,7 +700,8 @@ static int jffs2_garbage_collect_metadata(struct jffs2_sb_info *c, struct jffs2_
 	struct jffs2_raw_inode ri;
 	struct jffs2_node_frag *last_frag;
 	union jffs2_device_node dev;
-	char *mdata = NULL, mdatalen = 0;
+	char *mdata = NULL;
+	int mdatalen = 0;
 	uint32_t alloclen, ilen;
 	int ret;
 
It looks like garbage collecting any symlink with a target length of
more than 128 characters, on a system where char is signed by default,
was broken since February 2001 when jffs2_garbage_collect_metadata() was
first (half-)written.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation