[patch 2/4] jffs2: fix unbalanced locking

[Brian Norris]

Hi Li / Andrew,

On Wed, Feb 12, 2014 at 12:44:55PM -0800, Andrew Morton wrote:
> From: Li Zefan <lizefan at huawei.com>
> Subject: jffs2: fix unbalanced locking
> 
> This was found by our internal debugging feature on runtime, but this bug
> won't lead to deadlock, as the structure that this lock is embedded in is
> freed on error.

Well, one of its callers frees it without unlocking it, but you're
forgetting about one of its other callers, and in doing so, you are
introducing a potential double-unlock instead!

Look at
  jffs2_iget()
  |_ mutex_lock(&f->sem)
  |_ jffs2_do_read_inode()
  |  |_ jffs2_do_read_inode_internal()
  |_ mutex_unlock(&f->sem)

jffs2_iget() already has the proper locking for f->sem, but with your
patch, you're turning this into a double-unlock in the error case.

So unless I'm mistaken, I'll give a NAK to this patch. It's one of those
patches generated by automated testing that has no practical value, but
rather has the potential to cause more bugs.

BTW, the right way to handle lock balancing is to handle the unlocking
at the same level where you do the locking. So I guess you're looking
for the following patch instead, which is really not very useful because
(as Li noted) the lock is freed immediately afterward anyway:

diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index 386303dca382..d28e6eafce05 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -1400,8 +1400,8 @@ int jffs2_do_crccheck_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *i
 	f->inocache = ic;
 
 	ret = jffs2_do_read_inode_internal(c, f, &n);
+	mutex_unlock(&f->sem);
 	if (!ret) {
-		mutex_unlock(&f->sem);
 		jffs2_do_clear_inode(c, f);
 	}
 	jffs2_xattr_do_crccheck_inode(c, ic);

> Signed-off-by: Li Zefan <lizefan at huawei.com>
> Cc: David Woodhouse <dwmw2 at infradead.org>
> Cc: Brian Norris <computersforpeace at gmail.com>
> Cc: Artem Bityutskiy <artem.bityutskiy at linux.intel.com>
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> ---
> 
>  fs/jffs2/readinode.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff -puN fs/jffs2/readinode.c~jffs2-fix-unbalanced-locking fs/jffs2/readinode.c
> --- a/fs/jffs2/readinode.c~jffs2-fix-unbalanced-locking
> +++ a/fs/jffs2/readinode.c
> @@ -1143,6 +1143,7 @@ static int jffs2_do_read_inode_internal(
>  		JFFS2_ERROR("cannot read nodes for ino %u, returned error is %d\n", f->inocache->ino, ret);
>  		if (f->inocache->state == INO_STATE_READING)
>  			jffs2_set_inocache_state(c, f->inocache, INO_STATE_CHECKEDABSENT);
> +		mutex_unlock(&f->sem);
>  		return ret;
>  	}
>  
> @@ -1159,6 +1160,7 @@ static int jffs2_do_read_inode_internal(
>  			jffs2_free_tmp_dnode_info(rii.mdata_tn);
>  			rii.mdata_tn = NULL;
>  		}
> +		mutex_unlock(&f->sem);
>  		return ret;
>  	}
>  
> @@ -1183,6 +1185,7 @@ static int jffs2_do_read_inode_internal(
>  			if (!rii.fds) {
>  				if (f->inocache->state == INO_STATE_READING)
>  					jffs2_set_inocache_state(c, f->inocache, INO_STATE_CHECKEDABSENT);
> +				mutex_unlock(&f->sem);
>  				return -EIO;
>  			}
>  			JFFS2_NOTICE("but it has children so we fake some modes for it\n");
> _

Brian