Re: [PATCH] dm-integrity: initialize journal accounting in dm_integrity_ctr

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] dm-integrity: NULL pointer dereference in dm_integrity_resume()
  [1]
--

commit 8723480dc905caec3ed05a2dfe72dc8d6472ba26
Author: Benjamin Marzinski <bmarzins@redhat.com>

dm-integrity: initialize journal accounting in dm_integrity_ctr

This commit initializes ic->free_sectors to ic->journal_entries in
dm_integrity_ctr() so that journal accounting is valid if the first resume
enters a failed state due to a changed superblock and skips journal replay.

> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 802b377613b06..bb2ec9bad6f0d 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -5410,6 +5410,12 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned int argc, char **argv
>  		if (r)
>  			goto bad;
>  
> +		/*
> +		 * initialize journal accounting like a cleared journal, to
> +		 * handle a changed superblock on resume, where the journal
> +		 * is not replayed.
> +		 */
> +		ic->free_sectors = ic->journal_entries;
>  	}
>  
>  	if (ic->mode == 'B') {

[Severity: High]
This problem wasn't introduced by this patch, but can modifying the
on-disk superblock while suspended also cause dm_integrity_resume() to
dereference a NULL pointer [1]?

In dm_integrity_ctr(), ic->recalc_wq is only allocated when
ic->internal_hash is set, and ic->journal is only allocated when ic->mode
is not 'R' or 'I'.  However, dm_integrity_resume() rereads the superblock
from disk and checks SB_FLAG_DIRTY_BITMAP and SB_FLAG_RECALCULATING
without verifying that those resources were allocated at constructor time:

drivers/md/dm-integrity.c:dm_integrity_resume() {
	...
	if (ic->sb->flags & cpu_to_le32(SB_FLAG_DIRTY_BITMAP)) {
		DEBUG_print("resume dirty_bitmap\n");
		rw_journal_sectors(ic, REQ_OP_READ, 0,
				   ic->n_bitmap_blocks * (BITMAP_BLOCK_SIZE >> SECTOR_SHIFT), NULL);
	...
	if (ic->sb->flags & cpu_to_le32(SB_FLAG_RECALCULATING)) {
		__u64 recalc_pos = le64_to_cpu(ic->sb->recalc_sector);

		DEBUG_print("recalc pos: %llx / %llx\n", recalc_pos, ic->provided_data_sectors);
		if (recalc_pos < ic->provided_data_sectors) {
			queue_work(ic->recalc_wq, &ic->recalc_work);
	...
}

If the superblock is modified while suspended to set SB_FLAG_RECALCULATING
on a target without an internal hash, queue_work() dereferences a NULL
ic->recalc_wq.  Likewise, setting SB_FLAG_DIRTY_BITMAP on an inline-mode
target causes rw_journal_sectors() to index into a NULL ic->journal.

[1] https://sashiko.dev/#/bug/linux-824a598f-b950-4742-b4b6-ae6b1159c25b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111004905.1047753-1-bmarzins@redhat.com?part=1