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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dm-push-backup: fd write races pb_destroy() and uses freed pbio_pool in resubmit_pbios()
- [Low] dm-push-backup: push_backup_get_fd() installs fd before result is delivered
- [Low] dm-push-backup: event fd has no .poll handler, so it cannot be polled
--

commit 013b902b0f480dc76bc700b18a002953deb80967
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

dm/push_backup: add event file descriptor interface

This patch adds an anon-inode file descriptor interface to dm-push-backup.
The fd is obtained with a new push_backup_get_fd target message. Userspace
then uses read() to get the next cluster range to back up and write() to
acknowledge it. A kref on struct push_backup is tied 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

[ ... ]

> @@ -534,6 +591,73 @@ static int push_backup_write(struct push_backup *pb,
>  	return 0;
>  }
>  
> +static ssize_t pb_fd_write(struct file *file, const char __user *buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	struct push_backup *pb = file->private_data;
> +	struct push_backup_range range;
> +	int ret;
> +
> +	if (count != sizeof(range))
> +		return -EINVAL;
> +	if (copy_from_user(&range, buf, sizeof(range)))
> +		return -EFAULT;
> +
> +	ret = push_backup_write(pb, range.clu, range.nr);
> +	return ret ? : sizeof(range);
> +}

[Severity: High]
Can this race with pb_destroy() and use freed memory?

Before this patch, push_backup_write() was reachable only through
pb_message(), which runs under dm_get_live_table() and ctl_rwsem, so it
could not overlap the target destructor. Now pb_fd_write() calls it with
neither.

The dead check in push_backup_write() is made under pb->lock, but
resubmit_pbios() runs after the lock is dropped:

push_backup_write()
    spin_unlock_irq(&pb->lock);
    ...
    if (!list_empty(&pbio_list))
        resubmit_pbios(pb, &pbio_list);

resubmit_pbios()
    list_del_init(&pbio->list);
    dm_requeue_original_rq(pbio->rq);
    mempool_free(pbio, pb->pbio_pool);

If pb_destroy() runs on another CPU after the writer has unlinked the
pbios and dropped the lock, it can proceed as follows:

pb_destroy()
    spin_lock_irq(&pb->lock);
    pb->dead = true;
    ...
    spin_unlock_irq(&pb->lock);
    ...
    destroy_workqueue(pb->wq);
    kvfree(pb->pending_htable);
    mempool_destroy(pb->pbio_pool);
    ...
    pb_put(pb);

The writer would then call mempool_free() on a pbio_pool that
mempool_destroy() has already freed. WARN_ON_ONCE(pb->nr_delayed) in
pb_destroy() would not fire in this case, because the writer has already
decremented nr_delayed.

The new kref keeps only the struct push_backup allocation alive. It does not
keep pbio_pool, pending_htable, wq or origin_dev alive. pb_destroy() does
not wait for fd readers or writers that have already passed the dead check.

The fd can also be inherited or passed to a process other than the one that
opened it. Also, __dm_destroy() suspends the device without draining
postponed requests, so pending pbios can exist when dtr runs.

Is there something that serializes pb_fd_write() against pb_destroy(), for
example draining in-flight fd operations before freeing the sub-resources?

> +static int pb_fd_release(struct inode *inode, struct file *file)
> +{
> +	pb_put(file->private_data);
> +	return 0;
> +}
> +
> +static const struct file_operations pb_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= pb_fd_read,
> +	.write		= pb_fd_write,
> +	.release	= pb_fd_release,
> +};

[Severity: Low]
The interface is described as an event file descriptor, but pb_fops has no
.poll callback.

Can a backup tool multiplex this fd with its other I/O? As written, it can
only block in read() or busy-poll with O_NONBLOCK and get -EAGAIN.

pb_has_event() and pb->waitq already exist and look like a natural basis
for a poll handler. Would it make sense to add .poll before the UAPI is
fixed? Adding it later is harder because read() does not consume anything
and returns 0 to mean "finished".

> +static int push_backup_get_fd(struct push_backup *pb,
> +			      char *result, unsigned int maxlen)
> +{
> +	unsigned int sz = 0;
> +	int fd;
> +
> +	kref_get(&pb->ref);
> +	fd = anon_inode_getfd("[push_backup]", &pb_fops, pb,
> +			      O_RDWR | O_CLOEXEC);
> +	if (fd < 0) {
> +		pb_put(pb);
> +		return fd;
> +	}
> +
> +	DMEMIT("%d", fd);
> +	return 1;
> +}

[Severity: Low]
Can this leak the fd when the result is not delivered?

anon_inode_getfd() does fd_install() irrevocably. DMEMIT() never reports
truncation, so push_backup_get_fd() returns 1 even if the result buffer is
too small and the fd number is truncated or omitted. The same applies if the
dm-ioctl copy-out fails afterwards.

In those cases the caller never learns the fd number, and the open fd
continues to hold a pb kref and a module reference through pb_fops.owner
until the process exits.

Would it be better to check maxlen before creating the fd, or to use
get_unused_fd_flags() and call fd_install() only after the result has been
formatted?

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

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