diff mbox

JFFS2: jffs2_symlink/jffs2_mkdir/jffs2_mknod bug?

Message ID 1259744053.19465.902.camel@macbook.infradead.org
State New, archived
Headers show

Commit Message

David Woodhouse Dec. 2, 2009, 8:54 a.m. UTC 
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...?

Comments

Joakim Tjernlund Dec. 2, 2009, 9:16 a.m. UTC | #1 
>
> 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
David Woodhouse Dec. 2, 2009, 9:48 a.m. UTC | #2 
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).
diff mbox

Patch

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);