Re: [Devel] [PATCH hci-8.1 3/5] drivers/md/push_backup: do not re-arm deadline timer in push_backup_write()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-push-backup: stale-deadline re-arm in pb_timer_func() can leave timer far in future
--

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

drivers/md/push_backup: do not re-arm deadline timer in push_backup_write()

This patch removes the mod_timer() call from push_backup_write(). The
commit message says pb_timer_func() re-checks pb->deadline_jiffies each
time it fires, so re-arming after updating the field is unnecessary. It
also says this prepares for making the write path reachable without
pb->ctl_rwsem.

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

> diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
> index 1ae4ccc208ff..8ce84a1a696e 100644
> --- a/drivers/md/dm-push-backup.c
> +++ b/drivers/md/dm-push-backup.c

[ ... ]

> @@ -519,8 +519,7 @@ static int push_backup_write(struct push_backup *pb,
>  		}
>  	}
>  
> -	has_more = (pb->nr_delayed != 0);
> -	if (has_more)
> +	if (pb->nr_delayed)
>  		pb->deadline_jiffies = get_jiffies_64() + pb->timeout_in_jiffies;
>  	else
>  		pb->deadline_jiffies = S64_MAX;

[ ... ]

> -	if (!list_empty(&pbio_list)) {
> +	if (!list_empty(&pbio_list))
>  		resubmit_pbios(pb, &pbio_list);
> -		if (has_more)
> -			mod_timer(&pb->deadline_timer, pb->timeout_in_jiffies + 1);
> -	}

[Severity: Medium]
Can the deadline timer be left armed far in the future after this change?

The commit message relies on pb_timer_func() re-checking the deadline, but
that callback samples the deadline under pb->lock and then calls mod_timer()
after dropping the lock:

drivers/md/dm-push-backup.c:pb_timer_func() {
    ...
	spin_lock_irqsave(&pb->lock, flags);
	deadline = pb->deadline_jiffies;
	spin_unlock_irqrestore(&pb->lock, flags);

	if (unlikely(time_before64(now, deadline)))
		mod_timer(timer, deadline - now + 1);
    ...
}

Consider this sequence:

1. push_backup_write() completes the last delayed pbio and sets
   pb->deadline_jiffies = S64_MAX, while the timer is still armed for the
   old expiry.

2. The timer fires. pb_timer_func() reads deadline = S64_MAX under
   pb->lock and drops the lock.

3. On another CPU, postpone_if_required_for_backup() takes pb->lock, moves
   pb->nr_delayed from 0 to 1, and sets a fresh deadline_jiffies. It then
   calls mod_timer(&pb->deadline_timer, ...) after dropping the lock, so
   the timer is armed for the short timeout.

4. pb_timer_func() then runs its stale mod_timer(timer, deadline - now + 1)
   with deadline = S64_MAX. This overwrites the short expiry with a
   far-future one.

After step 4, the only arming site left is the pb->nr_delayed == 1
transition in postpone_if_required_for_backup(). push_backup_write() no
longer re-arms the timer, so nothing corrects the expiry.

pb->nr_delayed stays above zero with the timer far in the future. Delayed
requests would then not be released by the backup timeout if userspace
stops servicing the backup.

The window is narrow. It spans the gap between spin_unlock_irqrestore() and
mod_timer() in pb_timer_func(). Before this patch, a later
push_backup_write() with pending pbios could re-arm the timer and recover.

Could the re-arm in pb_timer_func() be done in a way that cannot overwrite a
newer expiry? One option is to re-read or re-arm the deadline under
pb->lock. Another is to have the arming sites in
postpone_if_required_for_backup() and pb_timer_func() order their updates
consistently.

Later patches in this series do not appear to close this window. The patch
described as fixing mod_timer() usage only changes the expiry arithmetic.
The event file descriptor patch does not change this path.

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

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