linux-ext4 - Re: [PATCH 2/2] jbd2: do not clear the BH_Mapped flag when forgetting a metadata buffer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 12 Feb 2020 09:45:11 +0100
From:   Jan Kara <jack@...e.cz>
To:     "zhangyi (F)" <yi.zhang@...wei.com>
Cc:     Jan Kara <jack@...e.cz>, tytso@....edu, linux-ext4@...r.kernel.org,
        luoshijie1@...wei.com, zhangxiaoxu5@...wei.com
Subject: Re: [PATCH 2/2] jbd2: do not clear the BH_Mapped flag when
 forgetting a metadata buffer

On Thu 06-02-20 23:28:01, zhangyi (F) wrote:
> Thanks for the comments.
> 
> On 2020/2/6 19:46, Jan Kara wrote:
> > On Mon 03-02-20 22:04:58, zhangyi (F) wrote:
> [..]
> >> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> >> index 6396fe70085b..a649cdd1c5e5 100644
> >> --- a/fs/jbd2/commit.c
> >> +++ b/fs/jbd2/commit.c
> >> @@ -987,10 +987,13 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> >>  		if (buffer_freed(bh) && !jh->b_next_transaction) {
> >>  			clear_buffer_freed(bh);
> >>  			clear_buffer_jbddirty(bh);
> >> -			clear_buffer_mapped(bh);
> >> -			clear_buffer_new(bh);
> >> -			clear_buffer_req(bh);
> >> -			bh->b_bdev = NULL;
> >> +			if (buffer_unmap(bh)) {
> >> +				clear_buffer_unmap(bh);
> >> +				clear_buffer_mapped(bh);
> >> +				clear_buffer_new(bh);
> >> +				clear_buffer_req(bh);
> >> +				bh->b_bdev = NULL;
> >> +			}
> > 
> > Any reason why you don't want to clear buffer_req and buffer_new flags for
> > all buffers as well? I agree that b_bdev setting and buffer_mapped need
> > special treatment.
> > 
> IIUC, for the buffer coming from jbd2_journal_forget() is always 'block
> device backed' metadata buffer (not pretty sure), and for these metadata
  Yes, it is.

> buffer, buffer_new flag will not be set. At the same time, since it's
> always mapped, so it's fine to keep the buffer_req flag even it's freed
> by the filesystem now, because it means the block device has committed
> this buffer, and it seems that it does not affect we reuse this buffer.
> Am I missing something ?

OK, you're right that buffer_new shouldn't be ever set for block backed
buffers and we don't care about buffer_req. So let's keep the split of bits
to clear as you did and just add a comment that for block device buffers it
is enough to clear buffer_jbddirty and buffer_freed, for file mapping
buffers (i.e., journalled data) we have to be more careful and clear more
bits.

								Honza

-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR