From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from szxga05-in.huawei.com ([45.249.212.191]:14646 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728659AbeKJRtS (ORCPT ); Sat, 10 Nov 2018 12:49:18 -0500 Subject: Re: [Ocfs2-devel] [PATCH RESEND] ocfs2: fix a misuse a of brelse after failing ocfs2_check_dir_entry To: Changwei Ge , Changwei Ge , Andrew Morton , "ocfs2-devel@oss.oracle.com" , "junxiao.bi@oracle.com" , "jiangqi903@gmail.com" References: <5BE67BD4.2020703@huawei.com> <63ADC13FD55D6546B7DECE290D39E37301277E2772@H3CMLB12-EX.srv.huawei-3com.com> CC: "stable@vger.kernel.org" From: piaojun Message-ID: <5BE6910F.1090304@huawei.com> Date: Sat, 10 Nov 2018 16:04:31 +0800 MIME-Version: 1.0 In-Reply-To: <63ADC13FD55D6546B7DECE290D39E37301277E2772@H3CMLB12-EX.srv.huawei-3com.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 >>> >>> 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 >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Changwei Ge >>> --- >>> 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@oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >> > . > From mboxrd@z Thu Jan 1 00:00:00 1970 From: piaojun Date: Sat, 10 Nov 2018 16:04:31 +0800 Subject: [Ocfs2-devel] [PATCH RESEND] ocfs2: fix a misuse a of brelse after failing ocfs2_check_dir_entry In-Reply-To: <63ADC13FD55D6546B7DECE290D39E37301277E2772@H3CMLB12-EX.srv.huawei-3com.com> References: <5BE67BD4.2020703@huawei.com> <63ADC13FD55D6546B7DECE290D39E37301277E2772@H3CMLB12-EX.srv.huawei-3com.com> Message-ID: <5BE6910F.1090304@huawei.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Changwei Ge , Changwei Ge , Andrew Morton , "ocfs2-devel@oss.oracle.com" , "junxiao.bi@oracle.com" , "jiangqi903@gmail.com" Cc: "stable@vger.kernel.org" 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 >>> >>> 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 >>> Cc: stable at vger.kernel.org >>> Signed-off-by: Changwei Ge >>> --- >>> 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 >> > . >