Hi Li / Andrew,
On Wed, Feb 12, 2014 at 12:44:55PM -0800, Andrew Morton wrote:
> From: Li Zefan <lizefan@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> Cc: Brian Norris <computersforpeace@xxxxxxxxx>
> Cc: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html