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

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

[Brian Norris]

+ linux-mtd (I realized MTD wasn't CC'd on these re-sends)

On Tue, Feb 25, 2014 at 06:03:31PM -0800, Brian Norris wrote:
> Hi Andrew,
> 
> I've run all 4 of these patches (for the second one, I'm using my latest
> version; I'll resend it formally) through a little bit of testing here.
> 
> Hi Wang,
> 
> Thanks for the patch.
> 
> 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.
> 
> > Fix it by releasing the lock in jffs2_new_inode() on error.
> > 
> > Signed-off-by: Wang Guoli <andy.wangguoli@huawei.com>
> > Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Wang Guoli <andy.wangguoli@huawei.com>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: <stable@vger.kernel.org> # 2.6.34+

Is there anything specific to 2.6.34 for this patch, other than your
testing?

> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  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);

Applied to l2-mtd.git, thanks.

Brian

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

[Brian Norris]

+ 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 <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.

> >> Fix it by releasing the lock in jffs2_new_inode() on error.
> >>
> >> Signed-off-by: Wang Guoli <andy.wangguoli@huawei.com>
> >> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> >> Cc: David Woodhouse <dwmw2@infradead.org>
> >> Cc: Wang Guoli <andy.wangguoli@huawei.com>
> >> Cc: Brian Norris <computersforpeace@gmail.com>
> >> Cc: <stable@vger.kernel.org> # 2.6.34+
> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> >> ---
> >>
> >>  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

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

[Wang Nan]

On 2014/2/27 14:55, Brian Norris wrote:
> + 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 <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?

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

>>>> Fix it by releasing the lock in jffs2_new_inode() on error.
>>>>
>>>> Signed-off-by: Wang Guoli <andy.wangguoli@huawei.com>
>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>>> Cc: Wang Guoli <andy.wangguoli@huawei.com>
>>>> Cc: Brian Norris <computersforpeace@gmail.com>
>>>> Cc: <stable@vger.kernel.org> # 2.6.34+
>>>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>>> ---
>>>>
>>>>  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
> 

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

[Brian Norris]

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