Re: [Devel] [PATCH hci-8.1 v6 7/7] dm/push_backup: add event file descriptor interface

[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_fd_read() returns -ERESTARTSYS but UAPI header promises EINTR
--

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

dm/push_backup: add event file descriptor interface

This patch adds a push_backup_get_fd target message that returns an anon
inode fd. The fd supports read() for the next range to back up and write()
to acknowledge a range. It also adds a kref on struct push_backup so the
object lives until the last fd is released.

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

[ ... ]

> +static int pb_wait_range(struct push_backup *pb, u64 *clu, u64 *nr)
> +{
> +	int ret;
> +
> +	while (1) {
> +		ret = pb_next_range(pb, clu, nr);
> +		if (ret != -EAGAIN)
> +			return ret;
> +		if (wait_event_interruptible(pb->waitq, pb_has_event(pb)))
> +			return -ERESTARTSYS;
> +	}
> +}

[ ... ]

> +static ssize_t pb_fd_read(struct file *file, char __user *buf,
> +			  size_t count, loff_t *ppos)
> +{
> +	struct push_backup *pb = file->private_data;
> +	struct dm_push_backup_range range;
> +	int ret;
> +
> +	if (count != sizeof(range))
> +		return -EINVAL;
> +
> +	ret = pb_wait_range(pb, &range.clu, &range.nr);
> +	if (ret <= 0)
> +		return ret;

[Severity: Low]
Is the EINTR documented for read() actually what userspace will see here?

pb_wait_range() returns -ERESTARTSYS when wait_event_interruptible() is
interrupted, and pb_fd_read() passes it through unchanged via the
"ret <= 0" return. The legacy push_backup_read() maps -ERESTARTSYS to
-EINTR, but the fd path does not.

If the signal handler has SA_RESTART, or no handler runs, the kernel
restarts read() transparently and userspace never sees EINTR. Without
SA_RESTART it is converted to EINTR.

> diff --git a/include/uapi/linux/dm-push-backup.h b/include/uapi/linux/dm-push-backup.h
> new file mode 100644
> --- /dev/null
> +++ b/include/uapi/linux/dm-push-backup.h

[ ... ]

> + *   EINTR         - interrupted by a signal while waiting.

The header documents EINTR unconditionally, so the documented behaviour
depends on signal disposition. Should the header describe the restart
behaviour, or should pb_fd_read() map -ERESTARTSYS to -EINTR as
push_backup_read() does?

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

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