linux-kernel - Re: [RFC PATCH] jbd2: fix a potential assertion failure due to improperly dirtied 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] [day] [month] [year] [list]

Message-ID: <20240809155508.taxgdkwuvsbg3i2k@quack3>
Date: Fri, 9 Aug 2024 17:55:08 +0200
From: Jan Kara <jack@...e.cz>
To: Stephen Zhang <starzhangzsd@...il.com>
Cc: Jan Kara <jack@...e.cz>, tytso@....edu, jack@...e.com,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	zhangshida@...inos.cn, Baolin Liu <liubaolin@...inos.cn>
Subject: Re: [RFC PATCH] jbd2: fix a potential assertion failure due to
 improperly dirtied buffer

On Thu 08-08-24 11:05:26, Stephen Zhang wrote:
> Jan Kara <jack@...e.cz> 于2024年8月7日周三 20:07写道：
> > So I agree with your analysis now. But still don't like adding hacks to
> > jbd2 to acommodate for this oddity of data=journal mode. Since we already
> > have ext4_block_write_begin() implementation anyway, we should be able to
> > tweak it to do the right thing for data=journal mode inodes...
> >
> > So we could replace uses of __block_write_begin() with
> > ext4_block_write_begin() and then call do_journal_get_write_access() in
> > ext4_block_write_begin() for inodes with journalled data after the buffer
> > is mapped with get_block().
> >
> > From the part:
> >                                 if (folio_test_uptodate(folio)) {
> >                                         clear_buffer_new(bh);
> >                                         set_buffer_uptodate(bh);
> >                                         mark_buffer_dirty(bh);
> >                                         continue;
> >                                 }
> >
> > we can actually remove the clear_buffer_new() and mark_buffer_dirty() bits
> > because they will be done by block_commit_write() or
> > folio_zero_new_buffers() and they are superfluous and somewhat odd here
> > anyway.
> >
> > And the call to folio_zero_new_buffers() from ext4_block_write_begin()
> > needs to call ext4_journalled_zero_new_buffers() for inodes where data is
> > journalled.
> >
> > Will you try to implement this or should I look into it?
> >
> 
> Yeah, Thank you for giving me the opportunity to work on something truly
> meaningful. All I can do until now is some small cleanups. And doing cleanups
> all the time is annoyable to the maintainers and frustrating to me. I
> will try my best.
> 
> So basically, we should:
> 1.Trace the user data dirting in ext4_block_write_begin().
> 2.Replace the uncontrollable __block_write_begin with ext4_block_write_begin().
> 3.Remove some superfluous things.

Yes. In the first patch, I'd convert all uses of __block_write_begin() to
ext4_block_write_begin(). In the second patch I'd replace
folio_zero_new_buffers() with ext4_journalled_zero_new_buffers() if inode
has journalled data (with explanation to avoid unexpected dirtying). In the
third patch I'd remove the clear_buffer_new() and mark_buffer_dirty()
mentioned above with explanation that either folio_zero_new_buffers() or
block_commit_write() take care of dirtying the buffer properly. Thanks for
working on this!

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