ANDROID: dm-bow: serialize state changes with completion of writes

dm-bow has a long-standing race condition. When in the CHECKPOINT state,
writes are completed asynchronously on a workqueue, rather than
occurring synchronously within dm_bow_map() as they are when in the
COMMITTED state. There is no synchronization between the completion of
these writes and the state change, leading to potential corruption.

Specifically, problems occur during the transition from CHECKPOINT to
COMMITTED. When state_store() triggers this state change, it sets an
atomic variable that causes all subsequent writes to follow the
synchronous write path used by the COMMITTED state. Any already
in-flight writes on the workqueue complete asynchronously, however, and
as if still in CHECKPOINT. When bow_write() runs for these writes, any
blocks that were considered free while in CHECKPOINT are considered safe
to write to. Some of those blocks, however, may have already been
recently written to via the synchronous COMMITTED path.

When that happens, those recently-written post-commit blocks will be
overwritten by the pre-commit write. This results in both corruption of
the blocks written after the state transitioned to COMMITTED as well as
the loss of the writes that were still in-flight during the state
transition.

Fix this by adding the missing synchronization. Acquire a mutex when
changing the state and, with this held, flush the workqueue to ensure
that all writes prior to the state change have complete. Use this same
lock to ensure no new writes will be performed/queued until the state
change is complete. This mutex adds negligible overhead since the only
hot path it is acquired on is one that's already serialized via the
existing ranges_lock.

Fixes: 0ce3eb37e9 ("ANDROID: dm-bow: Add dm-bow feature")
Change-Id: I5e7e11d6a43ab7eb9fe95b8c4065def564608fcd
Signed-off-by: Matt Wagantall <mattnw@meta.com>

drivers/md/dm-bow.c

@@ -87,6 +87,7 @@ struct bow_context {
 	struct mutex ranges_lock; /* Hold to access this struct and/or ranges */
 	struct rb_root ranges;
 	struct dm_kobject_holder kobj_holder;	/* for sysfs attributes */
+	struct mutex state_lock;
 	atomic_t state; /* One of the enum state values above */
 	u64 trims_total;
 	struct log_sector *log_sector;
@@ -526,6 +527,12 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 		return -EINVAL;
 	}
 
+	/* Block new writes until state change is complete. */
+	mutex_lock(&bc->state_lock);
+
+	/* Flush any already-queued writes before the state change. */
+	flush_workqueue(bc->workqueue);
+
 	mutex_lock(&bc->ranges_lock);
 	original_state = atomic_read(&bc->state);
 	if (state != original_state + 1) {
@@ -561,6 +568,8 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 bad:
 	mutex_unlock(&bc->ranges_lock);
+	mutex_unlock(&bc->state_lock);
+
 	return ret;
 }
 
@@ -736,6 +745,7 @@ static int dm_bow_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	}
 
 	init_completion(&bc->kobj_holder.completion);
+	mutex_init(&bc->state_lock);
 	mutex_init(&bc->ranges_lock);
 	bc->ranges = RB_ROOT;
 	bc->bufio = dm_bufio_client_create(bc->dev->bdev, bc->block_size, 1, 0,
@@ -1121,6 +1131,8 @@ static int dm_bow_map(struct dm_target *ti, struct bio *bio)
 	int ret = DM_MAPIO_REMAPPED;
 	struct bow_context *bc = ti->private;
 
+	/* Fast path when already committed or when performing a read. */
+
 	if (likely(bc->state.counter == COMMITTED))
 		return remap_unless_illegal_trim(bc, bio);
 
@@ -1130,6 +1142,13 @@ static int dm_bow_map(struct dm_target *ti, struct bio *bio)
 	if (bio->bi_iter.bi_size == 0)
 		return remap_unless_illegal_trim(bc, bio);
 
+	/*
+	 * Fall back to the slower path when we may be in TRIM/CHECKPOINT.
+	 * Operations must wait for any pending state changes to complete.
+	 */
+
+	mutex_lock(&bc->state_lock);
+
 	if (atomic_read(&bc->state) != COMMITTED) {
 		enum state state;
 
@@ -1152,6 +1171,8 @@ static int dm_bow_map(struct dm_target *ti, struct bio *bio)
 		mutex_unlock(&bc->ranges_lock);
 	}
 
+	mutex_unlock(&bc->state_lock);
+
 	if (ret == DM_MAPIO_REMAPPED)
 		return remap_unless_illegal_trim(bc, bio);