Re: [Devel] [PATCH hci-8.1 v5 7/7] drivers/md/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: push_backup_get_fd() installs fd before ioctl copy-out can fail
--

commit a842da27ce790f5d0a5ba6d00f2e722207c6d972
Author: Andrey Zhadchenko <andrey.zhadchenko@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 cluster range from that fd and write() an acknowledgement to it, instead of sleeping inside a dm message. The patch also adds a kref on struct push_backup so that it outlives the target while fds remain open.

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 push_backup_get_fd(struct push_backup *pb,
> +			      char *result, unsigned int maxlen)
> +{
> +	unsigned int sz = 0;
> +	struct file *file;
> +	int fd;
> +
> +	fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fd < 0)
> +		return fd;
> +
> +	DMEMIT("%d", fd);
> +	if (sz + 1 >= maxlen) {
> +		put_unused_fd(fd);
> +		return 1;
> +	}
> +
> +	kref_get(&pb->ref);
> +	file = anon_inode_getfile("[push_backup]", &pb_fops, pb, O_RDWR);
> +	if (IS_ERR(file)) {
> +		pb_put(pb);
> +		put_unused_fd(fd);
> +		return PTR_ERR(file);
> +	}
> +
> +	fd_install(fd, file);
> +	return 1;
> +}

[Severity: Low]
Can the fd number be lost after fd_install() here?

The fd is installed into the caller's table before the dm ioctl core has copied the result buffer back to userspace:

ctl_ioctl()
  target_message()
    pb_message()
      push_backup_get_fd()
        fd_install(fd, file);
        return 1;
  ...
  if (!r && copy_to_user(user, param, param->data_size))
          r = -EFAULT;

If that copy_to_user() in ctl_ioctl() fails, for example because the user buffer is read-only or gets unmapped by another thread, the ioctl returns -EFAULT. The fd number was only ever placed in param, so the caller never learns it.

The fd then stays open, and it keeps its reference on pb->ref, until the process exits or happens to close that number.

The overflow check and the anon_inode_getfile() failure path are both unwound before fd_install(), so only the copy-out failure is uncovered.

Would it be possible to deliver the fd number through a path that cannot fail after installation? One option is a dedicated ioctl that does put_user() first and then fd_install().

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

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