On 2014/2/13 14:48, Brian Norris wrote:
> 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!
>
But I don't think I should be blamed here.
Without my patch, some of the error paths release f->sem but some don't,
so the potential double-unlock is already there.
> 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:
>
Yeah, I do believe it's better to do locking/unlocking in the same level.
How about this:
[PATCH v2] jffs2: fix unbalanced locking
On runtime our internal debugging feature warned f->sem isn't unlocked
when returning to userspace. It's because f->sem isn't unlocked in
jffs2_do_crccheck_inode() on error, but this bug won't lead to deadlock,
as the structure that this lock is embedded in is freed immediately.
After looking into the code, I found in jffs2_do_read_inode_internal()
some error paths release f->sem but some won't, so this may lead to
double-unlock:
jffs2_iget()
|_ mutex_lock(&f->sem)
|_ jffs2_do_read_inode()
| |_ jffs2_do_read_inode_internal()
| |_ mutex_unlock(&f->sem)
| |_ jffs2_do_clear_inode(c, f)
| |_ return ret
|_ mutex_unlock(&f->sem)
This patch makes sure jffs2_do_read_inode_internal() never returns
with f->sem unlocked, so locking and unlocking are in the same level.
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Li Zefan <lizefan@xxxxxxxxxx>
---
fs/jffs2/readinode.c | 53 ++++++++++++++++++++++++----------------------------
1 file changed, 24 insertions(+), 29 deletions(-)
diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index 386303d..6f22234 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -1203,18 +1203,16 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
JFFS2_ERROR("failed to read from flash: error %d, %zd of %zd bytes read\n",
ret, retlen, sizeof(*latest_node));
/* FIXME: If this fails, there seems to be a memory leak. Find it. */
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return ret?ret:-EIO;
+ ret = ret ? ret : -EIO;
+ goto out;
}
crc = crc32(0, latest_node, sizeof(*latest_node)-8);
if (crc != je32_to_cpu(latest_node->node_crc)) {
JFFS2_ERROR("CRC failed for read_inode of inode %u at physical location 0x%x\n",
f->inocache->ino, ref_offset(rii.latest_ref));
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
switch(jemode_to_cpu(latest_node->mode) & S_IFMT) {
@@ -1251,16 +1249,14 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
* operation. */
uint32_t csize = je32_to_cpu(latest_node->csize);
if (csize > JFFS2_MAX_NAME_LEN) {
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return -ENAMETOOLONG;
+ ret = -ENAMETOOLONG;
+ goto out;
}
f->target = kmalloc(csize + 1, GFP_KERNEL);
if (!f->target) {
JFFS2_ERROR("can't allocate %u bytes of memory for the symlink target path cache\n", csize);
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
ret = jffs2_flash_read(c, ref_offset(rii.latest_ref) + sizeof(*latest_node),
@@ -1271,9 +1267,7 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
ret = -EIO;
kfree(f->target);
f->target = NULL;
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return ret;
+ goto out;
}
f->target[csize] = '\0';
@@ -1289,25 +1283,22 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
if (f->metadata) {
JFFS2_ERROR("Argh. Special inode #%u with mode 0%o had metadata node\n",
f->inocache->ino, jemode_to_cpu(latest_node->mode));
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
if (!frag_first(&f->fragtree)) {
JFFS2_ERROR("Argh. Special inode #%u with mode 0%o has no fragments\n",
f->inocache->ino, jemode_to_cpu(latest_node->mode));
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
/* ASSERT: f->fraglist != NULL */
if (frag_next(frag_first(&f->fragtree))) {
JFFS2_ERROR("Argh. Special inode #%u with mode 0x%x had more than one node\n",
f->inocache->ino, jemode_to_cpu(latest_node->mode));
/* FIXME: Deal with it - check crc32, check for duplicate node, check times and discard the older one */
- mutex_unlock(&f->sem);
- jffs2_do_clear_inode(c, f);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
/* OK. We're happy */
f->metadata = frag_first(&f->fragtree)->node;
@@ -1317,8 +1308,13 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
}
if (f->inocache->state == INO_STATE_READING)
jffs2_set_inocache_state(c, f->inocache, INO_STATE_PRESENT);
-
- return 0;
+out:
+ if (ret) {
+ mutex_unlock(&f->sem);
+ jffs2_do_clear_inode(c, f);
+ mutex_lock(&f->sem);
+ }
+ return ret;
}
/* Scan the list of all nodes present for this ino, build map of versions, etc. */
@@ -1400,10 +1396,9 @@ 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);
- if (!ret) {
- mutex_unlock(&f->sem);
+ mutex_unlock(&f->sem);
+ if (!ret)
jffs2_do_clear_inode(c, f);
- }
jffs2_xattr_do_crccheck_inode(c, ic);
kfree (f);
return ret;
--
1.8.0.2
--
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