[Ocfs2-devel] [PATCH RESEND] ocfs2: fix a misuse a of brelse after failing ocfs2_check_dir_entry

[piaojun]

Hi Changwei,

Sorry for the oversight for your patch. There is nothing wrong.

Thanks,
Jun

On 2018/11/10 15:58, Changwei Ge wrote:
> Hi Jun,
> 
> Thanks for looking into this, my code from Linux mainline is not looked like yours...
> 
> On 2018/11/10 14:36, piaojun wrote:
>> Hi Changwei,
>>
>> On 2018/5/28 22:40, Changwei Ge wrote:
>>> From: Changwei Ge <ge.changwei at h3c.com>
>>>
>>> Somehow, file system metadata was corrupted, which causes
>>> ocfs2_check_dir_entry() to fail in function ocfs2_dir_foreach_blk_el().
>>>
>>> According to the original design intention, if above happens we should
>>> skip the problematic block and continue to retrieve dir entry. But there
>>> is obviouse misuse of brelse around related code.
>>>
>>> After failure of ocfs2_check_dir_entry(), currunt code just moves to next
>>> position and uses the problematic buffer head again and again during
>>> which the problematic buffer head is released for multiple times. I
>>> suppose, this a serious issue which is long-lived in ocfs2. This may
>>> cause other file systems which is also used in a the same host insane.
>>>
>>> So we should also consider about bakcporting this patch into
>>> linux -stable.
>>>
>>> Suggested-by: Changkuo Shi <shi.changkuo at h3c.com>
>>> Cc: stable at vger.kernel.org
>>> Signed-off-by: Changwei Ge <ge.changwei at h3c.com>
>>> ---
>>>   fs/ocfs2/dir.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
>>> index b048d4f..c121abb 100644
>>> --- a/fs/ocfs2/dir.c
>>> +++ b/fs/ocfs2/dir.c
>>> @@ -1897,8 +1897,7 @@ static int ocfs2_dir_foreach_blk_el(struct inode *inode,
>>>   				/* On error, skip the f_pos to the
>>>   				   next block. */
>>>   				ctx->pos = (ctx->pos | (sb->s_blocksize - 1)) + 1;
>>> -				brelse(bh);
>>> -				continue;
>>> +				break;
>>
>> I guess return is more appropriate than break here as it will cause double
>> free buffer:
>>
>> "
>> ctx->pos = (ctx->pos | (sb->s_blocksize - 1)) + 1;
>> brelse(bh);
>> break;
>> "
> 
> Linux mainline should not have 'brelse(bh)', could your please rebase your tree?
> 
> '''
> ccd979bdb (Mark Fasheh         2005-12-15 14:31:24 -0800 1897)                          /* On error, skip the f_pos to the
> ccd979bdb (Mark Fasheh         2005-12-15 14:31:24 -0800 1898)                             next block. */
> 3704412bd (Al Viro             2013-05-22 21:06:00 -0400 1899)                          ctx->pos = (ctx->pos | (sb->s_blocksize - 1)) + 1;
> 29aa30167 (Changwei Ge         2018-11-02 15:48:15 -0700 1900)                          break;
> '''
>>
>> "
>> brelse(bh);
>> bh = NULL;
>> if (!persist && stored)
>> 	break;
>> "
>>
>> Thanks,
>> Jun
>>
>>>   			}
>>>   			if (le64_to_cpu(de->inode)) {
>>>   				unsigned char d_type = DT_UNKNOWN;
>>>
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>
> .
>