From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pb0-x233.google.com ([2607:f8b0:400e:c01::233]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WIut6-0002cM-OH for linux-mtd@lists.infradead.org; Thu, 27 Feb 2014 06:56:10 +0000 Received: by mail-pb0-f51.google.com with SMTP id un15so2117107pbc.38 for ; Wed, 26 Feb 2014 22:55:44 -0800 (PST) Date: Wed, 26 Feb 2014 22:55:40 -0800 From: Brian Norris To: Wang Nan Subject: Re: [patch 1/4] jffs2: unlock f->sem on error in jffs2_new_inode() Message-ID: <20140227065540.GC13420@norris-Latitude-E6410> References: <20140212204454.8FA415A40F6@corp2gmr1-2.hot.corp.google.com> <20140226020331.GB4194@ld-irv-0074> <530E8C91.4060703@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <530E8C91.4060703@huawei.com> Cc: artem.bityutskiy@linux.intel.com, stable@vger.kernel.org, linux-mtd@lists.infradead.org, andy.wangguoli@huawei.com, genghui 00204690 , akpm@linux-foundation.org, dwmw2@infradead.org List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , + linux-mtd Hi Wang, On Thu, Feb 27, 2014 at 08:53:37AM +0800, Wang Nan wrote: > On 2014/2/26 10:03, Brian Norris wrote: > > On Wed, Feb 12, 2014 at 12:44:54PM -0800, Andrew Morton wrote: > >> From: Wang Guoli > >> Subject: jffs2: unlock f->sem on error in jffs2_new_inode() > >> > >> If jffs2_new_inode() succeeds, it returns with f->sem held, and the caller > >> is responsible for releasing the lock. If it fails, it still returns with > >> the lock held, but the caller won't release the lock, which will lead to > >> deadlock. > > > > Have you actually seen a deadlock for this? AFAICT, the error cases for > > jffs2_new_inode() all occur before anyone else actually has a reference > > to the inode, so I don't expect that we should see the deadlock. > > > > We found this bug by reading code. There's no deadlock have been actually seen. Hmm, then I think maybe we should drop the stable tag. I'm not sure it's a practical issue, and it also has unrelated indentation changes. That means it breaks two of the rules in Documentation/stable_kernel_rules.txt. I'm dropping the stable tag unless someone shouts. > >> Fix it by releasing the lock in jffs2_new_inode() on error. > >> > >> Signed-off-by: Wang Guoli > >> Signed-off-by: Wang Nan > >> Cc: Artem Bityutskiy > >> Cc: David Woodhouse > >> Cc: Wang Guoli > >> Cc: Brian Norris > >> Cc: # 2.6.34+ > >> Signed-off-by: Andrew Morton > >> --- > >> > >> fs/jffs2/fs.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff -puN fs/jffs2/fs.c~jffs2-unlock-f-sem-on-error-in-jffs2_new_inode fs/jffs2/fs.c > >> --- a/fs/jffs2/fs.c~jffs2-unlock-f-sem-on-error-in-jffs2_new_inode > >> +++ a/fs/jffs2/fs.c > >> @@ -457,12 +457,14 @@ struct inode *jffs2_new_inode (struct in > >> The umask is only applied if there's no default ACL */ > >> ret = jffs2_init_acl_pre(dir_i, inode, &mode); > >> if (ret) { > >> - make_bad_inode(inode); > >> - iput(inode); > >> - return ERR_PTR(ret); > >> + mutex_unlock(&f->sem); > >> + make_bad_inode(inode); > >> + iput(inode); > >> + return ERR_PTR(ret); > >> } > >> ret = jffs2_do_new_inode (c, f, mode, ri); > >> if (ret) { > >> + mutex_unlock(&f->sem); > >> make_bad_inode(inode); > >> iput(inode); > >> return ERR_PTR(ret); > >> @@ -479,6 +481,7 @@ struct inode *jffs2_new_inode (struct in > >> inode->i_size = 0; > >> > >> if (insert_inode_locked(inode) < 0) { > >> + mutex_unlock(&f->sem); > >> make_bad_inode(inode); > >> iput(inode); > >> return ERR_PTR(-EINVAL); Brian