Re: [patch 1/4] jffs2: unlock f->sem on error in jffs2_new_inode()

[Brian Norris <computersforpeace@gmail.com>]

On Thu, Feb 27, 2014 at 04:33:20PM +0800, Wang Nan wrote:
> On 2014/2/27 14:55, Brian Norris wrote:
> > 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 <andy.wangguoli@huawei.com>
> >>>> 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.
> 
> Thank you for your review, but it is still an obvious bug, isn't it?

Maybe. I'm not really an expert in file systems, but it looks like you
won't ever hit a deadlock, because no one will ever get a reference to
this inode in the error path, and therefore no one will ever try to
re-lock the lock. So while it looks like an inconsistent error path, it
may be benign, and therefore this patch is not really a necessary
bugfix.

Again, I'm not really sure, and I'm not willing to stake a "stable"
patch on code reading by two non-experts with no test case. As I see it,
Documentation/stable_kernel_rules.txt is written for a reason.

> We will work for test case, and will let you know if we successfully
> trigger deadlock.

Yes, please do let me know if you can identify one in practice. Or if
you can convince me how the inode may leak out to cause a deadlock on
the error paths in question.

Brian