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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2014/2/26 10:03, 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@xxxxxxxxxx>
>> 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.

Thank you.

>> Fix it by releasing the lock in jffs2_new_inode() on error.
>>
>> Signed-off-by: Wang Guoli <andy.wangguoli@xxxxxxxxxx>
>> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
>> Cc: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
>> Cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
>> Cc: Wang Guoli <andy.wangguoli@xxxxxxxxxx>
>> Cc: Brian Norris <computersforpeace@xxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx> # 2.6.34+
>> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>
>>  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
> 


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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]