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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dm-push-backup: pb_timer_func() can re-arm timer or queue work after stop/destroy
- [Medium] dm-push-backup: pb_destroy() uses non-waiting timer_delete_sync_try() instead of timer_shutdown_sync()
- [Medium] dm-push-backup: cleanup_backup() leaves deadline_jiffies stale, so a racing timer callback can re-arm or queue_work()
--

commit f322079c9ebbcb0d0c2a4b8257ff8a52b4e79a15
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

drivers/md/push_backup: arm deadline timer under pb->lock

This patch moves the mod_timer() calls in pb_timer_func() and
postpone_if_required_for_backup() under pb->lock. It also makes the timer
callback idle when deadline_jiffies is S64_MAX. It drops the
rcu_read_lock()/synchronize_rcu() pairing between
postpone_if_required_for_backup() and push_backup_stop().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-152912

> diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
> --- 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 (deadline == S64_MAX)
> +		goto unlock; /* Nothing is delayed, let the timer idle */
>  	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);
>  }

[Severity: Medium]
Can pb_timer_func() re-arm the timer or queue work after push_backup_stop()
has returned?

The callback does not check pb->alive. It only tests deadline_jiffies
against S64_MAX. cleanup_backup() sets pb->alive = false and drains
nr_delayed to 0, but it leaves deadline_jiffies at its old finite value.
Only setup_pb() and push_backup_write() write S64_MAX.

Consider this sequence:

CPU1
pb_timer_func()
    blocks on spin_lock_irqsave(&pb->lock)

CPU0
push_backup_stop()
    cleanup_backup()
        pb->alive = false;
        (deadline_jiffies still finite)
    timer_delete_sync_try()   <- returns -1, callback is running

CPU1 then takes pb->lock, sees a finite deadline, and calls mod_timer()
or queue_work(pb->wq, &pb->worker). If push_backup_start() has already run
for a new session, the stale do_pb_work() could call cleanup_backup() and
expire that session early.

Should cleanup_backup() set pb->deadline_jiffies = S64_MAX under pb->lock,
or should pb_timer_func() check pb->alive?

> @@ -272,16 +273,13 @@ static int postpone_if_required_for_backup(struct push_backup *pb,

[ ... ]

> @@ -400,8 +398,6 @@ 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);
>  	flush_workqueue(pb->wq);

[Severity: Medium]
The commit message says "the callback re-arming itself is handled by
timer_delete_sync_try()". Is that accurate?

timer_delete_sync_try() calls __try_to_del_timer_sync(timer, false). That
returns -1 without waiting when base->running_timer == timer, and it does
not prevent re-arming. Here the -1 result only triggers WARN_ON_ONCE().

The removed synchronize_rcu() only covered
postpone_if_required_for_backup() arming the timer. It never covered the
callback re-arming itself.

So after this change, can the callback still call mod_timer() or
queue_work() after flush_workqueue() has returned? Would
timer_shutdown_sync(), or a loop that waits for the running callback,
be more appropriate here?

The same WARN_ON_ONCE(timer_delete_sync_try(&pb->deadline_timer) < 0)
pattern in pb_destroy() is not touched by this patch. It runs before
destroy_workqueue(pb->wq) and pb_put(pb).

If pb_timer_func() is running on another CPU and blocked on pb->lock, can
it call queue_work() on a destroyed workqueue? Can it also touch pb after
it has been freed? When pb_destroy() runs without a prior push_backup_stop(),
pb->alive is still true and a finite deadline can still exist. Would
timer_shutdown_sync() be the right call in pb_destroy()?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103003246.712239-1-andrey.zhadchenko@virtuozzo.com?part=5

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