linux-kernel - Re: [PATCH] dm-snapshot: Flush merged data before committing metadata

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: <mhng-81f035b4-1104-44e8-9099-4a99972512b3@palmerdabbelt-glaptop>
Date:   Wed, 13 Jan 2021 11:14:58 -0800 (PST)
From:   Palmer Dabbelt <palmer@...belt.com>
To:     kernel-team@...roid.com
CC:     Akilesh Kailash <akailash@...gle.com>,
        David Anderson <dvander@...gle.com>, kernel-team@...roid.com,
        agk@...hat.com, snitzer@...hat.com, dm-devel@...hat.com,
        linux-kernel@...r.kernel.org
Subject:     Re: [PATCH] dm-snapshot: Flush merged data before committing metadata

On Sun, 27 Dec 2020 23:14:07 PST (-0800), kernel-team@...roid.com wrote:
> If the origin device has a volatile write-back
> cache and the following events occur:
>
> 1: After finishing merge operation of one set of exceptions,
>    merge_callback() is invoked.
> 2: Update the metadata in COW device tracking the merge completion.
>    This update to COW device is flushed cleanly.
> 3: System crashes and the origin device's cache where the recent
>    merge was completed has not been flushed.
>
> During the next cycle when we read the metadata from the COW device,
> we will skip reading those metadata whose merge was completed in
> step (1). This will lead to data loss/corruption.
>
> To address this, flush the origin device post merge IO before
> updating the metadata.

This is essentially the same as the flushes added in 8b3fd1f53af3 ("dm clone:
Flush destination device before committing metadata") and 694cfe7f31db ("dm
thin: Flush data device before committing metadata"), but for dm-snap.  Looks
like this bug has been around since before the git tree, so I'm not sure what
the right thing to do with a Fixes tag is.

> Signed-off-by: Akilesh Kailash <akailash@...gle.com>
> ---
>  drivers/md/dm-snap.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
> index 4668b2cd98f4..11890db71f3f 100644
> --- a/drivers/md/dm-snap.c
> +++ b/drivers/md/dm-snap.c
> @@ -141,6 +141,11 @@ struct dm_snapshot {
>  	 * for them to be committed.
>  	 */
>  	struct bio_list bios_queued_during_merge;
> +
> +	/*
> +	 * Flush data after merge.
> +	 */
> +	struct bio flush_bio;
>  };
>
>  /*
> @@ -1121,6 +1126,17 @@ static void snapshot_merge_next_chunks(struct dm_snapshot *s)
>
>  static void error_bios(struct bio *bio);
>
> +static int flush_data(struct dm_snapshot *s)
> +{
> +	struct bio *flush_bio = &s->flush_bio;
> +
> +	bio_reset(flush_bio);
> +	bio_set_dev(flush_bio, s->origin->bdev);
> +	flush_bio->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
> +
> +	return submit_bio_wait(flush_bio);
> +}
> +
>  static void merge_callback(int read_err, unsigned long write_err, void *context)
>  {
>  	struct dm_snapshot *s = context;
> @@ -1134,6 +1150,11 @@ static void merge_callback(int read_err, unsigned long write_err, void *context)
>  		goto shut;
>  	}
>
> +	if (flush_data(s) < 0) {
> +		DMERR("Flush after merge failed: shutting down merge");
> +		goto shut;
> +	}
> +
>  	if (s->store->type->commit_merge(s->store,
>  					 s->num_merging_chunks) < 0) {
>  		DMERR("Write error in exception store: shutting down merge");
> @@ -1318,6 +1339,7 @@ static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  	s->first_merging_chunk = 0;
>  	s->num_merging_chunks = 0;
>  	bio_list_init(&s->bios_queued_during_merge);
> +	bio_init(&s->flush_bio, NULL, 0);

There's no bio_uninit() in the error handling code here, but there aren't any
in any of the other cleanup blocks either.  The bio_uninit() call won't do
anything here, as it does nothing for empty BIOs, so I guess it just doesn't
matter.

>  	/* Allocate hash table for COW data */
>  	if (init_hash_tables(s)) {
> @@ -1504,6 +1526,8 @@ static void snapshot_dtr(struct dm_target *ti)
>
>  	dm_exception_store_destroy(s->store);
>
> +	bio_uninit(&s->flush_bio);
> +
>  	dm_put_device(ti, s->cow);
>
>  	dm_put_device(ti, s->origin);

Reviewed-by: Palmer Dabbelt <palmerdabbelt@...gle.com>