Re: [Devel] [PATCH hci-8.1 2/5] drivers/md/push_backup: clear pb->alive under pb->lock

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-push-backup: pb->alive KCSAN race only partly fixed, plain accesses remain
--

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

drivers/md/push_backup: clear pb->alive under pb->lock

This patch makes pb_message() take pb->lock around the store that clears
pb->alive when handling push_backup_stop. The aim is to avoid a possible
KCSAN report and to match other users of pb->alive.

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 9a6252f2b21e..1ae4ccc208ff 100644
> --- a/drivers/md/dm-push-backup.c
> +++ b/drivers/md/dm-push-backup.c
> @@ -581,7 +581,9 @@ static int pb_message(struct dm_target *ti, unsigned int argc, char **argv,
>  		if (!pb->map)
>  			return -EBADF;
>  
> +		spin_lock_irq(&pb->lock);
>  		pb->alive = false;
> +		spin_unlock_irq(&pb->lock);
>  		wake_up_interruptible(&pb->waitq); /* pb->alive = false */
>  	}

[Severity: Low]
Does this fully avoid the KCSAN report described in the commit message
("it may potentially trigger KCSAN")?

This store in pb_message() runs before ctl_rwsem is taken, so it holds
pb->lock but not ctl_rwsem. A concurrent push_backup_start() holds
ctl_rwsem but not pb->lock, so the two do not exclude each other:

push_backup_start()
    if (pb->alive)
        return -EEXIST;

That read is a plain read with no pb->lock and no READ_ONCE(), so it can
still race with the store above.

The wait_event condition used by push_backup_read() is also evaluated
without pb->lock. It reads the flag through pb_has_event():

    !READ_ONCE(pb->alive) || !READ_ONCE(pb->map_bits) || ...

Here the reader is marked, but none of the writers use WRITE_ONCE(). The
stores in pb_message() (this patch), push_backup_stop(), and the
pb->alive = true store in the push_backup_start() setup path are all
still plain stores.

KCSAN can report a plain store racing with a lockless read, so taking
pb->lock around only this one store may not remove the report.

Would it make sense to use WRITE_ONCE() on the stores to pb->alive and
READ_ONCE() on the read in push_backup_start() (or take pb->lock for that
read)? The commit message could also be adjusted if the intent is only to
make the locking consistent.

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

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