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 <[email protected]> >> 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: <[email protected]> Signed-off-by: Li Zefan <[email protected]> --- 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
