[RFC,v4,0/8] ext4, jbd2: fix 3 issues about bdev_try_to_free_page()

Message

Zhang Yi June 10, 2021, 11:24 a.m. UTC

This patchset fix a potential filesystem inconsistency problem and two
use-after-free problems about bdev_try_to_free_page().

Patch 1-4: Fix a potential filesystem inconsistency problem caused by
           freeing buffers and doing umount concurrently, and also do
           some cleanup.
Patch 5-8: Add a shrinker to release journal_head of checkpoint buffers
           and remove the buggy bdev_try_to_free_page() at all.

Changes since v3:
 - Patch 2: Fix one spelling mistake.
 - Patch 3: Drop unnecessary 'result' check codes.
 - Patch 5, declare static for jbd2_journal_shrink_[scan|count](),
   and use percpu_counter_read_positive() to calculate the number of
   shrinkable journal heads.
 - Add 'Reviewed-by' tag from Jan besides the fifth patch.


Hi Jan,
 
I modify the fifth patch as Dave suggested, please give a look at this
patch again.

Thanks,
Yi.

------------------

Changes since v2:
 - Fix some comments and spelling mistakes on patch 2 and 3.
 - Give up the solution of add refcount on super_block and fix the
   use-after-free issue in bdev_try_to_free_page(), switch to introduce
   a shrinker to free checkpoint buffers' journal_head and remove the
   whole callback at all.

Changes since v1:
 - Do not use j_checkpoint_mutex to fix the filesystem inconsistency
   problem, introduce a new mark instead.
 - Fix superblock use-after-free issue in blkdev_releasepage().
 - Avoid race between bdev_try_to_free_page() and ext4_put_super().


Zhang Yi (8):
  jbd2: remove the out label in __jbd2_journal_remove_checkpoint()
  jbd2: ensure abort the journal if detect IO error when writing
    original buffer back
  jbd2: don't abort the journal when freeing buffers
  jbd2: remove redundant buffer io error checks
  jbd2,ext4: add a shrinker to release checkpointed buffers
  jbd2: simplify journal_clean_one_cp_list()
  ext4: remove bdev_try_to_free_page() callback
  fs: remove bdev_try_to_free_page callback

 fs/block_dev.c              |  15 ---
 fs/ext4/super.c             |  29 ++---
 fs/jbd2/checkpoint.c        | 206 +++++++++++++++++++++++++++++-------
 fs/jbd2/journal.c           | 101 ++++++++++++++++++
 fs/jbd2/transaction.c       |  17 ---
 include/linux/fs.h          |   1 -
 include/linux/jbd2.h        |  37 +++++++
 include/trace/events/jbd2.h | 101 ++++++++++++++++++
 8 files changed, 414 insertions(+), 93 deletions(-)

Comments

Christoph Hellwig June 17, 2021, 9:48 a.m. UTC | #1

Looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

Christoph Hellwig June 21, 2021, 3:25 p.m. UTC | #2

On Thu, Jun 17, 2021 at 10:48:49AM +0100, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <[e-mail]>

Just curious: does anyone plan to pick this up for 5.14?  Getting rid
of the layering violation on the block device mapping would really help
with some changes I am working on.

Jan Kara June 22, 2021, 8:24 a.m. UTC | #3

On Mon 21-06-21 16:25:13, Christoph Hellwig wrote:
> On Thu, Jun 17, 2021 at 10:48:49AM +0100, Christoph Hellwig wrote:
> > Looks good:
> > 
> > Reviewed-by: Christoph Hellwig <[e-mail]>
> 
> Just curious: does anyone plan to pick this up for 5.14?  Getting rid
> of the layering violation on the block device mapping would really help
> with some changes I am working on.

AFAIK Ted was planning on picking this up for the merge window...

								Honza