linux-kernel - Re: [Ocfs2-devel] [PATCH v2] ocfs2: Fix handle refcount leak in two exception handling paths

Open Source and information security mailing list archives

Date:   Wed, 8 Sep 2021 17:12:58 +0000
From:   Wengang Wang <wen.gang.wang@...cle.com>
To:     Joseph Qi <joseph.qi@...ux.alibaba.com>
CC:     Chenyuan Mi <cymi20@...an.edu.cn>,
        akpm <akpm@...ux-foundation.org>, Xin Tan <tanxin.ctf@...il.com>,
        Xiyu Yang <xiyuyang19@...an.edu.cn>,
        "yuanxzhang@...an.edu.cn" <yuanxzhang@...an.edu.cn>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ocfs2-devel@....oracle.com" <ocfs2-devel@....oracle.com>
Subject: Re: [Ocfs2-devel] [PATCH v2] ocfs2: Fix handle refcount leak in two
 exception handling paths

Hi,

Sorry for late involving, but this doesn’t look right to me.

> On Sep 8, 2021, at 3:51 AM, Joseph Qi <joseph.qi@...ux.alibaba.com> wrote:
> 
> 
> 
> On 9/8/21 6:20 PM, Chenyuan Mi wrote:
>> The reference counting issue happens in two exception handling paths
>> of ocfs2_replay_truncate_records(). When executing these two exception
>> handling paths, the function forgets to decrease the refcount of handle
>> increased by ocfs2_start_trans(), causing a refcount leak.
>> 
>> Fix this issue by using ocfs2_commit_trans() to decrease the refcount
>> of handle in two handling paths.
>> 
>> Signed-off-by: Chenyuan Mi <cymi20@...an.edu.cn>
>> Signed-off-by: Xiyu Yang <xiyuyang19@...an.edu.cn>
>> Signed-off-by: Xin Tan <tanxin.ctf@...il.com>
> 
> Reviewed-by: Joseph Qi <joseph.qi@...ux.alibaba.com>
>> ---
>> fs/ocfs2/alloc.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
>> index f1cc8258d34a..b05fde7edc3a 100644
>> --- a/fs/ocfs2/alloc.c
>> +++ b/fs/ocfs2/alloc.c
>> @@ -5940,6 +5940,7 @@ static int ocfs2_replay_truncate_records(struct ocfs2_super *osb,
>> 		status = ocfs2_journal_access_di(handle, INODE_CACHE(tl_inode), tl_bh,
>> 						 OCFS2_JOURNAL_ACCESS_WRITE);
>> 		if (status < 0) {
>> +			ocfs2_commit_trans(osb, handle);
>> 			mlog_errno(status);
>> 			goto bail;
>> 		}
>> @@ -5964,6 +5965,7 @@ static int ocfs2_replay_truncate_records(struct ocfs2_super *osb,
>> 						     data_alloc_bh, start_blk,
>> 						     num_clusters);
>> 			if (status < 0) {
>> +				ocfs2_commit_trans(osb, handle);

As a transaction, stuff expected to be in the same handle should be treated as atomic.
Here the stuff includes the tl_bh and other metadata block which will be modified in ocfs2_free_clusters().
Coming here, some of related meta blocks may be in the handle but others are not due to the error happened.
If you do a commit, partial meta blocks are committed to log. — that breaks the atomic idea, it will cause FS inconsistency.
So what’s reason you want to commit the meta block changes, which is not all of expected, in this handle to journal log?

Do you really see a hit on the failure? or just you detected the refcount leak by code review?

You may want to look at ocfs2_journal_dirty() for the error handling part.

thanks,
wengang

>> 				mlog_errno(status);
>> 				goto bail;
>> 			}
>> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@....oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives