From mboxrd@z Thu Jan 1 00:00:00 1970 From: piaojun Date: Sat, 10 Nov 2018 14:33:56 +0800 Subject: [Ocfs2-devel] [PATCH RESEND] ocfs2: fix a misuse a of brelse after failing ocfs2_check_dir_entry In-Reply-To: References: Message-ID: <5BE67BD4.2020703@huawei.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Changwei Ge , Andrew Morton , "ocfs2-devel@oss.oracle.com" , "junxiao.bi@oracle.com" , "jiangqi903@gmail.com" Cc: "stable@vger.kernel.org" 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; " " brelse(bh); bh = NULL; if (!persist && stored) break; " Thanks, Jun > } > if (le64_to_cpu(de->inode)) { > unsigned char d_type = DT_UNKNOWN; > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from szxga06-in.huawei.com ([45.249.212.32]:54149 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728567AbeKJQSK (ORCPT ); Sat, 10 Nov 2018 11:18:10 -0500 Subject: Re: [Ocfs2-devel] [PATCH RESEND] ocfs2: fix a misuse a of brelse after failing ocfs2_check_dir_entry To: Changwei Ge , Andrew Morton , "ocfs2-devel@oss.oracle.com" , "junxiao.bi@oracle.com" , "jiangqi903@gmail.com" References: CC: "stable@vger.kernel.org" From: piaojun Message-ID: <5BE67BD4.2020703@huawei.com> Date: Sat, 10 Nov 2018 14:33:56 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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; " " brelse(bh); bh = NULL; if (!persist && stored) break; " Thanks, Jun > } > if (le64_to_cpu(de->inode)) { > unsigned char d_type = DT_UNKNOWN; >