[Devel] [PATCH hci-8.1 v4 6/6] drivers/md/push_backup: add event file descriptor interface

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] dm-push-backup: pb_fops has no .poll, so the event fd cannot be polled
- [Low] dm-push-backup: push_backup_write() checks range before -ENODEV/-ESTALE
- [Low] dm push_backup: fd_install() in message handler precedes ioctl result copy-out
--

commit 34e71a14b728d9c4ab310c5d9fc752f38ad72889
Author: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>

drivers/md/push_backup: add event file descriptor interface

This patch adds a push_backup_get_fd target message that returns an anon
inode fd. The backup tool can read() the next range to back up from the fd
and write() to acknowledge it, instead of sleeping inside a dm message. A
kref on struct push_backup ties the lifetime to the number of open fds.

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

[ ... ]

> @@ -471,11 +528,13 @@ static int push_backup_write(struct push_backup *pb,
>  		return -EBADF;
>  	if (clu >= nr_clus || nr > nr_clus - clu)
>  		return -E2BIG;
> -	if (!pb->map)
> -		return -ESTALE;
>  
>  	spin_lock_irq(&pb->lock);
> -	if (pb->destroyed) {
> +		spin_unlock_irq(&pb->lock);
> +		return -ENODEV;
> +	}

[Severity: Low]
Can the -ENODEV and -ESTALE results documented for write() on the fd be
hidden here?

In push_backup_write(), the range check returns -E2BIG before pb->lock is
taken and before pb->destroyed and "!pb->map || !pb->alive" are tested:

	if (clu >= nr_clus || nr > nr_clus - clu)
		return -E2BIG;

	spin_lock_irq(&pb->lock);
	if (pb->destroyed) {

A caller of pb_fd_write() that passes an out of range value to a stopped or
destroyed target gets -E2BIG instead of -ESTALE or -ENODEV. Would it make
sense to test the target state first, or to document the ordering in the
UAPI header?

[ ... ]

> +static const struct file_operations pb_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= pb_fd_read,
> +	.write		= pb_fd_write,
> +	.release	= pb_fd_release,
> +};

[Severity: Low]
This isn't a bug, but pb_fops has no .poll callback. The UAPI header
documents -EAGAIN for O_NONBLOCK read(), yet the fd cannot be used with
poll, select or epoll.

Without it, how is a single-threaded tool expected to wait for readiness
on this fd? It would need a dedicated blocking thread or a busy loop on
O_NONBLOCK read().

pb->waitq and pb_has_event() already exist, so would a .poll implementation
based on poll_wait(file, &pb->waitq, wait) and pb_has_event() be
reasonable here?

> +static int push_backup_get_fd(struct push_backup *pb,
> +			      char *result, unsigned int maxlen)
> +{

[ ... ]

> +	fd_install(fd, file);
> +	return 1;
> +}

[Severity: Low]
Can this leave an installed fd whose number is never delivered to
userspace?

push_backup_get_fd() calls fd_install() and returns 1 from within the
message handler. The result buffer is copied out by dm-ioctl only after
the target returns:

drivers/md/dm-ioctl.c:ctl_ioctl() {
    ...
	if (!r && copy_to_user(user, param, param->data_size))
		r = -EFAULT;
    ...
}

If this copy_to_user() fails, for example because the user buffer is
unmapped or read-only, the ioctl returns -EFAULT. The fd stays installed
in the caller's fd table and nothing rolls it back.

The fd holds a kref on struct push_backup and a module reference on
dm-push-backup through anon_inode_getfile(). The target and the module
therefore stay pinned until the process exits or closes the fd by guessing
its number.

Only a CAP_SYS_ADMIN caller passing a bad buffer can reach this. Would it
be worth documenting this behaviour, or is there a way to defer
fd_install() until the result has been delivered?

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