[Devel] [PATCH hci-8.1 v6 5/7] dm/push_backup: arm deadline timer under pb->lock

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

Both pb_timer_func() and postpone_if_required_for_backup() decide
whether to arm the timer under pb->lock but call mod_timer() after
dropping it, so the decisions can be applied out of order:

  CPU1 pb_timer_func()            CPU2 postpone_if_required_for_backup()
  lock; deadline = S64_MAX
  unlock
                                  lock; nr_delayed = 1
                                  deadline_jiffies = now + timeout
                                  unlock
                                  mod_timer(timeout)
  mod_timer(from S64_MAX)

The late mod_timer() of the callback overwrites the fresh deadline, and
the write is delayed with the timer parked in a very distant future.
Only a later push_backup_write() that leaves requests delayed re-arms
it; if the backup tool stops acknowledging ranges, the backup never
expires.

Make the decision and the arming atomic: call mod_timer() under
pb->lock at both sites, and do not re-arm when nothing is delayed.
mod_timer() and queue_work() are fine under a spinlock, and the timer
base lock is dropped while the callback runs, so there is no lock
inversion.

The callback checks pb->nr_delayed rather than pb->alive on purpose.
pb_message() clears pb->alive for "push_backup_stop" before taking
pb->ctl_rwsem, and if the tool gets a fatal signal while waiting for
the rwsem, push_backup_stop() never runs. The deadline must still
expire then, so that do_pb_work() releases the delayed writes.
cleanup_backup() zeroes nr_delayed under the same lock hold that clears
alive, and postpone only delays while alive, so after stop the callback
neither re-arms nor queues work. While alive, nr_delayed == 0 is the
same as deadline_jiffies == S64_MAX.

The callback racing with push_backup_stop() was never handled either.
synchronize_rcu() waits for a callback that is already running when it
starts, since the timer softirq is an RCU reader, but not for a firing
that starts after the grace period. timer_delete_sync_try() is
try_to_del_timer_sync(), it returns -1 without waiting when the
callback is running, and we only WARN on that. Such a callback could
re-arm the timer or queue do_pb_work() after flush_workqueue(),
expiring a later session. The same happens in pb_destroy(), which
WARNs and goes on to destroy_workqueue() and kfree(pb) under the
running callback: a use-after-free. Use timer_delete_sync() in
push_backup_stop() and timer_shutdown_sync() in pb_destroy() to wait
for it; the work it may have queued is flushed by
flush_workqueue()/destroy_workqueue() which follow.

As a bonus the rcu_read_lock()/synchronize_rcu() dance between
postpone_if_required_for_backup() and push_backup_stop() is no longer
needed: the timer is only armed under pb->lock, by postpone while
pb->alive is set and by the callback while requests are delayed, both
of which cleanup_backup() ends under pb->lock, and the callback itself
is waited for. Drop it. push_backup_write() still re-arms the timer
outside pb->lock, but it is serialized with push_backup_stop() by
pb->ctl_rwsem; the next patch removes that call.

Fixes: 62e07487348e ("dm: Add push backup driver")
https://virtuozzo.atlassian.net/browse/VSTOR-152912

Feature: dm-backup: push backup target driver
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: new patch
v3: bail out of pb_timer_func() when !pb->alive; wait for a running
    callback with timer_delete_sync() in push_backup_stop() and
    timer_shutdown_sync() in pb_destroy() instead of WARNing on
    timer_delete_sync_try() < 0; fixed the commit message claim about it
v4: no changes
v5: moved before "do not re-arm deadline timer"; pb_timer_func()
    checks pb->nr_delayed instead of pb->alive, so an interrupted
    "push_backup_stop" still expires the deadline; commit message
    describes the race instead of the harmless parked timer, the
    synchronize_rcu() window precisely and the pb_destroy()
    use-after-free; added Fixes: tag
v6: subject prefix changed to dm/push_backup, added Feature: tag

 drivers/md/dm-push-backup.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
index b6d1629245235..f0de26add11c2 100644
--- a/drivers/md/dm-push-backup.c
+++ b/drivers/md/dm-push-backup.c
@@ -241,23 +241,24 @@ static void pb_timer_func(struct timer_list *timer)
 
 	spin_lock_irqsave(&pb->lock, flags);
 	deadline = pb->deadline_jiffies;
-	spin_unlock_irqrestore(&pb->lock, flags);
-
+	if (!pb->nr_delayed)
+		goto unlock;
 	if (unlikely(time_before64(now, deadline)))
 		mod_timer(timer, jiffies + (deadline - now) + 1);
 	else
 		queue_work(pb->wq, &pb->worker);
+unlock:
+	spin_unlock_irqrestore(&pb->lock, flags);
 }
 
 static int postpone_if_required_for_backup(struct push_backup *pb,
 					   struct request *rq)
 {
-	bool queue_timer = false;
 	struct pb_bio *pbio;
 	unsigned long flags;
+	bool wake = false;
 	int ret = 0;
 
-	rcu_read_lock(); /* See push_backup_stop() */
 	spin_lock_irqsave(&pb->lock, flags);
 	if (!pb->alive)
 		goto unlock;
@@ -272,16 +273,13 @@ static int postpone_if_required_for_backup(struct push_backup *pb,
 	pb->nr_delayed += 1;
 	if (pb->nr_delayed == 1) {
 		pb->deadline_jiffies = get_jiffies_64() + pb->timeout_in_jiffies;
-		queue_timer = true;
+		mod_timer(&pb->deadline_timer, jiffies + pb->timeout_in_jiffies + 1);
+		wake = true;
 	}
 unlock:
 	spin_unlock_irqrestore(&pb->lock, flags);
 
-	if (queue_timer)
-		mod_timer(&pb->deadline_timer, jiffies + pb->timeout_in_jiffies + 1);
-	rcu_read_unlock();
-
-	if (queue_timer)
+	if (wake)
 		wake_up_interruptible(&pb->waitq);
 
 	return ret;
@@ -400,9 +398,7 @@ static int push_backup_stop(struct push_backup *pb,
 		return -EBADF;
 	cleanup_backup(pb);
 
-	/* Wait postpone_if_required_for_backup() starts timer */
-	synchronize_rcu();
-	WARN_ON_ONCE(timer_delete_sync_try(&pb->deadline_timer) < 0);
+	timer_delete_sync(&pb->deadline_timer);
 	flush_workqueue(pb->wq);
 
 	spin_lock_irq(&pb->lock);
@@ -632,7 +628,7 @@ static void pb_destroy(struct push_backup *pb)
 {
 	WARN_ON_ONCE(pb->nr_delayed);
 
-	WARN_ON_ONCE(timer_delete_sync_try(&pb->deadline_timer) < 0);
+	timer_shutdown_sync(&pb->deadline_timer);
 	if (pb->wq)
 		destroy_workqueue(pb->wq);
 	kvfree(pb->map); /* Is's not zero if stop was not called */
-- 
2.43.5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel