Re: [Devel] [PATCH hci-8.1 v4 5/6] drivers/md/push_backup: arm deadline timer under pb->lock

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 11/3/26 11:03, Andrey Zhadchenko wrote:
> When push_backup_write acks last delayed request it sets
> deadline_jiffies = S64_MAX and the next fired timer uses this
> for mod_timer(): expiry is set to some very distant future.
>
> Previously a later push_backup_write() would accidentally
> repair the expiry; since it no longer touches the timer, nothing
> does until stop.

Hi Andrey,

When the callback parks the timer this way, nothing is delayed, and the
next delayed write takes the nr_delayed == 1 branch in
postpone_if_required_for_backup(), which arms the timer itself. So this
case is harmless and "nothing does until stop" does not hold. The real
bug is the race item 5 of your cover letter describes: the callback
reads S64_MAX under the lock, postpone arms a fresh deadline, and the
late mod_timer() of the callback overwrites it while writes are
delayed. Could these two paragraphs describe that instead?

> A callback blocked on pb->lock could thus outlive stop and re-arm the
> timer or queue do_pb_work() after flush_workqueue(), expiring a later
> session.

A small precision: synchronize_rcu() already waited for a callback that
was running, or spinning on pb->lock, when it started, because the
timer softirq is an RCU reader. What escaped is a firing that starts
after the grace period.

> @@ -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->alive || deadline == S64_MAX)
> +		goto unlock;

pb->alive is not cleared by cleanup_backup() only. pb_message() clears
it for "push_backup_stop" before it takes ctl_rwsem, on purpose since
6cd7e75f682f ("dm/push_backup: Allow to stop push_backup at any
time"):

	spin_lock_irq(&pb->lock);
	pb->alive = false;
	spin_unlock_irq(&pb->lock);
	...
	ret = down_write_killable(&pb->ctl_rwsem);
	...
	if (unlikely(ret))
		goto out;

If the stopping tool gets a fatal signal while the rwsem is held by
another message, push_backup_stop() never runs. alive is false,
nr_delayed is non-zero and the deadline is real. Before this patch the
timer still expired it and do_pb_work() released the guest writes.
With this hunk the callback returns and does not re-arm, read() and
write() on the fd return -ESTALE, and the writes hang until somebody
sends "push_backup_stop" again. dm suspend, "@stats_create" and remove
wait for them meanwhile.

Checking nr_delayed instead keeps what the patch relies on:

	if (!pb->nr_delayed)
		goto unlock;

cleanup_backup() zeroes nr_delayed under the same lock hold that clears
alive, and postpone only delays while alive. So after stop the callback
still neither re-arms nor queues work, and in the state above it expires
the deadline as before. While alive, deadline == S64_MAX is the same as
nr_delayed == 0.

> -	WARN_ON_ONCE(timer_delete_sync_try(&pb->deadline_timer) < 0);
> +	timer_shutdown_sync(&pb->deadline_timer);

This also fixes a use-after-free: removing the target while the
callback ran made pb_destroy() WARN and go on to destroy_workqueue()
and kfree(pb) under it. Both that and the stop race came with the
driver, so please add:

Fixes: 62e07487348e ("dm: Add push backup driver")

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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